Fix: Make bifurcation angles globally invariant

[arnaudon]

If one sets a non-[0,1,0] pia_direction, as it is the case for insitu synthesis, bifurcation angles are 'wrong', in the sense that they will depend on the pia_direction. As this is a global rotation of a cell, the bifurcation angles should be invariant to pia_direction.

As an example, here is a synthesized cell on a plane (z=0 always) with rotation invariance (this PR) and pia_direction = [1, 1, 1]:

and the same setup without this PR:

we see in the first case the original plane (z=0) rotated to match y-> pia_direction=[1,1,1].

[arnaudon]

PS: this PR is based on https://github.com/BlueBrain/NeuroTS/pull/94, which we may want to merge first

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (43a9f80) 97.76% compared to head (1f4a4a6) 97.79%. Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #95 +/- ## ========================================== + Coverage 97.76% 97.79% +0.02% ========================================== Files 39 39 Lines 2197 2219 +22 Branches 381 386 +5 ========================================== + Hits 2148 2170 +22 Misses 30 30 Partials 19 19 ``` | [Flag](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | Coverage Δ | | |---|---|---| | [pytest](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | `97.79% <100.00%> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | Coverage Δ | | |---|---|---| | [neurots/astrocyte/space\_colonization.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9hc3Ryb2N5dGUvc3BhY2VfY29sb25pemF0aW9uLnB5) | `94.32% <100.00%> (ø)` | | | [neurots/generate/algorithms/abstractgrower.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9hbGdvcml0aG1zL2Fic3RyYWN0Z3Jvd2VyLnB5) | `100.00% <100.00%> (ø)` | | | [neurots/generate/algorithms/basicgrower.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9hbGdvcml0aG1zL2Jhc2ljZ3Jvd2VyLnB5) | `100.00% <100.00%> (ø)` | | | [neurots/generate/algorithms/tmdgrower.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9hbGdvcml0aG1zL3RtZGdyb3dlci5weQ==) | `99.24% <100.00%> (ø)` | | | [neurots/generate/grower.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9ncm93ZXIucHk=) | `100.00% <ø> (ø)` | | | [neurots/generate/orientations.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9vcmllbnRhdGlvbnMucHk=) | `100.00% <ø> (ø)` | | | [neurots/generate/tree.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS90cmVlLnB5) | `100.00% <100.00%> (ø)` | | | [neurots/morphmath/bifurcation.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9tb3JwaG1hdGgvYmlmdXJjYXRpb24ucHk=) | `100.00% <100.00%> (ø)` | |

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.65217% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 97.79%. Comparing base (43a9f80) to head (2a47bca). Report is 3 commits behind head on main.

:exclamation: Current head 2a47bca differs from pull request most recent head 46d8912. Consider uploading reports for the commit 46d8912 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #95 +/- ## ========================================== + Coverage 97.76% 97.79% +0.02% ========================================== Files 39 39 Lines 2197 2224 +27 Branches 381 387 +6 ========================================== + Hits 2148 2175 +27 Misses 30 30 Partials 19 19 ``` | [Flag](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | Coverage Δ | | |---|---|---| | [pytest](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | `97.79% <95.65%> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | Coverage Δ | | |---|---|---| | [neurots/generate/algorithms/tmdgrower.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9hbGdvcml0aG1zL3RtZGdyb3dlci5weQ==) | `99.25% <100.00%> (+0.01%)` | :arrow_up: | | [neurots/generate/grower.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9ncm93ZXIucHk=) | `100.00% <ø> (ø)` | | | [neurots/generate/tree.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS90cmVlLnB5) | `100.00% <ø> (ø)` | | | [neurots/morphmath/bifurcation.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9tb3JwaG1hdGgvYmlmdXJjYXRpb24ucHk=) | `100.00% <100.00%> (ø)` | | | [neurots/generate/algorithms/basicgrower.py](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#diff-bmV1cm90cy9nZW5lcmF0ZS9hbGdvcml0aG1zL2Jhc2ljZ3Jvd2VyLnB5) | `93.75% <71.42%> (-6.25%)` | :arrow_down: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/BlueBrain/NeuroTS/pull/95/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain)

[arnaudon]

thanks @adrien-berchet , @lidakanari can we merge this?

[eleftherioszisis]

Shouldn't the pia_direction be passed from the context or some other abstraction? You are passing the argument pia_direction to classes that are agnostic of a spatial embedding.

[arnaudon]

it is passed via parameters file

[eleftherioszisis]

it is passed via parameters file

You are passing pia_direction and pia_rotation to functions/classes that should know nothing about a spatial context. Some of them are purely mathematical. Maybe you meant to name it global_orientation|direction or something like that?

[arnaudon]

is pia bothering you? its legacy, I'm to lazy to change its name

[lidakanari]

My main concern is that the 'pia_direction' does not correspond to the orientation that is set as input for the apical. This could potentially cause an issue if one of the trees needs to grow towards a different direction, while the others grow to other directions.

Since this directionality is only exposed at the tree level, it is not obvious how the growth of the neuron as the whole will be affected. Shouldn't this impact be global rather than local at the tree level?

In addition, the global orientation has not been an issue for any other cell type apart from "flat" cells. Was this wrong so far but we missed it in the morphometrics? I am not sure how this correction will impact the pyramidal cells for example that are currently growing as expected taking into account the computed angle distributions. Could we add a test that this PR fixes so that we are aware of the improvement? It will be more clear this way :)

[arnaudon]

@lidakanari so I did a complete refactoring, it was indeed not making so much sense. I did the following:

• renamed PIA_DIRECTION to Y_DIRECTION
• renamed pia_direction to y_direction
• I pass pia_direction via the context, as it really is a context dependent parameter, it simplifies a lot of things in fact
• I added an example script (which I can move to the tests once this is agreed solution) that computes a neuron without this option, with this option set to default [0, 1, 0] value and with value [1, 0, 0].
• I made sure the resulting neuron when grown with y_direction = [1, 0, 0] is exactly the same as default one, when rotated from x to y.
• this works only with the trunk angle algo I implemented a while ago, the original one I didn't touch for this, as I don't use it via region-grower
• For this, I had to also rotate random numbers in the growth of section, which may be add a bit of computational cost for nothing, except ensuring neurons are the same, regardless of pia_direction value. We may not want this level of 'rotation invariance', but as you want.

I hope this is more clear now!