Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a flag to disable some checks #427

Open
adrien-berchet opened this issue Nov 14, 2022 · 6 comments · May be fixed by #431
Open

Add a flag to disable some checks #427

adrien-berchet opened this issue Nov 14, 2022 · 6 comments · May be fixed by #431

Comments

@adrien-berchet
Copy link
Member

MorphIO performs many checks when loading a file, but sometimes these checks are more restrictive than the standard (e.g. soma representation is more restrictive than the standard, it does not accept custom tree types, it does not accept bifurcations at the first point, etc.). It would be nice to be able to choose which level of checks are performed. We could start with just 2 levels: the min level for MorphIO to be able to load the file and the max level which is the current behavior. Later we could add a few other intermediate levels if it is needed.

@lidakanari
Copy link

Here is an example that fails:

https://neuromorpho.org/neuron_info.jsp?neuron_name=Neuron-8-E-and-I-synapses
https://neuromorpho.org/dableFiles/iascone/CNG%20version/Neuron-8-E-and-I-synapses.CNG.swc

The file is:
139305.zip

And fails to load because of the following error:

n = tmd.io.load_neuron_from_morphio('139305.swc')
---------------------------------------------------------------------------
SomaError                                 Traceback (most recent call last)

----> 1 n = tmd.io.load_neuron_from_morphio('./Complete/139305.swc')

File ~/BBP/bbpreps/GitHub/BBP/TMD/tmd/io/io.py:160, in load_neuron_from_morphio(path_or_obj, user_tree_types)
    157 tree_types = redefine_types(user_tree_types)
    159 if isinstance(path_or_obj, (str, Path)):
--> 160     obj = Morphology(path_or_obj)
    161     filename = path_or_obj
    162 else:

SomaError: 
./Complete/139305.swc:19:error
Found soma bifurcation
The following children have been found:
./Complete/139305.swc:20:warning
./Complete/139305.swc:21:warning

@eleftherioszisis
Copy link
Collaborator

eleftherioszisis commented Nov 14, 2022

@adrien-berchet

MorphIO performs many checks when loading a file, but sometimes these checks are more restrictive than the standard

which standard? swc, h5v1, or ascii?

it does not accept custom tree types

I think this is doable, however not sure how difficult in the c++ implementation. In general, MorphIO should support all custom types in the swc spec.

it does not accept bifurcations at the first point

This is a limitation of the representation which is section-based. MorphIO has to satisfy all supported representations, and some point-based features are just not compatible with all three..

@lidakanari
I agree that MorphIO should not make assumptions on the soma representation, however it does and on top of that has methods for surface area and volume. Changing that at this point would be disruptive for tools that depend on these representations on both python and c++ sides.

@lidakanari
Copy link

lidakanari commented Nov 14, 2022

@eleftherioszisis I agree this might be disruptive for other tools. However, in some cases where we need for example to view the morphology, or compute persistence, it does not make sense to discard the whole cell. I propose to have an optional argument to resolve these cases. The default behavior does not need to change. However, we can set the optional argument to "not fail the loading for non breaking issues" in software such as TMD. In addition, it would be important to still show a warning so the user is aware of the unusual behavior.

I would suggest to at least try to support the data that are provided by neuromorpho.org (so swc variants)

@mgeplf
Copy link
Contributor

mgeplf commented Nov 15, 2022

We currently have to focus on the MMB work; I'm open to the idea, so if you want to make an example PR that we can look at, that would work.

I have a similar concern to @eleftherioszisis, in that we currently assume that the results of the parser stage pass all the invariants that the rest of the code depends on. Relaxing the parser means these invariants would have to be checked elsewhere, or that the code may crash or produce incorrect results. Fixing all these would be extremely difficult, because we'd probably only run across them when problems are raised.

@adrien-berchet
Copy link
Member Author

I explored the code a bit and there is a options parameter that we can pass to the Morphology constructor to enable/disable some internal processes. Maybe we could just use this mechanism to solve this issue? As far as I can see, we basically 'just' have to add new entries in the morphio::enums::Option enum and then use these new options in the readers. WDYT?

@mgeplf
Copy link
Contributor

mgeplf commented Nov 16, 2022

As far as I can see, we basically 'just' have to add new entries in the morphio::enums::Option

Yup, makes sense.

@adrien-berchet adrien-berchet linked a pull request Nov 25, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants