-
Notifications
You must be signed in to change notification settings - Fork 75
Transfert functions for reorient #1295
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
base: dev_3.0.x
Are you sure you want to change the base?
Conversation
Add pathspec dependency to project requirements.
Update paper figures with better resolution
Add maximum operation to scil_volume_math
…_tests Small changes from my tests
|
This is now aim to merge into master. Introducing the SFI : StatefulImage I highly suggest someone tries it with my example. I will modify a single script (scil_volume_crop maybe?) later. |
[ENH] Fix test - uv issue
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1295 +/- ##
==========================================
+ Coverage 72.51% 72.60% +0.08%
==========================================
Files 295 297 +2
Lines 25442 25522 +80
Branches 3565 3577 +12
==========================================
+ Hits 18449 18529 +80
+ Misses 5488 5487 -1
- Partials 1505 1506 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Seems great! I did not test, but it would be easier with a script. Maybe we can merge and test more when the first script is created?
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.
Pull request overview
This PR introduces new functionality for handling reorientations of nibabel images with state management to preserve original orientation information. The implementation allows loading images in a specific orientation (e.g., RAS) for processing while maintaining the ability to save them back in their original orientation.
Changes:
- Added
StatefulImageclass that extendsnib.Nifti1Imageto track original and current orientations - Added
validate_axcodesutility function to validate orientation axis codes - Added comprehensive test suites for both new modules
- Minor code style improvements (encoding parameters, quote consistency, whitespace)
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scilpy/io/stateful_image.py | New class for managing image orientation state with load/save/reorient methods |
| src/scilpy/utils/orientation.py | New utility function for validating axis codes (L/R, A/P, S/I) |
| src/scilpy/io/tests/test_stateful_image.py | Comprehensive test suite for StatefulImage class functionality |
| src/scilpy/utils/tests/test_orientation.py | Test suite for validate_axcodes function |
| src/scilpy/utils/scilpy_bot.py | Added explicit UTF-8 encoding to subprocess and file operations |
| src/scilpy/utils/init.py | Changed quote style from single to double quotes for consistency |
| src/scilpy/io/image.py | Removed extra blank line for code cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
CHrlS98
left a comment
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 like it. Here is my first round of comments. An example will help understanding the code. Also, are the peaks extracted using scilpy displayed correctly inside mi-brain even when the stride is different from [1,2,3,4] ? If so will the new method break the interoperability with mi-brain?
|
@CHrlS98 @EmmaRenauld @arnaudbore This can be tested with various stride (flip axis LPS/RAS, but also swap axis PSL, SAR) for the scripts (scil_volume_reshape, scil_volume_crop, scil_volume_resample, scil_volume_reslice_to_reference) Try random transformations in different axis and compared to the same command on master, and compare results to see which is intuitive/right. I think it is important to try really random operation that we would never try (change stride to -2,1,3, resample to 2mmx0.5mmx1mm and see if this results in what we want on master vs this branch. Use the link above for a ZIP, try the same operation on a image with a crazy stride and a wmparc with a normal stride and check if they end up correct. If you 3 could try something weird (without coordination) and describe what you did, what it gave, what it should have been, I think we will make this work ! |
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.
Here is a first test I did: Using a T1 with stride -1, 2, 3 in mrinfo, appears as LAS with scil_header_print_info. Flipping in y:
- scil_volume_flip: flipped in mi-brain, and mrinfo shows the same info as the original T1. Still appears as LAS.
- mrtransform -flip: flipped in mi-brain, but mrinfo a new stride, -1, -2, 3. Appears as LPS in our header_print. The affine as an extra -1 in y.
It that against our goal? We don't seem to have the same definition of "flipping"
Will make other tests.
src/scilpy/cli/scil_volume_crop.py
Outdated
| assert_outputs_exist(parser, args, args.out_image, args.output_bbox) | ||
|
|
||
| img = nib.load(args.in_image) | ||
| simg = StatefulImage.load(args.in_image, to_orientation='RAS') |
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.
to_orientation='RAS' is your default value, so I would not add it (here and in other calls below)
|
|
||
| def resample_volume(img, ref_img=None, volume_shape=None, iso_min=False, | ||
| voxel_res=None, | ||
| def resample_volume(simg: StatefulImage, ref_img: nib.Nifti1Image = None, |
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.
When I wanted to add type hints with this format, Arnaud told me that it was not our style in scilpy, we only put it in the docstring below. Are we reviewing our preferences? I personnally love it (we do it a lot in dwi_ml) but it's true that we don't use it elsewhere.
Ahh excellent question, do not use scil_volume_flip for this testing, the scilpy script flip the data and leave the header identical (flip the numpy array). I did not explore the scil_volume_flip script yet, this will be another round of changes. |
|
@arnaudbore I changed the base so it points to dev3.0.x, could you update dev3.0.x with master to avoid conflict / large diff? |


Quick description
New functions to handle reorientations of nibabel images to get Numpy array where L is L, R is R, etc.
...
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist