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.

Sign up for GitHub

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

Jump to bottom

Add more options for ASC and SWC loaders #431

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add more options for ASC and SWC loaders #431

wants to merge 1 commit into from

Conversation

adrien-berchet
Copy link
Member

Fixes #427

@adrien-berchet
Copy link
Member Author

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
Copy link
Member Author

@eleftherioszisis and @mgeplf WDYT?

@adrien-berchet adrien-berchet mentioned this pull request Nov 29, 2022
Draft
matz-e
matz-e reviewed Dec 7, 2022
View reviewed changes
include/morphio/enums.h Outdated Show resolved Hide resolved
src/readers/morphologyASC.cpp Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
@adrien-berchet
Copy link
Member Author

@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
Copy link
Collaborator

eleftherioszisis commented Feb 14, 2023 •
edited
Loading

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
Copy link
Member Author

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 😇 ).
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
Copy link
Member

matz-e commented Feb 14, 2023

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
Copy link
Member Author

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
Copy link
Member

matz-e commented Feb 14, 2023

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
Copy link
Member Author

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
Copy link
Member Author

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

@adrien-berchet
Copy link
Member Author

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

@adrien-berchet
WIP: Add options to disable some checks
4296eb9 
Add soma bifurcation for ASC files

Add new options and tests

Use proper warnings and fix tests

Rename single point section into root bifurcation

Add bindings

Cleaning

Clean tests

Apply clang-format

Improve readability of enum Option

Remove duplicated test

Restore add_test in else() block

Add option for root point not equal to -1 in SWC files

Add Python bindings for custom root id

Add tests for conversions

Check that converted files can be read
@adrien-berchet
Copy link
Member Author

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

@mgeplf
Copy link
Contributor

mgeplf commented Sep 29, 2023

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

@adrien-berchet
Copy link
Member Author

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 😜
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matz-e matz-e matz-e left review comments

@mgeplf mgeplf Awaiting requested review from mgeplf

@eleftherioszisis eleftherioszisis Awaiting requested review from eleftherioszisis

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a flag to disable some checks
4 participants
@adrien-berchet @eleftherioszisis @matz-e @mgeplf