Copy input image header to the output by default #4397
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.99% +0.13%
==========================================
Files 144 145 +1
Lines 16419 16709 +290
Branches 3434 3551 +117
==========================================
+ Hits 15082 15371 +289
+ Misses 792 767 -25
- Partials 545 571 +26
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.
- 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. |
|
Re: Setting There are quite a few tests that throw this warning: Will update those now. |
| assert result.header["cal_max"] == 1 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
@Remi-Gau, here I test the warning being thrown for copy_header=False (for all image.py functions)
There was a problem hiding this comment.
Just so you dont have to review everything again
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
and here, the test for warning for resampling.py functions
|
Everything looks green. You can merge at will @man-shu |
imagemodule do not carry over the header information to the output #4393 and Add future warning about copying header by default #4426The following functions under
nilearn.imagehave been updated to copy image header by defaultcrop_imgmean_imgthreshold_imgbinarize_imgresample_imgreorder_imgremove the check for number of dimensions inmath_imgwhencopy_header_fromis not None, ifnew_img_likecan handle it (explained in [BUG] Several functions under theimagemodule do not carry over the header information to the output #4393)