-
Notifications
You must be signed in to change notification settings - Fork 59
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
Alter tractogram (trim, cut, replace, subsample, transform) #992
base: master
Are you sure you want to change the base?
Conversation
Hello @frheault, Thank you for updating !
Comment last updated at 2024-06-18 14:03:36 UTC |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #992 +/- ##
==========================================
- Coverage 68.14% 67.63% -0.51%
==========================================
Files 419 420 +1
Lines 21369 21662 +293
Branches 3216 3243 +27
==========================================
+ Hits 14561 14651 +90
- Misses 5533 5746 +213
+ Partials 1275 1265 -10
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Round 1 of comments. I didn't really read the code yet, only the descriptions.
Pep8 also needs to be addressed.
input tractogram. | ||
""" | ||
# Import in function to avoid circular import error | ||
from scilpy.tractanalysis.reproducibility_measures import compute_dice_voxel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that our management is not perfect yet.
tractanalysis.reproducibility_measures.compute_dice_voxel
seems to be usable with any map. So it is not necessarily a tractanalysis method. Maybe it could go in image.volume_operations
(or in volume_math if we also want to offer this option in scil_volume_math). Or in stats.something but I don't like it as much.
#!/usr/bin/env python | ||
# -*- coding: utf-8 -*- | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would indeed rename. But not just bundle: the goal of this script is not simply to alter. I would imagine that a scil_tractogram_alter.py would ask user how to alter. Here it decides alone (if no option is provided). Can we put a longer, but more significative name? scil_bundle_alter_to_dice?
Also, I see that in your new methods (ex, subsample_streamlines_alter), you have an option baseline_sft=None
that is never used in your script. Can you add an option --reference_bundle
in your script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean by "Here it decides alone (if no option is provided)."
I agree that the name scil_bundle_alter.py does not mean much
Here are a few more suggestions:
- scil_bundle_alter_preserve_overlap.py
- scil_bundle_alter_optimize_overlap.py
- scil_bundle_alter_to_target_dice.py
(alter could be replaced by adjust, vary, adapt, modify, but I like alter because of the definition: change or cause to change in character or composition, typically in a comparatively small but significant way)
baseline_sft: It is used in another function (replace)
|
||
""" | ||
This script is used to alter a bundle to reach specific minimum dice | ||
coefficient from the original bundle. The script will subsample, trim, cut, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a little while to understand the goal of the script. Maybe something like this for the first sentence?
This script is used to alter a bundle while keeping it similar enough to the original version (keeping the dice coefficient above a given threshold).
This script is used to alter a bundle to reach specific minimum dice | ||
coefficient from the original bundle. The script will subsample, trim, cut, | ||
replace or transform the streamlines until the minimum dice is reached. | ||
(cannot be combined in one run, use the script multiple times if needed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not clear for me. What cannot be combined? Do you mean Cannot be used on multiple bundles in one run? Or the various operations choices cannot be combined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
various operations choices cannot be combined
replace or transform the streamlines until the minimum dice is reached. | ||
(cannot be combined in one run, use the script multiple times if needed) | ||
|
||
All operation use a dichotomic search to find the parameter that gets as close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operations.
""" | ||
Shuffle the orientation of the streamlines. The streamlines are not | ||
reversed, only the orientation of the points within the streamlines is | ||
shuffled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes its unclear, it is not a blanket reverse all streamlines, it is a shuffle head/tail of random streamlines
def replace_streamlines_alter(sft, min_dice=0.90, epsilon=0.01): | ||
""" | ||
Replace streamlines based on a dice similarity metric. | ||
The function will upsamples the streamlines (with parallel transport), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upsample
|
||
|
||
def trim_streamlines_alter(sft, min_dice=0.90, epsilon=0.01): | ||
# Import in function to avoid circular import error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docstring
This affect the whole bundle. | ||
|
||
The transform operation will apply random transformations to the streamlines | ||
until the minimum dice is reached. This affect the whole bundle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description of the method, you only mention rotations. Here, you say any transformation. Can you be more precise?
#!/usr/bin/env python3 | ||
# -*- coding: utf-8 -*- | ||
|
||
def test_help_option(script_runner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More tests required.
Quick description
New script to simulate alteration of tractogram (should be used for bundle, so maybe I will rename)
The operation are describe in the docstring of the script:
...
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist