BlueBrain / NeuroM

Neuronal Morphology Analysis Tool
https://neurom.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
103 stars 55 forks source link

Type of Neuron.root_sections != Neuron.sections #971

Closed adrien-berchet closed 2 years ago

adrien-berchet commented 3 years ago

Neuron.sections returns a list of neurom.core.neuron.Section objects while Neuron.root_sections returns a list of morphio._morphio.mut.Section objects, which is quite disturbing. I am wondering if we should not override the root_sections method to return neurom.core.neuron.Section objects, I think it would be more consistent. WDYT @eleftherioszisis ?

(moved https://github.com/BlueBrain/MorphIO/issues/334 here since it more related to NeuroM)

eleftherioszisis commented 3 years ago

While it is trivial to override the in Morphology the morphio.mut.Morphology method root_sections, the real question here is what would be the repercussions of not returning morphio objects anymore.

For tools that use the morphio object underneath for mutations, this could mean a breaking change. However, it is sensible that the neurom Morphology should provide neurom objects and not mixed ones. Thus IMO it should override all the morphio methods and return consistent types.

@mgeplf @arnaudon

arnaudon commented 3 years ago

+1 for consistent types in neurom, regardless on how we access them. One can still access the protected morphio object from neurom, right?

eleftherioszisis commented 3 years ago

Currently neurom Morphology inherits from morphio mut.Morphology, therefore it is not trivial to access the parent methods once you override them.

If neurom morphology allowed you to do all the operations you would do with morphio, why would you need to access morphio's methods?

arnaudon commented 3 years ago

ah ok, I vaguely remember needing to do something like that a long time ago. But you are right, there is no need really. Eventually, one can load with morphio, and convert to neurom using Neuron class I think.

adrien-berchet commented 3 years ago

Currently neurom Morphology inherits from morphio mut.Morphology, therefore it is not trivial to access the parent methods once you override them.

Maybe we could rename them with a morphio_ prefix for example, like soma in the current version: https://github.com/BlueBrain/NeuroM/blob/master/neurom/core/morphology.py#L435

eleftherioszisis commented 3 years ago

I am not convinced that having methods named morphio_ would bring anything to the table. If someone would like to extract morphometrics and mutate the morphology why not use exclusively the neurom interface? Otherwise, one could use directly morphio.

adrien-berchet commented 3 years ago

I am not convinced either, it's just it is already the case for soma so it could be a way to do it. But it's not my preferred way :-)

I think an interesting way of doing this is what Pandas does (see here for example: https://github.com/pandas-dev/pandas/blob/v1.3.3/pandas/core/frame.py#L564). When we inherit from pandas.DataFrame for example, we have to define which class is used to cast the result of each method. So we could have a similar system that just casts the results of the methods to NeuroM objects. And if one wants to get a MorphIO object from a NeuroM object we could add a method like object.to_morphio (or whatever name) and then one would get a morphio ojbect whose methods return other morphio objects. So it ensures consistency.

mgeplf commented 2 years ago

I think we'll be looking at this for the 'v4' version of NeuroM (which will also change the Axon-on-Dendrite handling).

In my opinion: NeuroM should only work with immutable morphologies; any changes to a morphology can be accomplished by converting it into a MorphIO::mut::Morphology, and if further analysis is needed, it can be loaded by NeuroM

One can still access the protected morphio object from neurom, right?

Yes, I think we'd have a Morphology.to_morphio() method, or something like it.

arnaudon commented 2 years ago

Tbh, given this never ending struggle between morphio/neurom, I would deprecate neurom, and put all the feature extraction stuff of neurom in morphio, so it's all consistent. I don't know the origin of having to similar codes handling morphologies, but as far as I can see, it was a pretty bad move haha

adrien-berchet commented 2 years ago

Tbh, given this never ending struggle between morphio/neurom, I would deprecate neurom, and put all the feature extraction stuff of neurom in morphio, so it's all consistent. I don't know the origin of having to similar codes handling morphologies, but as far as I can see, it was a pretty bad move haha

I agree with that, IMO NeuroM should only contain analysis functions that should just be applied to MorphIO objects (that's what I talked about for the GSoC project). But ofc it's quite a big work.

mgeplf commented 2 years ago

They have different targets:

adrien-berchet commented 2 years ago

Ah sorry, I read too fast @arnaudon 's message, I actually don't fully agree with it :smile: I thought it was just about deprecating the specific Morphology, Section, etc. classes from NeuroM, not the entire code. I agree that having two codes with different targets is a good thing. And in this case many codes just don't need to compute morphometrics and only need to process morphologies on their own, so they are supposed to just need MorphIO. But what I mean is that these two codes are not properly separated at the moment since NeuroM introduces specific classes instead of just using the raw objects from MorphIO to compute the morphometrics. This lead sometimes to use the NeuroM classes just to process the morphologies because we want to use the helpers introduced by NeuroM to access sections or points of the morphology, even if we don't care about the morphometrics. On the contrary, if we merge the neurom.core.Morphology and morphio.Morphology classes, then NeuroM will only take a MorphIO object, compute the morphometrics and return the results. I think the roles of each code would be clearer that way since it would be: MorphIO is only intended to load, update and save morphologies while NeuroM is only intended to compute morphometrics.

mgeplf commented 2 years ago

@adrien-berchet: I think we're close to on the same page; I agree with the which is quite disturbing sentence you wrote at the start of the thread, and that's what we should aim towards, imo.

Basically, the fact that NeuroM uses MorphIO is an implementation detail, that shouldn't be something the user needs to know about. I thik we can provide from_morphio() and to_morphio() methods, but that's the maximum of the public API that is MorphIO aware, and that a library user can rely on.

Anyways, we're actively looking at this now, and will get back to you. (Note: I'm also keeping the iterator points you mentioned wrt MorphIO in the back of my head - it hasn't been forgotten :))

adrien-berchet commented 2 years ago

Ok, look good! Let's keep in touch about this. Thanks :)

eleftherioszisis commented 2 years ago

I am closing this issue, as it has been tackled in https://github.com/BlueBrain/NeuroM/commit/d66d0370eff3224bdcc9ed2f1b17ef5b55ce63d9 and will be available in NeuroM v4.