[FIX] Headers not carried over by math_img
#4337
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. |
|
Maybe also include resampling functions in this PR ? |
|
I ran into a use case where one could remove the time dimension in the expression to from nilearn import image
from pathlib import Path
import numpy as np
from nibabel import Nifti1Image
from nilearn._utils.data_gen import generate_fake_fmri
def generate_fake_fmri_with_tr(tr=1.0):
# Generate a fake fmri image
img, mask = generate_fake_fmri()
# Change the TR of the image
header = img.header.copy()
header["pixdim"][4] = tr
img = Nifti1Image(img.get_fdata(), img.affine, header)
return img, mask
img1, mask = generate_fake_fmri_with_tr(2)
img2, mask = generate_fake_fmri_with_tr()
# check the TRs of the two images
print(img1.header.get_zooms()[3])
print(img2.header.get_zooms()[3])
# mean over time dimension and subtract the two images
result = image.math_img(
"np.mean(img1, axis=-1) - np.mean(img2, axis=-1)", img1=img1, img2=img2
)
# will return a ValueError that TRs are different (if you switch to this branch)In this case, it does not make sense to throw an error if Any ideas on how we could separate out such events? @bthirion @Remi-Gau |
|
Seems like a job for |
|
Oops. It looks very hard to me to handle properly all cases To me his would justify not to copy the whole header by default... |
|
I agree. The solution I am trying to implement uses:
Will try to push it soon. But this might be difficult to maintain in long-term, given the complexity of regex. Same issues would apply to Speaking of resampling only handling spatial resampling we should probably clarify that in the docs. |
|
Maybe just add an argument to copy the header information, that would be False by default, that people could use at their own risk. Demoing the feature would be relevant. |
|
I think it would also be useful to allow the users to select which |
|
So now the user can specify which image to copy the header from. As long as this image and the result image have same dimensions, it will copy the header: from nilearn import image
from nibabel import Nifti1Image
from nilearn._utils.data_gen import generate_fake_fmri
def generate_fake_fmri_with_tr(tr=1.0):
# Generate a fake fmri image
img, mask = generate_fake_fmri()
# Change the TR of the image
header = img.header.copy()
header["pixdim"][4] = tr
img = Nifti1Image(img.get_fdata(), img.affine, header)
return img, mask
img1, mask = generate_fake_fmri_with_tr(2)
img2, mask = generate_fake_fmri_with_tr()
# check the TRs of the two images
print(img1.header.get_zooms())
print(img2.header.get_zooms())
# try adding the two images
img_sum = image.math_img(
"np.mean(img1, axis=-1) - np.mean(img2, axis=-1)",
img1=img1,
img2=img2,
copy_header_from="img1",
)
# will throw Value error: The result of the formula has a different number of dimensions than the image to copy the header from.
# user could instead mean the images before subtraction
img1_mean = image.mean_img(img1)
img2_mean = image.mean_img(img2)
img_sum = image.math_img(
"img1 - img2", img1=img1_mean, img2=img2_mean, copy_header_from="img1"
)
# this will work |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4337 +/- ##
==========================================
+ Coverage 91.85% 92.15% +0.30%
==========================================
Files 144 143 -1
Lines 16419 16501 +82
Branches 3434 3465 +31
==========================================
+ Hits 15082 15207 +125
+ Misses 792 749 -43
Partials 545 545
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I think this PR is big enough already. I will make a separate one to handle |
|
Still at a conference. Don't have time for an in depth review just yet, but you may want to check why a test is failing with the oldest dependency. |
|
Yeah, it's a failure in doctest. I am downloading a dataset in the short example right under the reference to |
|
Ok, so @Remi-Gau and I found that the doctest is failing because I am trying to fetch a dataset in the docstring example, and that is currently not possible in the testing runs on the CI. So writing a full example to demo the I looked into other full examples on So, I am now writing a separate standalone example for this. |
maybe we can have this in a separate PR, so we can already merge the fix? |
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Good to know. I wasn't doing that in previous PRs. Done now! |
| @@ -49,3 +49,5 @@ Changes | |||
|
|
|||
| - :bdg-dark:`Code` Use red to blue color map in the GLM reports (:gh:`4266` by `Rémi Gau`_). | |||
| - :bdg-dark:`Code` :class:`nilearn.maskers.NiftiSpheresMasker` will throw warnings if the ``labels`` passed to it is not a :obj:`list` of :obj:`str`, or if the number of items in the list of labels does not match the number of regions in the label image (:gh:`4274` by `Rémi Gau`_). | |||
|
|
|||
| - :bdg-dark:`Code` Copy headers from user-specified image to the result of :func:`nilearn.image.math_img` (:gh:`4337` by `Himanshu Aggarwal`_). | |||
There was a problem hiding this comment.
is it just a change or actually a fix?
There was a problem hiding this comment.
I was a bit confused about that, too.
But here's my thinking behind tagging it as a change:
The default behavior is still what was initially reported. We are just giving the user an option to copy the header if they want.
There was a problem hiding this comment.
OK makes sense: I got confused because it was closing an issue filed as bug, so I would have seen this as a fix.
that's fine then.
|
@bthirion had already approved so I will got ahead and merge this. |
Changes proposed in this pull request:
[x] copy last image's header in the result image (i.e. if the call ismath_img("img1+img2", img1, img2), the result would have img2's header)[x] check if the images have the same TRs, if not throw the "Input images cannot be compared..." error[ ] test the exceptions and equality of TRscopy_header_fromis passedcopy_header_fromcopy_header_fromisNonecal_maxandcal_minfields in the header, but the rest remains the same.