[FIX] Replace FSL `mcflirt` with AFNI `3dVolReg` #1354

oesteban · 2018-10-27T02:24:51Z

Closes #1535.

A second attempt to get #1283 finally merged in.

oesteban · 2018-10-27T02:38:24Z

Testing this here https://github.com/oesteban/fmriprep/tree/tests/fix-1354 to avoid excessive Circle usage.

However, I'm not positive the removal of rotation and shear does not have any additional side effects we haven't envisaged.

effigies · 2018-10-27T11:52:21Z

fmriprep/interfaces/images.py

+            if self.inputs.remove_rotation_and_shear:
+                out_fname = fname_presuffix(self.inputs.in_file, suffix='_valid',
+                                            newpath=runtime.cwd)
+                img = remove_rotation_and_shear(img)


Could it be that we're not correcting to closest canonical before removing rotation and shear in this interface?

https://circleci.com/workflow-run/a814ff84-9ddd-4d8b-9961-81348b8fa202

You were right on that we were not correcting to closest canonical, but that does not solve the problem (https://circleci.com/gh/oesteban/fmriprep/2636#artifacts/containers/0).

Question is: it seems to be working with the pipeline. Should we just disable the remove_rotation_and_shear for the tests only? Or we want to update the masks to include that?

So I think if we're removing rotation and shear, we need to be updating to RAS. Perhaps it makes sense to actually put that step directly into remove_rotation_and_shear, to prevent the case where we destroy orientation information.

If the output masks are valid, just in a different space, then we should update the targets.

oesteban · 2018-10-29T15:36:09Z

Status report:

Smoke tests on ds005, ds054 and ds210 look good
Regression tests of masks don't work unless the remove_rotation_and_shear is disabled. I see two options for these tests to pass: keep remove_rotation_and_shear off, or update both test datasets and masks with the orientation fixed.

There seem to be no side effects to the remove_rotation_and_shear in the workflow. I think we can go ahead and merge this (currently, with remove_rotation_and_shear disabled in the regression tests).

Opinions? @chrisfilo @effigies

effigies · 2018-10-29T16:03:17Z

fmriprep/utils/misc.py

+    import numpy as np
+    import nibabel as nb
+
+    img = nb.as_closest_canonical(img)


Oh, I see you did put this in here. Ignore the above.

effigies · 2018-10-29T16:08:23Z

fmriprep/workflows/bold/hmc.py


 DEFAULT_MEMORY_MIN_GB = 0.01


 # pylint: disable=R0914
-def init_bold_hmc_wf(mem_gb, omp_nthreads, name='bold_hmc_wf'):
+def init_bold_hmc_wf(use_mcflirt, mem_gb, omp_nthreads, name='bold_hmc_wf'):


If we eventually want to be able to swap in any MC algorithm, use_mcflirt is not a great name for the parameter. What about motion_correction that takes a string such as '3dvolreg' or 'mcflirt' (and I would accept 'afni' and 'fsl' as synonyms, at least at the command line).

effigies · 2018-10-29T16:13:21Z

fmriprep/workflows/bold/tests/test_util.py

@@ -58,7 +58,8 @@ def symmetric_overlap(img1, img2):
    )
 ])
 def test_masking(input_fname, expected_fname):
-    bold_reference_wf = init_bold_reference_wf(omp_nthreads=1)
+    bold_reference_wf = init_bold_reference_wf(
+        omp_nthreads=1, remove_rotation_and_shear=False)


Since you're not setting this to False in the actual pipeline, I would not set it False here. It's not obvious that a change to the algorithm that causes a 2% shift without this step will cause the same 2% shift with the step.

On the other hand, this test was placed because we wanted to have changes on the masking algorithm under control. Adding the reorientation just embeds complexity into the masking process but should not affect masking performance. I see that as an integration problem, and I'm not sure we want to upload test data and masks after reorientation to replace the current test data.

What if we decide to fall back into mcflirt? Then this test would require rolling data back again.

@chrisfilo where does the need from removing rotation and shear come from?

AFNI ignores those when calculating transformations. If you keep them in the reference and use ANTs to apply the transformations estimated using AFNI the results will be misaligned.

That makes sense. Do you think it would be possible to include rotation and shear here in the Volreg2ITK conversion instead?

That way we would avoid changing the image's original header.

Potentially, but that would be harder to implement.

What if we decide to fall back into mcflirt? Then this test would require rolling data back again.

As long as we always apply this cleaning to the entire BOLD series and , MCFLIRT will also be working from the cardinal RAS+ orientation. This would actually end up finishing up #260.

effigies · 2018-10-30T14:49:10Z

Should re-raise the following point from #260 (comment):

If we change the orientation of input files we need to adjust the phase encoding direction as well.

I would also add slice-encoding direction (#1345).

I think we need a quick interface (here as a function) to translate incoming directions to reoriented directions:

def update_encoding_axes(img, pe_dir, slice_dir):
    import nibabel as nb

    ornt = nb.orientations.io_orientation(img)

    axes = ornt[:, 0]
    flips = ornt[:, 1] # -1 if flip, 1 if no flip

    if pe_dir is not None:
        pe_idx = 'ijk'.index(pe_dir[0])
        pe_sign = -1 if pe_dir[1:] else 1
        pe_sign *= flips[pe_idx]
        pe_idx = axes[pe_idx]
        pe_dir = 'ijk'[pe_idx] + '-'[pe_sign:]
        
    if slice_dir is not None:
        ...

    return pe_dir, slice_dir

I can flesh out the interface and write tests, if you agree this is a good idea.

oesteban · 2018-10-30T16:02:08Z

Isn't safer/easier to calculate the analytic inverse of remove_rotation_and_shear and apply it when building the ITK transforms?

effigies · 2018-10-30T16:26:32Z

It's all passing around the correct metadata, so either way is fine with me. I've found applying transforms in the right order has been a bit of a pain, in the past. And transforms that involve adjusting the affine but not reslicing the data matrix are very finicky (see, e.g., #842), so I'm not sure how simple re-rotating-and-shearing will end up being.

oesteban · 2018-10-30T19:40:09Z

I agree that adjusting the affine without reslicing is really finicky.

Another concern to take into account. Merging #1334 (or any derivative of it) will make it very easy to implement an orig keyword to generate BOLD series in the original space. However, these outputs will not match the original orientation somewhat defeating the purpose of the reference. Does it make sense?

This is less of a problem if we set a boldref as reference. Since that reference would be produced by fMRIPrep, it is not a problem that the shear and rotation have been removed.

effigies · 2018-10-31T17:31:51Z

I guess the question is whether we ever really care about the "original space" so much as "absolutely minimal resampling". I'm inclined to say it's actually the latter, in which case boldref is the better choice, and we should just be explicit about it, but perhaps there is a use-case for orig?

effigies · 2018-10-31T17:57:05Z

Is this okay to push off to the next release?

effigies

This is going to require merging master. And I think we have some open discussion points. Maybe after OHBM abstracts are in, we should sit down and make a clear plan forward for swappable motion correction.

oesteban · 2018-12-14T23:44:22Z

👍

dangom · 2020-01-09T00:41:18Z

Hi guys,

related to #1928, I would love to see this PR merged in order to start experimenting with fMRIPREP for the robust preprocessing of high-res/partial FOV data.

I noticed, and correct me if I'm wrong, that one of the obstacles towards getting this PR merged is thatAFNI messes up the nifti headers and therefore may cause potential issues in downstream resampling/registration steps - in particular when interacting with ANTs.

Interestingly enough, a solution to this problem seems to have already been implemented by Freesurfer. If we look at the code of freesurfer's 3dVolReg wrapper, mc-afni2, we see that they use mri_convert to fix the 3dVolReg output. Would it not simplify the implementation of this PR simply swapping mcflirt for mc-afni2 or porting freesurfer's solution by piping the output of 3dVolReg through a call to mri_convert as well?

# Always convert the output. AFNI mucks with the goemetry,
# so we have to run mri_convert with --like to keep it.
# 3dvolreg will remove off-diagonal components (but keeps
# the basic geometry correct).
echo "#@# --------------------------------" |& tee -a $LF
set cmd = (mri_convert $newoutvol $outvol --in_like $tempvol)
pwd |& tee -a $LF
echo $cmd |& tee -a $LF
$cmd |& tee -a $LF
if($status) then
  echo "ERROR: converting output " |& tee -a $LF
  if($cleanup) then
    echo "... cleaning up ...."; |& tee -a $LF
    rm -r $tmpdir
  endif
  exit 1;
endif

oesteban · 2020-01-09T01:43:07Z

We are addressing the issue in nipy/nitransforms#9 without tricky hacks - just interpreting the "deobliquing" transforms as 3dVolreg does. An extra pair of hands will be more than very much appreciated there :)

dangom · 2020-01-12T01:26:26Z

Thanks for your answer.

I can't be of much help at the moment in interpreting AFNI transforms, since I don't have experience with AFNI. That said, I've been comparing 3dVolReg (via mc-afni2) and antsMotionCorr for high-res /partial FOV motion correction. Both work very well, and up to minor interpolation differences, results are almost identical.

3dVolReg has the disadvantage that it doesn't accept reference images with a different matrix size / resolution. antsMotionCorr does the right thing out of the box, so replacing an sbref for a whole-brain ref would work without further tweaks. Motion-correcting directly towards a whole brain (or large FOV) reference makes intuitively more sense for cases of extreme partial FOV ( < 1 cm coverage), where concepts such as a "mean volume" start to break down ( under motion each time-point may cover different, slightly non-overlapping anatomy).
The downside is that whole-brain references are generally separate acquisitions, so motion parameter estimates would potentially have to take that into account.

antsMotionCorr has the disadvantage that it's not currently supported by Nipype. An orphaned old-PR to add support exists, though.

oesteban · 2020-01-12T06:43:36Z

I'd be 👍 with bringing back the idea of antsMotionCorr - that stub in nipype was created when we were first playing with it.

oesteban · 2020-06-05T23:11:58Z

This PR has gone stale and almost impossible to merge right now. Also, we will hopefully go around the oblique x-forms issue in NiTransforms proper, and this migration (to AFNI's tooling) will become pretty trivial.

It is important to say that #1535 stands as relevant as when Chris initially posted it if not more (given the unreliability of FD Power traces we get from mcflirt).

oesteban added 2 commits October 26, 2018 19:22

Revert "Revert #1283 enh/3dvolreg"

94fdc83

do not remove_rotation_and_shear generating masks

2868b7c

effigies reviewed Oct 27, 2018

View reviewed changes

effigies mentioned this pull request Oct 27, 2018

Omnibus multi-echo pull request #1296

Merged

3 tasks

oesteban added 2 commits October 27, 2018 14:46

make sure as_closest_canonical is called

2437e68

reset cache of ds210

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

120c5f8

effigies reviewed Oct 29, 2018

View reviewed changes

effigies requested changes Dec 13, 2018

View reviewed changes

oesteban mentioned this pull request Jul 5, 2019

Incorrect alignment of resampled results when SBRefs have mismatched xforms #1684

Open

oesteban force-pushed the master branch from 0a3fe45 to b3a4d24 Compare October 9, 2019 19:25

dangom mentioned this pull request Jan 8, 2020

fMRIPREP for high-res and partial FOV #1928

Open

5 tasks

oesteban mentioned this pull request Jan 13, 2020

Concatenating different runs nipreps/dmriprep#43

Closed

oesteban closed this Jun 5, 2020

mgxd mentioned this pull request Dec 8, 2020

ENH: Replace MCFLIRT with 3dVolReg nipreps/fmriprep-rodents#15

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Replace FSL `mcflirt` with AFNI `3dVolReg` #1354

[FIX] Replace FSL `mcflirt` with AFNI `3dVolReg` #1354

oesteban commented Oct 27, 2018 •

edited by chrisgorgo

Loading

oesteban commented Oct 27, 2018

effigies Oct 27, 2018

oesteban Oct 27, 2018

oesteban Oct 27, 2018

effigies Oct 29, 2018

oesteban commented Oct 29, 2018 •

edited

Loading

effigies Oct 29, 2018

effigies Oct 29, 2018

effigies Oct 29, 2018

oesteban Oct 30, 2018

oesteban Oct 30, 2018

chrisgorgo Oct 30, 2018

oesteban Oct 30, 2018 •

edited

Loading

chrisgorgo Oct 30, 2018

effigies Oct 30, 2018

effigies commented Oct 30, 2018

oesteban commented Oct 30, 2018

effigies commented Oct 30, 2018

oesteban commented Oct 30, 2018 •

edited

Loading

effigies commented Oct 31, 2018

effigies commented Oct 31, 2018

effigies left a comment

oesteban commented Dec 14, 2018

dangom commented Jan 9, 2020

oesteban commented Jan 9, 2020

dangom commented Jan 12, 2020

oesteban commented Jan 12, 2020

oesteban commented Jun 5, 2020

[FIX] Replace FSL mcflirt with AFNI 3dVolReg #1354

[FIX] Replace FSL mcflirt with AFNI 3dVolReg #1354

Conversation

oesteban commented Oct 27, 2018 • edited by chrisgorgo Loading

oesteban commented Oct 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban commented Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Oct 30, 2018

oesteban commented Oct 30, 2018

effigies commented Oct 30, 2018

oesteban commented Oct 30, 2018 • edited Loading

effigies commented Oct 31, 2018

effigies commented Oct 31, 2018

effigies left a comment

Choose a reason for hiding this comment

oesteban commented Dec 14, 2018

dangom commented Jan 9, 2020

oesteban commented Jan 9, 2020

dangom commented Jan 12, 2020

oesteban commented Jan 12, 2020

oesteban commented Jun 5, 2020

[FIX] Replace FSL `mcflirt` with AFNI `3dVolReg` #1354

[FIX] Replace FSL `mcflirt` with AFNI `3dVolReg` #1354

oesteban commented Oct 27, 2018 •

edited by chrisgorgo

Loading

oesteban commented Oct 29, 2018 •

edited

Loading

oesteban Oct 30, 2018 •

edited

Loading

oesteban commented Oct 30, 2018 •

edited

Loading