Skip to content
This repository was archived by the owner on Feb 26, 2025. It is now read-only.

Sanitize with optional inhomogeneous check #97

Closed
wants to merge 1 commit into from

Conversation

eleftherioszisis
Copy link
Contributor

Provides the option to disable the inhomogeneous neurites check in sanitization. (#96)

@arnaudon
Copy link
Contributor

arnaudon commented Feb 2, 2022

I'm not sure this is a good idea, if this is not handled properly in other codes. At least here (for the few using this function) there will be a clear warning sign. If you want to merge that quickly, I would keep the check, but if this option is on, I would only raise a warning and let it pass. Otherwise, I'll wait to sort it out globally, then adapt this function so it matches whatever implementation of this axon on dendrite will be decided.

@eleftherioszisis
Copy link
Contributor Author

If you want to merge that quickly

Not at all. This is PR was mainly to probe for requirements and open a discussion. Think of it as a first draft.

I'm not sure this is a good idea, if this is not handled properly in other codes.

Would you elaborate a bit more on this? The default usage in which allow_inhomogeneous_trees=False is identical to how it was used before this change.

At least here (for the few using this function) there will be a clear warning sign.

Not sure I understand this bit. Do you want to emit a warning when inhomogeneous trees are encountered? Before this change there would be a CorruptedMorphology error if sanitize was run on an inhomogeneous morphology. For sanitize_all it would just drop that file in which the error was raised. But it is an error, nevertheless.

Otherwise, I'll wait to sort it out globally, then adapt this function so it matches whatever implementation of this axon on dendrite will be decided.

Keep in mind that we shouldn't be too neuron-centric because there are other types of cells too. This is why I opted for a generic allow_inhomogeneous_trees flag instead of something more specific. However, if NeuroR is strictly destined for neurons, then it would make sense to be more specific and check for the axon_on_dendrite type and let only that mixed tree pass.

@arnaudon
Copy link
Contributor

arnaudon commented Feb 2, 2022

  1. ok!
  2. my line implies: if you allow to bypass it, people will use it, then it won't be compatible with other tools. If I understood well (@mgeplf could confirm), this sanitize function was meant to be a non-optional/non-configurable one, that everybody has to 'pass' before using morphologies. If we start having options for each thing it does, it's better not to have it, and call the various checks/sanitize by hand depending on usecases (which we can still do if we want).
  3. from 2., if somebody has this flag=True in a code, forget it, and has morphologies which would crash, it would go un-noticed, and maybe the axon will disappear (or whatever may happen later). I think this new flag=True, should just convert the raise to a warning, as it is still dangerous to have such morphologies atm
  4. no no, generic is fine, I was just referring to the other discussion

@eleftherioszisis
Copy link
Contributor Author

if you allow to bypass it, people will use it, then it won't be compatible with other tools.

Indeed, but that's the purpose of the flag, to begin with, isn't it? If someone would like to work with mixed trees then it would make sense for this check to be optional.

If I understood well (@mgeplf could confirm), this sanitize function was meant to be a non-optional/non-configurable one, that everybody has to 'pass' before using morphologies. If we start having options for each thing it does, it's better not to have it, and call the various checks/sanitize by hand depending on usecases (which we can still do if we want).

My understanding from the following comment: BlueBrain/NeuroM#977 (comment)
was that sanitize, the way it is now it prevents blocks alternative workflows in which mixed trees are allowed. Otherwise, I doubt that sanitization which allows axons on dendrites would be part of the standard workflow anytime soon, if ever.

from 2., if somebody has this flag=True in a code, forget it, and has morphologies which would crash, it would go un-noticed, and maybe the axon will disappear (or whatever may happen later). I think this new flag=True, should just convert the raise to a warning, as it is still dangerous to have such morphologies atm

Indeed. I will change it into emitting a warning when mixed trees are allowed. However, be aware of the warnings limit in sanitize_all

def sanitize_all(input_folder, output_folder, nprocesses=1):
'''Sanitize all morphologies in input_folder and its sub-directories.
See :func:`~neuror.sanitize.sanitize` for more information on the sanitization process.
Args:
input_folder (str|pathlib.Path): input neuron
output_folder (str|pathlib.Path): output name
.. note:: the sub-directory structure is maintained.
'''
set_maximum_warnings(0)

adrien-berchet
adrien-berchet previously approved these changes Mar 8, 2022
Copy link
Member

@adrien-berchet adrien-berchet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to test my permissions, not a real approval :)

@adrien-berchet adrien-berchet dismissed their stale review March 8, 2022 10:27

Permission test ok

@arnaudon
Copy link
Contributor

solved here in mcar: BlueBrain/morphology-workflows#62

@arnaudon arnaudon closed this Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants