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

Module tractogram operations #658

Merged
merged 13 commits into from
Mar 8, 2023

Conversation

EmmaRenauld
Copy link
Contributor

Continuing the refactoring work, as discussed in group.

Explanation:

Based on our analysis of the scripts, we will create a new section scilpy.tractograms, which should contain:

  • streamline_operations.py --> operations on the points of the streamlines. Ex: resampling, smoothing, etc.
  • tractogram_operations.py (CURRENT PULL REQUEST!) --> operations on the streamlines as parts of the tractogram. Ex: upsampling the number of streamlines, splitting into two sub-stractograms, registration, concatenation, union of tractograms, etc.
  • segment/filtering --> segmentation of the tractogram based on user-defined criteria (ex, based on anatomy) or recobundles.
  • I am also adding: lazy_tractogram_operations, which are a little different as they include loading in the methods, which we usually keep in the mains.

This PR:

Creation of tractogram_operations.py and lazy_tractogram_operations.

Fonctions that were copy-pasted with no change:

  • most of what was in utils.streamlines.
  • io.streamlines.lazy_streamlines_count
  • split methods from tracking.tools
  • utils.transformation (flip_sft)

New methods created to clean mains in scripts:

  • shuffle_streamlines()
  • lazy_concatenate()

Other change:

  • Renamed scil_streamlines_math.py to scil_tractograms_math.py to reflect our new category understanding.

@frheault , @AntoineTheb , @AlexVCaron

@EmmaRenauld
Copy link
Contributor Author

On our toDo list for this PR:
there was to verify if there is still a reason for scil_remove_similar_streamlines compared to scil_resample_tractogram. I did not touch it for now. @frheault

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@frheault
Copy link
Member

I tested a few scripts and it still works (I think, I need to do it again with better data).

Do you think its possible to leave the old script that would print a warning (deprecated script) and then just call the new one (or may just point the user to the new one).

I am afraid changing scil_streamlines_math.py to scil_tractogram_math.py will generate a ton of email like 'WHERE IS THAT SCRIPT' for the next year.
A explicit warning for the next release at least could help the transition.

PS: Yes, just delete the scil_remove_similar_streamlines.py, and delete functions that are unique to that script (or not reusable) It is an over- engineered script that no one use. I will keep a copy in a personal graveyard.

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@frheault
Copy link
Member

frheault commented Mar 8, 2023

I think it would be important to leave a warning in scil_streamlines_math.py until the next release, but make it so it still works for a few months as a transition.

The easiest way would be that scil_tractogram_math.py has a main() with the argparser build, but then the whole content of the main is moved to another function that accepts args and parser as arguments.

Then scil_streamlines_math could display the warning, but then call the sub-function from the other script with its own parser and args.

This way, without duplication of code, the user can still use it and get a warning without all the flows and scripts crashing. This will be removed a few months later, so a complete deprecation cycle).

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore arnaudbore merged commit afc4fae into scilus:master Mar 8, 2023
@EmmaRenauld EmmaRenauld deleted the module_tractogram_operations branch April 6, 2023 14:09
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 this pull request may close these issues.

3 participants