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

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

Merged
merged 17 commits into from Oct 11, 2017

Conversation

Projects
None yet
3 participants
@effigies
Copy link
Collaborator

commented Sep 7, 2017

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

Closes #681.

@effigies effigies force-pushed the effigies:bbr_fallback branch 2 times, most recently from 8793bc7 to aaec592 Sep 8, 2017

@effigies effigies force-pushed the effigies:bbr_fallback branch 3 times, most recently from ad9b29b to c9eb35e Sep 18, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 18, 2017

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: poldracklab/niworkflows#195.

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

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

This comment has been minimized.

Copy link
@oesteban

oesteban Sep 18, 2017

Contributor

run_without_submitting=True perhaps?

name='bbregister')

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

This comment has been minimized.

Copy link
@oesteban

oesteban Sep 18, 2017

Contributor

All these new nodes should run_without_submitting=True?


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

return 0 if fallback else 1

This comment has been minimized.

Copy link
@oesteban

oesteban Sep 18, 2017

Contributor

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

This comment has been minimized.

Copy link
@effigies

effigies Sep 19, 2017

Author Collaborator

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

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))

This comment has been minimized.

Copy link
@oesteban

oesteban Sep 18, 2017

Contributor

2mm is not too small?

This comment has been minimized.

Copy link
@effigies

effigies Sep 19, 2017

Author Collaborator

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 effigies force-pushed the effigies:bbr_fallback branch 2 times, most recently from a4e1ba6 to 80565ec Sep 19, 2017


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

This comment has been minimized.

Copy link
@effigies

effigies Sep 19, 2017

Author Collaborator

Two changes here:

  1. Use the 2-norm to get total shift, not just max in any direction
  2. Increase shift threshold to 5mm

@effigies effigies force-pushed the effigies:bbr_fallback branch 2 times, most recently from 3a0847b to bf747e8 Sep 19, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2017

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

@effigies effigies force-pushed the effigies:bbr_fallback branch from aed0038 to f271f3d Sep 20, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2017

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 effigies force-pushed the effigies:bbr_fallback branch 2 times, most recently from 591af0d to f840db0 Sep 24, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 25, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2017

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

@effigies effigies force-pushed the effigies:bbr_fallback branch from f840db0 to 6989314 Sep 25, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 25, 2017

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.

@effigies effigies force-pushed the effigies:bbr_fallback branch 2 times, most recently from 60c51b6 to e88a8a7 Sep 26, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2017

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 effigies force-pushed the effigies:bbr_fallback branch from 786db5b to 8b4ec04 Oct 11, 2017

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 11, 2017

@oesteban Rebased and ready for review.

@effigies

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 11, 2017

@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

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2017

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
Copy link
Contributor

left a comment

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

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

This comment has been minimized.

Copy link
@oesteban

oesteban Oct 11, 2017

Contributor

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.

"""

This comment has been minimized.

Copy link
@oesteban

oesteban Oct 11, 2017

Contributor

Added link to original issue.

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(

This comment has been minimized.

Copy link
@oesteban

oesteban Oct 11, 2017

Contributor

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

This comment has been minimized.

Copy link
@effigies

effigies Oct 11, 2017

Author Collaborator

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).

@@ -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'):

This comment has been minimized.

Copy link
@oesteban

oesteban Oct 11, 2017

Contributor

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.

])

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'):

This comment has been minimized.

Copy link
@oesteban

oesteban Oct 11, 2017

Contributor

Refer to previous workflow regarding use_bbr

@oesteban oesteban merged commit ef1e129 into poldracklab:master Oct 11, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@effigies effigies deleted the effigies:bbr_fallback branch Oct 11, 2017

@effigies effigies referenced this pull request Mar 26, 2018

Closed

Document BBR fallback. #1035

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.