[REVIEW] Fallback to initial registration, if BBR fails #694

effigies · 2017-09-07T19:33:02Z

The easiest way to read this PR is to separately review #741, and read effigies/fmriprep@fsfix_common...bbr_fallback.

Closes #681.

effigies · 2017-09-18T20:27:09Z

This is ready for a review. It's working locally on @jdkent's dataset. Both FreeSurfer and FSL are falling back to mri_coreg/FLIRT.

The only thing left to do is fix cost parsing for non-BBR FLIRT so that we can compare registration performance to determine phase-encoding direction for SyN-SDC.

Depends: nipreps/niworkflows#195.

oesteban · 2017-09-18T21:41:41Z

fmriprep/workflows/bold.py

+        FunctionalSummary(output_spaces=output_spaces,
+                          registration='FreeSurfer' if freesurfer else 'FSL',
+                          registration_dof=bold2t1w_dof),
+        name='summary', mem_gb=DEFAULT_MEMORY_MIN_GB)


run_without_submitting=True perhaps?

oesteban · 2017-09-18T21:44:04Z

fmriprep/workflows/util.py

        name='bbregister')

+    transforms = pe.Node(niu.Merge(2), name='transforms')


All these new nodes should run_without_submitting=True?

oesteban · 2017-09-18T21:45:47Z

fmriprep/workflows/util.py

+
+    fallback = any((max_trans > 2, rot > np.pi / 36, max_scale > 1.1))
+
+    return 0 if fallback else 1


why not keeping the boolean type here (and downstream)?

It was intended as an index, but I can reverse these. That probably would be cleaner.

oesteban · 2017-09-18T21:46:48Z

fmriprep/workflows/util.py

+    rot = mat2axangle(rotation_matrix)[1]
+    max_scale = np.max(np.abs(scales))
+
+    fallback = any((max_trans > 2, rot > np.pi / 36, max_scale > 1.1))


2mm is not too small?

That might be. What seems like a reasonable translation threshold? It seems like it shouldn't be more than a couple voxels, right? (I think I picked 2 as a reasonable "1 voxel" threshold, but it was admittedly a quick and dirty guess.)

effigies · 2017-09-19T18:29:42Z

fmriprep/workflows/util.py

+
+    shift_magnitude = np.sqrt(trans.dot(trans))  # 2-norm
+    rot = mat2axangle(rotation_matrix)[1]
+    max_scale = np.max(np.abs(scales))


Two changes here:

Use the 2-norm to get total shift, not just max in any direction

Increase shift threshold to 5mm

effigies · 2017-09-20T14:43:25Z

The translation differences are large, and I believe it's a function of the .mat file format.

The FSL convention for transformation matrices uses an implicit centre of transformation - that is, a point that will be unmoved by that transformation, which is an arbitrary choice in general. This arbitrary centre of the transformation for FSL is at the mm origin (0,0,0) which is at the centre of the corner voxel of the image.

When using the transformation parameters from FLIRT, there is an additional complication in that the parameters are calculated in a way that uses a different centre convention: the centre of mass of the volume. The effect of this is that each of the three matrices above end up with an adjustment in the fourth column (top three elements only) that represents a shift between the corner origin and the centre of mass, while the rest of the matrix (first three columns) is unaffected. Once that is done the matrices are multiplied together, as indicated above, and you get your final matrix.

It may be that we just need to drop the translation check altogether.

oesteban · 2017-09-20T15:13:49Z

👍 I was ok with checking only the scaling and so I remain. The center being in a corner will also change the concept of angle, so those parameters will not be "trustworthy" either.

…

On Sep 20, 2017 07:43, "Chris Markiewicz" ***@***.***> wrote: The translation differences are large, and I believe it's a function of the .mat file format <https://fsl.fmrib.ox.ac.uk/fsl/fslwiki/FLIRT/FAQ#What_are_FLIRT_schedule_files.3F> . The FSL convention for transformation matrices uses an implicit centre of transformation - that is, a point that will be unmoved by that transformation, which is an arbitrary choice in general. This arbitrary centre of the transformation for FSL is at the mm origin (0,0,0) which is at the centre of the corner voxel of the image. When using the transformation parameters from FLIRT, there is an additional complication in that the parameters are calculated in a way that uses a different centre convention: the centre of mass of the volume. The effect of this is that each of the three matrices above end up with an adjustment in the fourth column (top three elements only) that represents a shift between the corner origin and the centre of mass, while the rest of the matrix (first three columns) is unaffected. Once that is done the matrices are multiplied together, as indicated above, and you get your final matrix. It may be that we just need to drop the translation check altogether. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#694 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkhxsmkMbs3C5Jvmxw3KsXZVO3kTaUjks5skSSNgaJpZM4PQSXp> .

effigies · 2017-09-20T15:15:41Z

Right. I was just playing with converting to LTA, and got identical scaling parameters but quite different translation and rotation angles.

effigies · 2017-09-20T20:34:48Z

Looking at a lot of these, I'm not sure that there's a clear scaling threshold. Some are mostly rotations/translations, and a small rotation with a very large translation can cause as much distortion as a small(ish) translation with a large rotation. One possibility could be to have a threshold on some function of rotation and translation...

I'm going to try converting to LTA, which I believe uses the RAS=(0,0,0) origin, which should at least be no worse than FSL's corner origin, and may result in more obvious thresholds.

I think there's a possibility we'll need override options, no matter how we decide this, allowing users to force BBR or fallback when our heuristics fail.

effigies · 2017-09-25T19:41:41Z

Just as an update on this, I'm definitely not getting the level of distortion @jdkent was in his original post. For the most part, the bbregister results look reasonable, while the FLIRT+BBR results are being rejected.

@jdkent Do you think you could re-run that subject with the latest release? I'm a little concerned that I'm no longer replicating the issue and getting bad metrics to find cut-offs for.

jdkent · 2017-09-25T19:42:49Z

I'll re-reun with the latest release and report back

effigies · 2017-09-25T23:40:29Z

I think I'm converging on using the "norm" from ArtifactDetect, which is a kind of compromise between @chrisfilo's idea of using framewise displacement and my earlier approach of using a differential affine matrix. The norm approach took a series of motion parameters, constructed affines, and finds the norm of the displacement, which is the maximum displacement of the centers of a bounding cube, and thus takes into account rotations, translations and scales (and shears, but we're only using 9-dof transforms).

With nipy/nipype#2198, I separate the norm calculation from constructing affines from the motion parameters, allowing us to directly pass the affine transform matrices.

The affines in LTA format (RAS2RAS) seem to produce differences of norms that correspond intelligibly with distortion, with norm > 20 being a pretty reliable indicator of a bad BBR, and < 10 indicating good. The exact threshold isn't obvious, with some that I marked "accept" and "reject" falling on either side as I moved between 15 and 20. For now I've put 15, and I'll see what you all think.

I'll package up the spreadsheet and visualizations first thing in the morning.

Given that I still haven't found a clear threshold, I'm inclined to add --force-bbr and --force-no-bbr options.

oesteban · 2017-10-09T21:06:23Z

@jdkent, @effigies what is the current status of this PR?

effigies · 2017-10-09T21:21:21Z

This is ready to review. I still like the affine difference we're taking, and think the main thing that needs to be decided is if we want a more stringent threshold.

jdkent · 2017-10-11T03:59:51Z

Alright, I ran the subject with --force-bbr and without the flag, and the results are much improved, just not identical to @effigies. I ran it with a clear PYTHONPATH and in different working directories, one thing that isn't clean is the fact that I'm using an existing freesurfer directory, and not re-rerunning recon-all. However, the function did work and improved my results, so I don't think I should be holding up this pull request any longer.

effigies · 2017-10-11T13:33:51Z

@oesteban Rebased and ready for review.

effigies · 2017-10-11T13:40:30Z

@jdkent Your "without the flag" one looks very similar to mine, so I'm glad we're in a similar place. How do you feel about that threshold, by the way? We can make it more likely to reject that first run. The more I look at these results, the more I think we might want to make the threshold a little more stringent...

Thanks for testing this out for us, by the way.

jdkent · 2017-10-11T14:55:55Z

oh, I thought the bbregister result was rejected for all the registrations. Then yes, a more stringent threshold could catch that case and improve the registration. I just don't know how much optimization for my case would sacrifice the generalization of the threshold to other datasets. Also, thank you for making this great tool, the work you are doing is awesome.

oesteban

Just some cosmetic comments. Great work. Thanks @jdkent for following up.

oesteban · 2017-10-11T15:34:54Z

fmriprep/workflows/util.py

 from niworkflows.interfaces.masks import SimpleShowMaskRPT

 from ..interfaces.images import extract_wm

 DEFAULT_MEMORY_MIN_GB = 0.01


+def compare_xforms(lta_list, norm_threshold=15):
+    import numpy as np


Please add a docstring with a link to the original issue to get quickly to the discussion and examples of how the threshold plays out (e.g. 10 = very small affine changes accepted and 20 = large changes accepted). Something along these lines.

""" Computes a *distance* between two transforms as the maximum overall displacement of the midpoints of the faces of a cube due to translation and rotation. See discussion `here <https://github.com/poldracklab/fmriprep/issues/681>`_. Parameters lta_list : list or tuple of str the two given affines in LTA format norm_threshold : int the upper bound limit to the distance of the second transform w.r.t. the first; the lower the smaller distances are accepted; a default of 15 was found empirically appropriate to decide whether BBR transforms should be rejected. """

Added link to original issue.

oesteban · 2017-10-11T15:38:26Z

fmriprep/workflows/util.py

-        BBRegisterRPT(dof=bold2t1w_dof, contrast_type='t2', init='coreg',
-                      registered_file=True, out_lta_file=True, generate_report=True),
-        name='bbregister')
+    mri_coreg = pe.Node(


Does MRICoreg generate a new T1w space with the appropriate xforms? I'll need to get this clear before I start with #745.

No. mri_coreg only generates a transform matrix. If you want to apply the transform, you can use mri_vol2vol (see MRICoregRPT or mri_convert).

oesteban · 2017-10-11T15:42:06Z

fmriprep/workflows/util.py

@@ -174,7 +194,7 @@ def init_skullstrip_bold_wf(name='skullstrip_bold_wf'):
    return workflow


-def init_bbreg_wf(bold2t1w_dof, name='bbreg_wf'):
+def init_bbreg_wf(use_bbr, bold2t1w_dof, omp_nthreads, name='bbreg_wf'):


An additional paragraph describing the feature being added would be very valuable. Just a couple of sentences to describe what happens if use_bbr=None.

oesteban · 2017-10-11T15:42:47Z

fmriprep/workflows/util.py

        ])

    return workflow


-def init_fsl_bbr_wf(bold2t1w_dof, name='fsl_bbr_wf'):
+def init_fsl_bbr_wf(use_bbr, bold2t1w_dof, name='fsl_bbr_wf'):


Refer to previous workflow regarding use_bbr

effigies force-pushed the bbr_fallback branch 2 times, most recently from 8793bc7 to aaec592 Compare September 8, 2017 18:53

jdkent mentioned this pull request Sep 13, 2017

Poor skullstripping/registration BOLD->T1w results #701

Closed

effigies force-pushed the bbr_fallback branch 3 times, most recently from ad9b29b to c9eb35e Compare September 18, 2017 19:06

effigies changed the title ~~[WIP] Fallback to initial registration, if BBR fails~~ [REVIEW] Fallback to initial registration, if BBR fails Sep 18, 2017

oesteban reviewed Sep 18, 2017

View reviewed changes

effigies force-pushed the bbr_fallback branch 2 times, most recently from a4e1ba6 to 80565ec Compare September 19, 2017 18:19

effigies commented Sep 19, 2017

View reviewed changes

effigies force-pushed the bbr_fallback branch 2 times, most recently from 3a0847b to bf747e8 Compare September 19, 2017 20:41

effigies force-pushed the bbr_fallback branch from aed0038 to f271f3d Compare September 20, 2017 16:30

effigies force-pushed the bbr_fallback branch 2 times, most recently from 591af0d to f840db0 Compare September 25, 2017 18:26

effigies force-pushed the bbr_fallback branch from f840db0 to 6989314 Compare September 25, 2017 20:26

effigies force-pushed the bbr_fallback branch 2 times, most recently from 60c51b6 to e88a8a7 Compare September 26, 2017 12:30

effigies added 15 commits October 11, 2017 09:17

ENH: Break mri_coreg out from bbregister

860d411

ENH: Enable first-pass reporting for FLIRT

291d2ac

ENH: Compare transforms before/after bbregister

15b512d

ENH: Compare transforms before/after FLIRT BBR

3839cde

REPORT: Update report names and summary to reflect registration used

b297914

RF: Update thresholds

8716799

DOC: Use simple graph for bold_reg [docs only]

0509970

LOG: Print transform difference criteria

3e6cc2f

CI: Preserve .mat files for out-of-band comparison

513b550

ENH: Get LTA transforms for all registration steps

ac12f75

ENH: Use affine difference norm

1e65e25

CLI: Add --force(-no)-bbr options

a6d7870

RF: Add use_bbr switching to fsl_bbr_wf

109dba0

RF: Add use_bbr switching to bbreg_wf

4497b56

RF: Switch comparisons from FSL to LTA

8b4ec04

effigies force-pushed the bbr_fallback branch from 786db5b to 8b4ec04 Compare October 11, 2017 13:31

oesteban approved these changes Oct 11, 2017

View reviewed changes

effigies added 2 commits October 11, 2017 12:41

RF: Use load_transform

bd84448

DOC: Update BBR docs

d7f9b29

oesteban mentioned this pull request Oct 11, 2017

Solicit data where non-BBR registrations outperform BBR #333

Closed

oesteban merged commit ef1e129 into nipreps:master Oct 11, 2017

effigies deleted the bbr_fallback branch October 11, 2017 18:47

oesteban mentioned this pull request Oct 12, 2017

ds000232 registration issues #734

Closed

effigies mentioned this pull request Mar 26, 2018

Document BBR fallback. #1035

Closed

		name='bbregister')

		transforms = pe.Node(niu.Merge(2), name='transforms')


		fallback = any((max_trans > 2, rot > np.pi / 36, max_scale > 1.1))

		return 0 if fallback else 1

[REVIEW] Fallback to initial registration, if BBR fails #694

[REVIEW] Fallback to initial registration, if BBR fails #694

Uh oh!

Conversation

effigies commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Sep 18, 2017

Uh oh!

oesteban Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Sep 20, 2017

Uh oh!

oesteban commented Sep 20, 2017 via email

Uh oh!

effigies commented Sep 20, 2017

Uh oh!

effigies commented Sep 20, 2017

Uh oh!

effigies commented Sep 25, 2017

Uh oh!

jdkent commented Sep 25, 2017

Uh oh!

effigies commented Sep 25, 2017

Uh oh!

oesteban commented Oct 9, 2017

Uh oh!

effigies commented Oct 9, 2017

Uh oh!

jdkent commented Oct 11, 2017

Uh oh!

effigies commented Oct 11, 2017

Uh oh!

effigies commented Oct 11, 2017

Uh oh!

jdkent commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oesteban left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oesteban Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

effigies commented Sep 7, 2017 •

edited

Loading

oesteban Sep 18, 2017 •

edited

Loading

jdkent commented Oct 11, 2017 •

edited

Loading

oesteban left a comment •

edited

Loading

oesteban Oct 11, 2017 •

edited

Loading