BlueBrain / NeuroR

A collection of tools to repair morphologies
https://neuror.readthedocs.io/
GNU Lesser General Public License v3.0
9 stars 6 forks source link

Sanitize deletes all children sections when parent section length is zero #99

Closed lidakanari closed 2 years ago

lidakanari commented 2 years ago

Hi, @Stannislav noticed that during sanitize of a neuron, if a section has zero length subsequent sections are deleted as well. This is probably coming from here:

https://github.com/BlueBrain/NeuroR/blob/79b4394a79fa60f3b26f5894ff1ce44d124e8945/neuror/sanitize.py#L112

This should not be the expected behavior for sanitize (imo). Instead when a zero length section is removed subsequent sections should be stitched back to its parent.

Could you please look into this issue? Thanks a lot!

Stannislav commented 2 years ago

Thanks @lidakanari for reporting this.

I think it boils down to morphio.mut.Morphology.delete_section deleting all subsequent sections, or so it seems.

eleftherioszisis commented 2 years ago

Do you have by any chance an example reproducing the reported behavior so that I could use it as a guide?

Stannislav commented 2 years ago

@eleftherioszisis Here's an example morphology for which whole neurites are removed: AA0066.zip

Script to reproduce:

# bug.py
from morphio import Morphology
from neuror.sanitize import fix_non_zero_segments

def show_neurites(m: Morphology) -> None:
    for sec in m.root_sections:
        print(f"* {sec.type.name}")
    print()

neuron = Morphology("AA0066.ASC")

print("Neurites before:")
show_neurites(neuron)

neuron_sanitized = fix_non_zero_segments(neuron)

print("Neurites after:")
show_neurites(neuron_sanitized)

output:

$ python bug.py 
Neurites before:
* axon
* basal_dendrite
* basal_dendrite
* basal_dendrite
* basal_dendrite
* basal_dendrite
* basal_dendrite
* basal_dendrite
* apical_dendrite

Neurites after:
* axon
* basal_dendrite
* basal_dendrite
* basal_dendrite
* basal_dendrite
eleftherioszisis commented 2 years ago

delete_section in MorphIO has a recursive flag which is by default true:

https://github.com/BlueBrain/MorphIO/blob/bd2e18e95a86d92a40a671171463df3838a81c19/binds/python/bind_mutable.cpp#L104-L114

Setting it to false should only delete that section. I will send a PR, but is it possible to try it out locally on your side in the meantime?

https://github.com/BlueBrain/NeuroR/blob/79b4394a79fa60f3b26f5894ff1ce44d124e8945/neuror/sanitize.py#L139

Should be neuron.delete_section(section, recursive=False)

eleftherioszisis commented 2 years ago

@Stannislav , I am afraid that in the end, the issue is not as simple as I first thought.

Deleting root sections with zero length while keeping the rest of the tree will result in adding two neurites where there was originally one. That could result in invalid cases where the cell would end up with two apicals or axons for example.

In addition, deleting a non-root section that is not a leaf would result in creating a multifurcation.

I believe first we need to decide if the side-effects above is really what we want. What do you think?

lidakanari commented 2 years ago

For me the main problem is the case in which the root is the zero-length section. If it's a subsequent section and this creates a multifurcation I don't think that is causing any serious problem down the line (we already have some multifurcations in our datasets). For the root section, could we move the point by a minimum acceptable distance i.e. 0.1um to create a non-zero section that will not damage the integrity of the subtree? I am not sure if that's the best solution but just an idea to consider.

Stannislav commented 2 years ago

@eleftherioszisis I guess these are the two use cases you're talking about? (sorry for the bad drawing) NeuroR sanitize

Stannislav commented 2 years ago

I did think something like that might happen. I had a thought like @lidakanari as well if it isn't possible to stretch the root segment to be non-zero length.

Stannislav commented 2 years ago

Iin all Janelia data the problematic sections always have only two identical points and one segment that connects them. Not that it helps to solve the problem, but it's always this specific single-segment case. Maybe it can help to trace back to origin of these artifacts?

eleftherioszisis commented 2 years ago

@eleftherioszisis I guess these are the two use cases you're talking about? (sorry for the bad drawing) NeuroR sanitize

Indeed, these are the two cases.

I did think something like that might happen. I had a thought like @lidakanari as well if it isn't possible to stretch the root segment to be non-zero length.

Extending the zero-length root section by a small offset is feasible. I have the following questions:

eleftherioszisis commented 2 years ago

Another case would be if one leaf is a non-zero length section. Upon removal, it will result in a unifurcation at that point.

lidakanari commented 2 years ago

@wvangeit what is your input on this? The sanitization to remove zero length sections is used for simulations afaik.

Does anything break if the root section is a bifurcation?

Does anything break with multifurcations?

If we add a "almost" zero length instead would it be preferable?

eleftherioszisis commented 2 years ago

Does anything break if the root section is a bifurcation?

Just to clarify, MorphIO is designed around section-centered connectivity. Therefore, it is not possible to have a bifurcation with no parent section.

lidakanari commented 2 years ago

OK, that explains the issue further. So I guess the only solution is to add some minimal length. We just need to check that this will not break anything. Also an important note, be careful on the diameter that this minimal section will have. It should be consistent with the subsequent sections in order not to break the electrical models!

wvangeit commented 2 years ago

I guess there is only a real problem if this happens at the root? In the other cases it seems fairly easily solvable to me? I.e. creating multifurcations, or merging sections together depending on how many sections come together after the removal of the bad section? Afaik the simulation will indeed have problem when there is a zero-length section. For the root case, it could indeed make sense to add a minimial diameter, not sure what a good value would be though, maybe @jamesgking knows? But does this not point to a problem of the morphology though? It seems biologically weird to have sections converging onto a zero-length root section.

wvangeit commented 2 years ago

(Btw, i'm also not sure it's ok to assume only the simulation would have a problem with zero lengths. There is a possibility other tools might also not be happy, like touch detection or so)

mgeplf commented 2 years ago

(sorry to join this late, I was off)

But does this not point to a problem of the morphology though?

That's my thought as well; I think we need to be careful if we start modifying the morphology, we lose the intent of what was being produced, and what might be a reasonable fix for one laboratory's quirks may not apply to other places where sanitization is used.

lidakanari commented 2 years ago

Hello, any updates on this one? I agree this is a problem of the reconstruction but we need to deal with this issue in a "sane" way to avoid breaking all other tools. The current behavior to remove the whole tree is certainly going to cause more issues than any other solution. So what would be the suggestion from your side @mgeplf @eleftherioszisis ? Would not using sanitize at all instead of creating an intermediate solution be preferable?

arnaudon commented 2 years ago

ah this is interesting thread I didn't see! It seems these 2 points on same place (not root) is just there way to have multifurcation that works in their codes? For root sections, I guess two root sections could be attached to soma on the same location, and when they clicked on neuroluscida on the same point twice it created this 0 length segment? In your morph above it seems it has 2 apicals then, and several more basals. I would just sanitize these by splitting the dendrites in 2 with same first point. For multifurcations, I think we have already some, which does not cause troubles afaik.

eleftherioszisis commented 2 years ago

Introducing arbitrary rules and artificial branches is a dangerous path for the "correctness" of morphology. These cases go beyond a simple sanitization because they require changing the topology of the morphology. Therefore, we need to be certain and have a way to justify these changes if we are going to use these morphologies for publications.

@arnaudon , this is my feeling too, but would it be correct to increase the number of neurites by splitting them?

arnaudon commented 2 years ago

I'm not sure where this data is from, and if it has a paper that goes with it, but may be it could be possible to trace back how many neurite they considered for some morphologies, or something like that, to be sure? I guess under the microscope it would look like 2 touching neurite trunks or something like that. Or maybe just ask somebody at janelia related to this dataset?

Stannislav commented 2 years ago

@eleftherioszisis @arnaudon The example file I attached above is from the Janelia dataset (the parent folder name was mouselight_isocortex_ASCII_Files). @lidakanari probably has all the details on this dataset.

lidakanari commented 2 years ago

The data come from here: http://ml-neuronbrowser.janelia.org/. Relevant publications from this list seem to be the 2019 - 2020: https://www.janelia.org/project-team/mouselight/publications?sort_by=field_date&items_per_page=10&redirect=1 But given the amount of reconstructions I doubt we can trace back the exact morphology and check what they saw in the microscope :/ but this issue goes beyond this dataset, it's a problem that might be rare but can potentially break a lot of subsequent steps. So it is indeed crucial to decide what is the correct behavior.

arnaudon commented 2 years ago

it seems they come from this paper: https://www.sciencedirect.com/science/article/pii/S0092867419308426?via%3Dihub which says they use this tool to manipulate morphologies : https://github.com/JaneliaSciComp/workstation we can just open some with there tool to see how these 0 lengths segments are interpreted?

arnaudon commented 2 years ago

also, have a look at the mmc3 video in the SI of this paper, pretty cool semi-automatic reconstructions tool they have! We see they have to click on which sections connect to which other sections. Maybe it's the semi-automated thing that creates the 0 len segments, as when they click near a reconstructed segment it automatically finds the closest one, so people don't have to be very precise when they attach it together.

adrien-berchet commented 2 years ago

Isn't the simplest solution to move the bifurcation point along the bisector of the bifurcation angle? So the starting point of the 0-length section is not moved and only the end point is moved (at 1um distance). It would increase the bifurcation angle a (little) bit but I think it would be the closest result from the initial morph that does not break anything and does not create multifurcations. image_inkscape

arnaudon commented 2 years ago

FYI: we are trying this fix: https://github.com/BlueBrain/morphology-workflows/pull/31

arnaudon commented 2 years ago

@adrien-berchet , I just saw your comment above, I move it in the direction of the center of soma. But your suggestion seem good as well. I don't thing we'll see a difference between these implementations, but I don't mind using yours

adrien-berchet commented 2 years ago

I think my solution is more generic. For example, I am wondering if it is possible that a 0-length section is located at the same point than the soma center? For example in the case of a soma with radius equal to 0 (since we are speaking about weird morphologies...). In that case moving the point in the soma direction would not work. But as you prefer.

arnaudon commented 2 years ago

yes, agreed! I'll update the PR, I didn't think of that option!

mgeplf commented 2 years ago

Per the talk today, my understanding of the plan for NeuroR: 1) Cases where a whole sub-tree are deleted is a bug; and should be fixed when we repro it 2) Zero length sections coming from the soma will cause an exception for sanitization, as it's a case where there isn't a one-size-fits-all solution; these can be fixed in at another stage of the pipeline, when it's known what the solution is based on the lab/agreed upon solution.

FrancescoCasalegno commented 2 years ago

@mgeplf to summarize and link to existing issues / pull request, this is my understanding. Please correct me if I am wrong.

  1. 0-length section at the soma
    1. NeuroR's sanitize should raise an error and stop if such a 0-length section is found at the soma beacuase no one-size-fits-all solution could be identified --> https://github.com/BlueBrain/NeuroR/issues/106
    2. CheckNeurite stage from morphology_workflows, which Sanitize stage depends from, will make sure that such 0-length sections at the soma are fixed (e.g. change to a very short length) so that Sanitize will not raise errors --> https://github.com/BlueBrain/morphology-workflows/pull/31
  2. 0-length section not at the soma
    1. In principle nothing strange should happen, we should just merge the nodes at the boundary of the section with 0 length. However, there currently seems to be issues in some cases, where the whole sub-tree is deleted even if we are not at the soma, and we need to fix that --> https://github.com/BlueBrain/NeuroR/issues/105
eleftherioszisis commented 2 years ago

107 fixes the recursive deletion and makes sanitize throw an error if a root section with zero length is present in the morphology.