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

FIX: Do not reorient magnitude images #98

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

effigies
Copy link
Member

In #33 it is shown that fieldmaps in AIL orientation produce an error in sdc_estimate_wf.phdiff_wf.fmap_postproc_wf.cleanup_wf.Despike where the brainmask generated in sdc_estimate_wf.phdiff_wf.magnitude_wf.bet is in a different orientation.

This is because the input to BET is reoriented to RAS in the magmrg step. I considered forcing all images to RAS, but that has the potential to cause problems with PhaseEncodingDirection, which is one of the voxel dimensions i, j, and k, and not coded in RAS space.

The simpler solution is simply not to reorient images during this workflow, to ensure that calculations are performed as intended. A further validation step could ensure that all fieldmap images have the same orientation and voxel sizes, but that would be a more significant change.

Because this is a change to an interface input, workflow directories can be reused without clearing. This can be safely included in the 1.5.x series of fMRIPrep.

Fixes #33.

@pull-assistant
Copy link

pull-assistant bot commented Apr 15, 2020

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     FIX: Do not reorient magnitude images

     DOC: Require Sphinx <3

Powered by Pull Assistant. Last update fc4009b ... c15234e. Read the comment docs.

@effigies effigies requested a review from oesteban April 15, 2020 19:31
@effigies
Copy link
Member Author

@oesteban Because this is so small, I'm going to put on my release manager hat and merge to start the process to get fMRIPrep 1.5.10 out the door. Please interrupt if you have concerns.

@effigies effigies merged commit ec937c6 into nipreps:maint/1.0.x Apr 15, 2020
@effigies effigies deleted the fix/no_reorient_magnitude branch April 15, 2020 19:56
effigies added a commit that referenced this pull request Apr 16, 2020
1.0.6 (April 15, 2020)

Bug-fix release.

* FIX: Do not reorient magnitude images (#98)
effigies added a commit to nipreps/fmriprep that referenced this pull request Apr 16, 2020
1.5.10 (April 16, 2020)

Bug-fix release in the 1.5.x series.

This release fixes a bug for **phase-difference fieldmaps that are not in RAS+ orientation**.
The bug presented as an error if the orientation was reordered relative to RAS+ (for example,
AIL+) and the swapped dimensions were not of the same size.
Otherwise, the bug introduced a poor masking of the phase difference map, and could be quite subtle
if the original orientation was LAS+.
Runs of fMRIPrep that used other susceptibility distortion correction (SDC) methods are not
currently considered problematic.

This bug affects all previous versions of fMRIPrep, as well as versions 20.0.0-20.0.5.

* FIX: Do not reorient magnitude images (nipreps/sdcflows#98)
effigies added a commit to nipreps/fmriprep that referenced this pull request Apr 16, 2020
20.0.6 (April 16, 2020)

Bug-fix release in the 20.0.x series.

This release fixes a bug for **phase-difference fieldmaps that are not in RAS+ orientation**.
The bug presented as an error if the orientation was reordered relative to RAS+ (for example,
AIL+) and the swapped dimensions were not of the same size.
Otherwise, the bug introduced a poor masking of the phase difference map, and could be quite subtle
if the original orientation was LAS+.
Runs of fMRIPrep that used other susceptibility distortion correction (SDC) methods are not
currently considered problematic.

This bug affects all earlier versions of fMRIPrep, except for 1.5.10 and any future releases in
the 1.5.x series.

* FIX: Do not reorient magnitude images nipreps/sdcflows#98
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant