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

@@            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	Coverage Δ
pytest	97.79% <100.00%> (+0.02%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
neurots/astrocyte/space_colonization.py	94.32% <100.00%> (ø)
neurots/generate/algorithms/abstractgrower.py	100.00% <100.00%> (ø)
neurots/generate/algorithms/basicgrower.py	100.00% <100.00%> (ø)
neurots/generate/algorithms/tmdgrower.py	99.24% <100.00%> (ø)
neurots/generate/grower.py	100.00% <ø> (ø)
neurots/generate/orientations.py	100.00% <ø> (ø)
neurots/generate/tree.py	100.00% <100.00%> (ø)
neurots/morphmath/bifurcation.py	100.00% <100.00%> (ø)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.85%. Comparing base (2ef760f) to head (d318f82). Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
neurots/generate/grower.py	90.90%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
- Coverage   97.88%   97.85%   -0.03%     
==========================================
  Files          39       39              
  Lines        2217     2242      +25     
  Branches      296      302       +6     
==========================================
+ Hits         2170     2194      +24     
  Misses         29       29              
- Partials       18       19       +1     

Flag	Coverage Δ
pytest	97.85% <98.27%> (-0.03%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
neurots/extract_input/from_neurom.py	100.00% <100.00%> (ø)
neurots/generate/algorithms/abstractgrower.py	100.00% <100.00%> (ø)
neurots/generate/algorithms/basicgrower.py	100.00% <100.00%> (ø)
neurots/generate/algorithms/tmdgrower.py	99.24% <100.00%> (ø)
neurots/generate/orientations.py	100.00% <100.00%> (ø)
neurots/generate/tree.py	100.00% <ø> (ø)
neurots/morphmath/bifurcation.py	100.00% <100.00%> (ø)
neurots/utils.py	100.00% <100.00%> (ø)
neurots/generate/grower.py	99.39% <90.90%> (-0.61%)	:arrow_down:

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[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!