-
Notifications
You must be signed in to change notification settings - Fork 301
[RTM] fixing header issues in HMC #504
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
Conversation
chrisgorgo
commented
May 3, 2017
- Fix header issue caused by bias field correction. Closes Output EPI voxel size slightly different from input #497
- Use reference image with bias field instead of after correction when doing motion correction. The reasoning is that we should match the bias field between the reference image and the 4D bold volume (which is not corrected for bias).
Waiting for @feilong to confirm this fix works. |
fmriprep/workflows/epi.py
Outdated
(gen_ref, cphdr, [('ref_image', 'hdr_file')]), | ||
(gen_ref, hmc, [('ref_image', 'ref_file')]), | ||
(cphdr, skullstrip_epi, [('out_file', 'in_file')]), | ||
(cphdr, outputnode, [('out_file', 'ref_image')]), |
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'm trying to understand this a bit better. cphdr.out_file
is just the bias-field corrected image with the original header. We're sure that this is safe -- i.e. that a different header on inu.output_image
doesn't reflect a change in the data?
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.
That is the assumption. the problem is that N4BiasFieldCorrection
strips/modies the affines in the header.
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.
Really? Do they give a reason, or is this a bug we should report? And is this the primary use for the CopyHeader
interface? If so, maybe we should make a reusable workflow that is simply N4BiasFieldCorrection
+ CopyHeader
so that it's obvious why we're doing this.
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.
We should report it and yes - we use it enough to create a wee workflow for this.
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.
Looks reasonable, assuming it resolves @feilong's problem. You should also update the niworkflows pin in docs/environment.yml
.
@@ -27,12 +27,13 @@ | |||
|
|||
from fmriprep.interfaces import DerivativesDataSink | |||
|
|||
from fmriprep.interfaces.images import GenerateSamplingReference | |||
from fmriprep.interfaces.images import GenerateSamplingReference, CopyHeader |
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.
You can drop the CopyHeader
import.
Commit 51bab77 works well on my data. |
That's great news - just to confirm did your test include upgrading niworkflows (which is part of this PR)? |
I got some problems when I first tried to use the latest commit, so I upgraded niworkflows, nipype, and pybids to versions shown in |
Awesome! Thank you so much for testing this. I feel much better knowing we have resolved this issue! |
You are most welcome! Thanks for fixing this issue! |