Skip to content
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

Potential "double-unwarping" bug in bold_to_t1w_transform when running fmriprep on multi-echo data #2727

Closed
markushs opened this issue Mar 1, 2022 · 9 comments
Labels
Milestone

Comments

@markushs
Copy link
Contributor

markushs commented Mar 1, 2022

What happened?

antsApplyTransforms run under bold_t1_trans_wf/bold_to_t1w_transform seems to include SDC warp field under --transforms when unwarping has already been performed.

What command did you use?

Not relevant, see info below.

What version of fMRIPrep are you running?

21.0.1

How are you running fMRIPrep?

Singularity

Is your data BIDS valid?

Yes

Are you reusing any previously computed results?

No

Please copy and paste any relevant log output.

Not relevant

Additional information / screenshots

Continuation of neurostars post https://neurostars.org/t/converting-between-scanner-and-t1w-space-using-supplied-affine-transforms/21719

Might be wrong here, but looks like there's a bug.

Briefly, in fmriprep, bold_ts2map_wf/t2smap_node is run on unwarped volumes (i.e. after susceptibility distortion correction). This workflow produces the "optimally combined" volume.

Later on, bold_t1_trans_wf/bold_to_t1w_transform is run on the output of bold_t2smap_wf(via split_opt_comb). That is, on already unwarped data. However, the topup estimated warp field is also here included as input under --transforms in the ants registration step: (together with the bbreg-estimated bold-to-T1w affine transformation). In other words, the functional space-T1w-data seems to have been unwarped twice.

In func_derivatives_wf/ds_ref_t1w_xfm, only the bbreg-affine is saved, however. That is, the *_from-scanner_to-T1w_mode-image_xfm.txt-transform only reflects the bbreg-calculations.

Rerunning the antsApplyTransforms-command from the example shown over at Neurostars, adding the warp field as shown here:

image

... produces an identical volume as the optimally combined T1w-space data produced within the fMRIprep-workflow (i.e. *_space-T1w_desc-preproc_bold.nii.gz ). But again, I think this volume has gone through unwarping twice.

Again, if I'm missing something here I'm awfully sorry. But pinging @tsalo as this issue affects the current tedana-PR (ME-ICA/tedana#847). The suggested approach in that PR seems correct, but the output will probably not be comparable to fMRIprep's double-unwarped output.

@markushs markushs added the bug label Mar 1, 2022
@markushs
Copy link
Contributor Author

markushs commented Mar 1, 2022

My best guess on how to fix this is this section (that can be deleted?)

(join_sdc_echos, bold_t1_trans_wf, [
# TEMPORARY: For the moment we can't use frame-wise fieldmaps
(("fieldwarp", _dpop), "inputnode.fieldwarp"),
]),

Given what happens at

if not multiecho:

... line 1166-1169 seem to cancel out the wanted behavior

bold_t1_trans_wf.inputs.inputnode.fieldwarp = "identity"

@effigies
Copy link
Member

effigies commented Mar 3, 2022

Yes, I think you're right. For multiecho data, fieldwarp should be 'identity' for all resampling workflows. so we want to remove both:

(join_sdc_echos, bold_t1_trans_wf, [
# TEMPORARY: For the moment we can't use frame-wise fieldmaps
(("fieldwarp", _dpop), "inputnode.fieldwarp"),
]),
(join_sdc_echos, bold_std_trans_wf, [
# TEMPORARY: For the moment we can't use frame-wise fieldmaps
(("fieldwarp", _dpop), "inputnode.fieldwarp"),
]),

@effigies
Copy link
Member

effigies commented Mar 3, 2022

Would you mind submitting a PR against maint/21.0.x?

@markushs
Copy link
Contributor Author

markushs commented Mar 3, 2022

@effigies Just did, but looks like my PR was based on master (submitted against maint/21.0.x though, so at least I got something right!)

@effigies
Copy link
Member

effigies commented Mar 3, 2022

To rebase onto the maint/21.0.x branch, do the following:

# Add upstream remote if missing
git remote add upstream https://github.com/nipreps/fmriprep.git
# Fetch upstream
git fetch upstream
# Rebase
git rebase -i upstream/maint/21.0.x

Here you should get a text editor with something like something like:

pick 508fe477 MNT: Ask for fmriprep-docker RUNNING line
pick 97f60efb fix two minor typos in report spec
pick c8256ee0 DOC: clarify calculation of confounding signals
pick dda76787 remove trailing whitespace
pick 6aca8d8d ENH: Add major/minor versions to base workflow name
pick b70345ec Update base.py

Just delete all but the last line:

pick b70345ec Update base.py

Save and exit. Then:

git push -f

@markushs
Copy link
Contributor Author

markushs commented Mar 3, 2022

Thanks @effigies! Should be fixed now.

@effigies
Copy link
Member

effigies commented Mar 4, 2022

Fixed in #2730.

@effigies effigies closed this as completed Mar 4, 2022
@effigies effigies added this to the 21.0.2 milestone Mar 4, 2022
@tsalo
Copy link
Collaborator

tsalo commented Mar 4, 2022

Just to make sure I understand (and perhaps a bit for posterity), the problem is with the T1w-space and standard-space output maps, but not with the reference-space echo-wise output maps or with the reference-to-T1w or T1w-to-standard transforms, right? So, for users of 21.0.0 who enabled --me-output-echos, they could produce correctly warped data by combining the echos with tedana and then applying the available transforms to the combined time series?

@effigies
Copy link
Member

effigies commented Mar 4, 2022

Just to make sure I understand (and perhaps a bit for posterity), the problem is with the T1w-space and standard-space output maps, but not with the reference-space echo-wise output maps or with the reference-to-T1w or T1w-to-standard transforms, right?

Correct.

So, for users of 21.0.0 who enabled --me-output-echos, they could produce correctly warped data by combining the echos with tedana and then applying the available transforms to the combined time series?

If by "available transforms" you mean BOLD-T1w and T1w-template, then yes. SDC warps are already applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants