search

BlueBrain / MorphIO

A python and C++ library for reading and writing neuronal morphologies
https://morphio.readthedocs.io
Apache License 2.0
26 stars 22 forks source link

Add more options for ASC and SWC loaders #431

Open adrien-berchet opened 1 year ago

adrien-berchet commented 1 year ago

Fixes #427

adrien-berchet commented 1 year ago

I did not add options for all checks, just for the ones that might be needed for TMD.

Also, I did not change the HDF5 reader at all for two reasons:

  1. its implementation is deeply designed according to the specifications given in https://github.com/BlueBrain/morphology-documentation/blob/main/source/h5v1.rst, so it would be hard and dangerous to remove some checks.
  2. the morphologies written in this format mainly come from MorphIO which do not allow to write morphologies that do not follow its specifications, so I don't think we will ever find a HDF5 file containing a morphology that can not be loaded with the current loader.
adrien-berchet commented 1 year ago

@eleftherioszisis and @mgeplf WDYT?

adrien-berchet commented 1 year ago

@matz-e @eleftherioszisis @mgeplf Could you tell me if this PR looks relevant to you (we can speak about implementation details later)? If it does I will add some docs about these options. Thanks

eleftherioszisis commented 1 year ago

Could you tell me if this PR looks relevant to you (we can speak about implementation details later)? If it does I will add some docs about these options. Thanks

I am in favor of adding these options if they aid in processing morphologies with artifacts.

However, my main concern is about the repercussions of using these options and then converting between formats. I believe we need way more tests to ensure that the converters will not break when using these options.

adrien-berchet commented 1 year ago

However, my main concern is about the repercussions of using these options and then converting between formats. I believe we need way more tests to ensure that the converters will not break when using these options.

I think they will because the HDF5 format is very strict and I did not change much in this one because I think the HDF5 files are mainly created using MorphIO, even though they could be created in another way (and also because the HDF5 part of MorphIO is harder to change :innocent: ). But I don't think it's really an issue if these flawed morphologies can not be written before they are fixed. But I am going to add some tests to see which cases are possible and which are not.

matz-e commented 1 year ago

I am in favor of adding these options if they aid in processing morphologies with artifacts.

However, my main concern is about the repercussions of using these options and then converting between formats. I believe we need way more tests to ensure that the converters will not break when using these options.

Same. Also, this should not create any illusion that all our software can run with non-fixed morphologies. We have a pretty rigid idea of how a morphology should look like (exactly 1 soma, e.g.)

adrien-berchet commented 1 year ago

Same. Also, this should not create any illusion that all our software can run with non-fixed morphologies. We have a pretty rigid idea of how a morphology should look like (exactly 1 soma, e.g.)

That's why I added warnings in all irregular cases, but you think it's not enough?

To be more clear I improved the conversion test to check that morphologies that can be written can then be read without any specific option. This means that the irregular morphologies can be loaded with the new options BUT when they can be written (mainly for soma issues) they are automatically fixed (not in a smart way but just to make it work). This only happens for weird somata (e.g. multiple somata or somata with bifuractions). So the user gets a warning but if he is able to write it, whether he manually fixes the morphology or he lets MorphIO to fix it automatically if it is possible, then MorphIO will be able to load it with the default loader afterwards.

matz-e commented 1 year ago

but you think it's not enough?

I'm more concerned. Let's hope it's enough. People being aware helps a lot already.

adrien-berchet commented 1 year ago

but you think it's not enough?

I'm more concerned. Let's hope it's enough. People being aware helps a lot already.

Ok :)

adrien-berchet commented 1 year ago

Do you have any other comment or any suggestion before merging?

adrien-berchet commented 1 year ago

Anything about this PR? @matz-e @mgeplf @eleftherioszisis

adrien-berchet commented 11 months ago

@mgeplf @eleftherioszisis @matz-e up :) I rebased and it covers everything I need for now so for me it's ready.

mgeplf commented 11 months ago

Thanks, I'm still far behind on fixing things :( I will add it to the list of things to do....

adrien-berchet commented 11 months ago

Thanks, I'm still far behind on fixing things :( I will add it to the list of things to do....

Ahah no problem, at least now it will be in your todo list :stuck_out_tongue_winking_eye: Thanks!