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
Copy input image header to the output by default #4397
base: main
Are you sure you want to change the base?
Conversation
👋 @man-shu Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4397 +/- ##
==========================================
+ Coverage 91.85% 91.95% +0.09%
==========================================
Files 144 143 -1
Lines 16419 16639 +220
Branches 3434 3529 +95
==========================================
+ Hits 15082 15300 +218
+ Misses 792 773 -19
- Partials 545 566 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The fix was indeed the one proposed in #4393. So now the default behavior for all functions under As for |
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.
The change LGTM but you must at least make it visible.
I'm wondering whether we should go through deprecation cycles...
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.
- See comment: https://github.com/nilearn/nilearn/pull/4397/files#r1579266563
- Should be mentioned in the changelog
yeah I am wondering about this as well, because some of this may induce behavior changes in the code. unless we decide that the old behavior was a bug and that this is a bug fix |
Personally I would be +1 deprecation cycles ! This is a noticeable enough change (and I don't know that the before was wrong ) that I think we should alert users. |
Good with me to err on the side of caution and let our users know. |
Yes, I also agree with going through the deprecation cycles and letting people know. But my feeling is that it should not affect existing internal nilearn routines because, as far as I know, we don't ever directly use the header information in any of the workflows. Some bids-dependent functionalities need TR info, but that is extracted from JSON sidecars. However, some external software probably does use the headers. |
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.
LGTM, thx.
image
module do not carry over the header information to the output #4393The following functions under
nilearn.image
have been updated to copy image header by defaultcrop_img
mean_img
threshold_img
binarize_img
resample_img
reorder_img
remove the check for number of dimensions inmath_img
whencopy_header_from
is not None, ifnew_img_like
can handle it (explained in [BUG] Several functions under theimage
module do not carry over the header information to the output #4393)