[ENH] Replace FSL `mcflirt` with AFNI `3dVolReg` #1283

chrisgorgo · 2018-09-13T21:10:12Z

Consistent with MRIQC
Faster than MCFLIRT
More accurate than MCFLIRT (https://www.ncbi.nlm.nih.gov/pubmed/16099178)

effigies

More accurate than MCFLIRT (https://www.ncbi.nlm.nih.gov/pubmed/16099178)

Is a 2005 comparison paper still a valid criterion for making such a move?

effigies · 2018-09-14T13:30:15Z

fmriprep/interfaces/images.py

+            import nibabel as nb
+            import numpy as np
+            affine = img.affine
+            affine[:3, :3] = np.diag(np.diag(affine[:3, :3]))


This will simply zero out the off-diagonal, right? That will end up producing an affine with smaller zooms than the original. Probably the correct way to do this is to do a rotation/zoom/shear/translation decomposition, and then to remove the rotations and shears.

That said, this is less a de-oblique than a removal of information that describes obliqueness. Is this what you want, and what's the reason for that?

Yes the intention was to remove off diagonal values (because otherwise AFNI doesn't generate transforms that can be applied by ANTs). Is there a python code snippet for rotation/zoom/shear/translation decomposition I could use?

effigies · 2018-09-14T13:46:20Z

fmriprep/interfaces/images.py

+            import numpy as np
+            affine = img.affine
+            affine[:3, :3] = np.diag(np.diag(affine[:3, :3]))
+            return nb.Nifti1Image(np.asanyarray(img.dataobj), affine, img.header)


I would prefer img.__class__ to nb.Nifti1Image.

effigies · 2018-09-14T13:49:56Z

fmriprep/workflows/bold/hmc.py

-    mcflirt = pe.Node(fsl.MCFLIRT(save_mats=True, save_plots=True),
-                      name='mcflirt', mem_gb=mem_gb * 3)
+    mc = pe.Node(afni.Volreg(args='-prefix NULL -twopass',
+                             zpad=4, outputtype='NIFTI_GZ'), name="mc", mem_gb=mem_gb * 3)


Do we need to specify outputtype if -prefix NULL?

No, but we never use the output so there is no need for saving it (even if Nipype will delete it anyway).

chrisgorgo · 2018-09-14T14:06:36Z

Is a 2005 comparison paper still a valid criterion for making such a move?

Probably not alone.

chrisgorgo · 2018-09-17T20:20:42Z

Tests are passing, but before I finish the PR by updating the documentation lets decide if we want to do this switch. @oesteban @effigies

effigies · 2018-09-17T20:51:06Z

I'm okay with moving to 3dvolreg by default, primarily on the grounds of decreasing dependence on FSL-licensed code.

However, given that we've gone through the trouble of implementing a MCFLIRT pathway, does it make sense to leave it as an option? Particularly from the perspective of people eventually using fMRIPrep to compare the quality of different algorithms/implementations, it seems worth making this a swappable component.

oesteban · 2018-10-04T17:48:31Z

I'm putting together the last release before RC1. My idea was to upgrade the version minor after that last release (i.e. start 1.2.x).

I think this is a somewhat large change that would fit better within the next 1.2.0, rather than 1.1.8.

WDYT?

chrisgorgo · 2018-10-04T17:53:08Z

I agree it's fairly substantial.

oesteban

Looks good overall. Left few comments.

oesteban · 2018-10-04T18:56:37Z

fmriprep/__about__.py

@@ -105,6 +105,7 @@
    'scikit-image',
    'versioneer',
    'pyyaml',
+    'transforms3d'


please add trailing comma

oesteban · 2018-10-04T19:01:24Z

requirements.txt

@@ -1,4 +1,4 @@
-niworkflows>=0.4.3
+git+https://github.com/poldracklab/niworkflows.git@fdf20103b9dedd4632bb8e26a5eb3b896e349fd2#egg=niworkflows


0.4.3 has this commit already, please revert this change (I guess the niworkflows dependency has been updated since you sent the PR)

oesteban · 2018-10-04T19:04:41Z

fmriprep/workflows/bold/hmc.py


-    fsl2itk = pe.Node(MCFLIRT2ITK(), name='fsl2itk',
-                      mem_gb=0.05, n_procs=omp_nthreads)
+    afni2itk = pe.Node(niu.Function(function=afni2itk_func,


If there's nothing against it, I'll convert this function node into a VOLREG2ITK fully-fledged nipype interface before merging in, similarly to MCFLIRT2ITK. I can remember that parallelizing that one was tricky but paid off with an important speed up on long datasets.

fmriprep/utils/misc.py

…/3dvolreg

oesteban · 2018-10-18T19:52:33Z

First results out of the oven: https://580-66973489-gh.circle-artifacts.com/0/tmp/ds054/derivatives/fmriprep/sub-100185.html

&

https://581-66973489-gh.circle-artifacts.com/0/tmp/ds005/derivatives/fmriprep/sub-01.html

oesteban · 2018-10-19T03:24:20Z

This is waiting on my offline tests comparing master vs. this PR on a real dataset.

effigies · 2018-10-23T15:28:11Z

@oesteban Any results from those tests?

oesteban · 2018-10-23T15:39:50Z

Not yet. I've been blocked with other things. I should have a decent run on ds005 with current master, so I just need to test out this branch.

oesteban · 2018-10-24T16:41:46Z

I think this is a thumbs up, see below (cc @effigies):

Master:

3dVolReg:

Master:

3dVolReg:

Master:

3dVolReg:

effigies

Looks good. That said, I think there's a fairly major bug here. See below comment.

Also, did we want to permit selection of MCFLIRT/3dVolReg with a CLI flag (I agree let's make 3dVolReg default)? If someone did want to run an apples-to-apples comparison of the tools (such as a replication of Oakes, et al., 2005, and potentially relevant to @miykael's Neurostars post), that would be useful infrastructure.

effigies · 2018-10-24T17:06:21Z

fmriprep/utils/misc.py

+    imgs = nb.four_to_three(nb.load(in_file))
+    for i, img in enumerate(imgs):
+        out_file = os.path.abspath('vol%04d.nii.gz' % i)
+        img = remove_rotation_and_shear(img)


I think we need to do img = nb.as_closest_canonical(img) before this. Consider an image with the affine:

>>> img.affine array([[ 0. , 3. , 0. , -88.4322052 ], [ -1.66738188, 0. , 2.6871357 , -47.71268463], [ 3.35891914, 0. , 1.33390486, -77.85404205], [ 0. , 0. , 0. , 1. ]]) >>> img.shape (33, 64, 64)

>>> remove_rotation_and_shear(img).affine array([[ 3.75000002, 0. , 0. , -88.4322052 ], [ 0. , 3. , 0. , -47.71268463], [ 0. , 0. , 3.00000007, -77.85404205], [ 0. , 0. , 0. , 1. ]]) >>> remove_rotation_and_shear(nb.as_closest_canonical(img)).affine array([[ 3. , 0. , 0. , -88.4322052 ], [ 0. , 3.00000007, 0. , -47.71268463], [ 0. , 0. , 3.75000002, -77.85404205], [ 0. , 0. , 0. , 1. ]])

Notice that both are RAS matrices, but with different zooms. And if you look at the shapes:

>>> remove_rotation_and_shear(img).shape (33, 64, 64) >>> remove_rotation_and_shear(nb.as_closest_canonical(img)).shape (64, 64, 33)

We clearly have a rotated data matrix, which cannot be true if both correctly have an RAS orientation. So we end up destroying all orientation information, not just the "off-diagonal" information, unless we rotate the data matrix first.

effigies · 2018-10-24T17:57:55Z

Also, there are docs to update: https://github.com/poldracklab/fmriprep/search?q=mcflirt&unscoped_q=mcflirt

oesteban · 2018-10-26T15:58:55Z

Good job folks

oesteban · 2018-10-27T01:11:18Z

Sorry I was too fast:

https://5616-53175327-gh.circle-artifacts.com/0/tmp/data/reports/ds001362/sub-01_task-taskname_run-01_bold_masks.svg

With the new bold references, it seems data are changed the orientation. I'm unsure whether we just need to update the masks or this regression reflects a deeper problem. But it seems clear that it was introduced here: https://circleci.com/gh/chrisfilo/fmriprep/616#artifacts/containers/0

Sorry about that :(

effigies · 2018-10-27T01:24:47Z

Hmm. Looks like the tests aren't failing properly with 0 overlap. Or possibly they're close enough voxel-wise but the affines are far off.

oesteban · 2018-10-27T01:29:36Z

Yup

…

On Fri, Oct 26, 2018, 18:24 Chris Markiewicz ***@***.***> wrote: Hmm. Looks like the tests aren't failing properly with 0 overlap. Or possibly they're close enough voxel-wise but the affines are far off. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1283 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkhxsiZCF7H9CHpLiIVnICG4ya4eLxxks5uo7XfgaJpZM4WoNvd> .

Revert #1283 enh/3dvolreg

chrisgorgo added 4 commits September 13, 2018 16:04

Replace MCFLIRT with 3dVolReg

682e60d

Replace MCFLIRT with 3dVolReg

829d762

new way to deoblique

7874bc7

make flake8 happy

4450915

effigies reviewed Sep 14, 2018

View reviewed changes

chrisgorgo mentioned this pull request Sep 14, 2018

Replacing MCFLIRT with 3dVolReg in FMRIPREP nih-fmrif/code-convergence-summaries#2

Open

chrisgorgo added 5 commits September 14, 2018 10:58

pin niworkflows to feature branch that resamples sbref

e2abc33

better fancier "deobliquing"

ec9da7e

updating niworkflows dependency

0a90c83

fixes

ca7d364

update niworkflows pin

add3e00

Merge branch 'master' into enh/3dvolreg

0420e3d

oesteban changed the title ~~[WIP] Replace MCFLIRT with 3dVolReg~~ [ENH] Replace FSL mcflirt with AFNI 3dVolReg Oct 3, 2018

oesteban reviewed Oct 4, 2018

View reviewed changes

oesteban added 9 commits October 16, 2018 15:55

Merge remote-tracking branch 'upstream/master' into enh/3dvolreg

5d70673

fix niworkflows pinning

27b824f

make new nipype interface

4abb15c

update docstring and boilerplate

3d93a9c

fix too long lines

3a8a2fa

fix error using fname_presuffix

61b094b

fix comment

1ce323b

Merge remote-tracking branch 'upstream/master' into enh/3dvolreg

7a5fae9

[skip ds005] Merge branch 'hotfix/reports-3' into enh/3dvolreg

c0db97f

oesteban reviewed Oct 18, 2018

View reviewed changes

fmriprep/utils/misc.py Outdated Show resolved Hide resolved

fmriprep/utils/misc.py Outdated Show resolved Hide resolved

Update fmriprep/utils/misc.py

b86e278

Update fmriprep/utils/misc.py

40a77b6

oesteban mentioned this pull request Oct 18, 2018

[FIX] Quick update to new template structure #1330

Merged

oesteban added 2 commits October 18, 2018 09:09

Merge branch 'fix/osf-templates' into enh/3dvolreg

1d77247

Merge branch 'enh/3dvolreg' of github.com:chrisfilo/fmriprep into enh…

145bde0

…/3dvolreg

oesteban mentioned this pull request Oct 19, 2018

Use pypreprocess.realign for motion estimation #444

Closed

2 tasks

effigies added this to the 1.2.0 milestone Oct 24, 2018

effigies requested changes Oct 24, 2018

View reviewed changes

oesteban and others added 7 commits October 25, 2018 18:53

Merge remote-tracking branch 'upstream/master' into enh/3dvolreg

e9de427

fix bug nipreps#1283 (comment)

bc32b8b

revise mentions to mcflirt

93c712b

revise documentation & add mcflirt flag

649737c

clean-up boilerplate

93fc896

boilerplate: fix afni version

55b4236

TEST: Add use_mcflirt to base test

bc48f64

effigies approved these changes Oct 26, 2018

View reviewed changes

oesteban merged commit b9576a5 into nipreps:master Oct 26, 2018

oesteban added a commit that referenced this pull request Oct 27, 2018

Merge pull request #1353 from poldracklab/revert-1283-enh/3dvolreg

ab686c9

Revert #1283 enh/3dvolreg

oesteban added a commit that referenced this pull request Oct 27, 2018

Revert "Revert #1283 enh/3dvolreg"

94fdc83

oesteban mentioned this pull request Oct 27, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Replace FSL `mcflirt` with AFNI `3dVolReg` #1283

[ENH] Replace FSL `mcflirt` with AFNI `3dVolReg` #1283

chrisgorgo commented Sep 13, 2018

effigies left a comment

effigies Sep 14, 2018

chrisgorgo Sep 14, 2018

effigies Sep 14, 2018

effigies Sep 14, 2018

chrisgorgo Sep 14, 2018

chrisgorgo commented Sep 14, 2018 •

edited

Loading

chrisgorgo commented Sep 17, 2018

effigies commented Sep 17, 2018

oesteban commented Oct 4, 2018

chrisgorgo commented Oct 4, 2018

oesteban left a comment

oesteban Oct 4, 2018

oesteban Oct 4, 2018

oesteban Oct 4, 2018

oesteban commented Oct 18, 2018 •

edited

Loading

oesteban commented Oct 19, 2018

effigies commented Oct 23, 2018

oesteban commented Oct 23, 2018

oesteban commented Oct 24, 2018

effigies left a comment

effigies Oct 24, 2018

oesteban Oct 24, 2018

effigies commented Oct 24, 2018

oesteban commented Oct 26, 2018

oesteban commented Oct 27, 2018

effigies commented Oct 27, 2018

oesteban commented Oct 27, 2018 via email

		@@ -1,4 +1,4 @@
		niworkflows>=0.4.3
		git+https://github.com/poldracklab/niworkflows.git@fdf20103b9dedd4632bb8e26a5eb3b896e349fd2#egg=niworkflows

[ENH] Replace FSL mcflirt with AFNI 3dVolReg #1283

[ENH] Replace FSL mcflirt with AFNI 3dVolReg #1283

Conversation

chrisgorgo commented Sep 13, 2018

effigies left a 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

Choose a reason for hiding this comment

chrisgorgo commented Sep 14, 2018 • edited Loading

chrisgorgo commented Sep 17, 2018

effigies commented Sep 17, 2018

oesteban commented Oct 4, 2018

chrisgorgo commented Oct 4, 2018

oesteban left a 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 commented Oct 18, 2018 • edited Loading

oesteban commented Oct 19, 2018

effigies commented Oct 23, 2018

oesteban commented Oct 23, 2018

oesteban commented Oct 24, 2018

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Oct 24, 2018

oesteban commented Oct 26, 2018

oesteban commented Oct 27, 2018

effigies commented Oct 27, 2018

oesteban commented Oct 27, 2018 via email

[ENH] Replace FSL `mcflirt` with AFNI `3dVolReg` #1283

[ENH] Replace FSL `mcflirt` with AFNI `3dVolReg` #1283

chrisgorgo commented Sep 14, 2018 •

edited

Loading

oesteban commented Oct 18, 2018 •

edited

Loading