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: Use t1w2fsnative_xfm to resample segmentations #201

Merged
merged 3 commits into from
Jun 7, 2020

Conversation

effigies
Copy link
Member

@effigies effigies commented Jun 7, 2020

Closes #154.

As noted in #154, if fsnative is not pre-aligned with the T1w template fMRIPrep produces, aparc/aseg do not get properly aligned, and due to using FreeSurfer outputs to refine the brainmask, everything suffers.

This first pipes the t1w2fsnative_xfm to the segmentation workflows (init_segs_to_native_wf). This workflow had used mri_label2vol + mri_convert. The mri_label2vol was already converting to NIfTI, so I dropped mri_convert.

I manually confirmed that using mri_label2vol with the inverse LTA gets the alignment right.

I also noticed that mri_label2vol has label, annotation and volume modes. Since aseg.mgz and aparc+aseg.mgz are already volumes (.mgz, rather than .label or .annot), I checked whether mri_vol2vol could be used.

That also worked. Even better, it takes its transforms in the usual direction (source -> target), so we don't have to leave comments about why we either pass the inverted LTA file, or pass the normal file and an "invert" flag.

Because the interface changed, I changed the name of the node.


Bug-fix status: This changes the workflow, but it does so in a way that should keep working directories reusable. And it fixes a bug in several versions of fMRIPrep. So I would suggest that we make an exception and release this as a bug-fix, which will allow us to fix the 20.0 and 20.1 series (and backport to 1.5, if it ever comes up).

@effigies effigies force-pushed the fix/resample_segs branch 2 times, most recently from 0f581fd to 780f875 Compare June 7, 2020 02:33
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

Maybe it's going to be easier to change the base to maint/20.0.x to avoid the license problem. Then progress changes to 20.1 and master (and backport if we get on a commitment strike).

@oesteban
Copy link
Member

oesteban commented Jun 7, 2020

Okay it was late when I reviewed this and I was probably unclear on my suggestion. What I meant with the PR base is:

  • Can you base the PR on maint/0.5.x?
  • We release a bugfix on sMRIPrep-0.5.x and also fMRIPrep-20.0.x
  • We figure out the canary problem elsewhere
  • We port to maint/0.6.x and master.

@effigies effigies changed the base branch from master to maint/0.5.x June 7, 2020 14:37
@effigies effigies merged commit 8d0a37e into nipreps:maint/0.5.x Jun 7, 2020
@effigies effigies deleted the fix/resample_segs branch June 7, 2020 16:41
@effigies
Copy link
Member Author

effigies commented Jun 7, 2020

maint/0.5.x can be safely merged into maint/0.6.x.

Diff:

diff --git a/.circleci/config.yml b/.circleci/config.yml
index ba2167709..09d304ca7 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -249,7 +249,8 @@ jobs:
             pyenv local $PY2
             echo -n "Python version: "
             python --version
-            pip install --upgrade pip setuptools
+            pip install --upgrade "pip<21"
+            pip install --upgrade setuptools
             pip install --upgrade wrapper/
             which smriprep-docker
             smriprep-docker -i poldracklab/smriprep:latest --help
diff --git a/smriprep/workflows/surfaces.py b/smriprep/workflows/surfaces.py
index 9e6702323..cd77b1536 100644
--- a/smriprep/workflows/surfaces.py
+++ b/smriprep/workflows/surfaces.py
@@ -231,10 +231,12 @@ gray-matter of Mindboggle [RRID:SCR_002438, @mindboggle].
         (autorecon_resume_wf, aseg_to_native_wf, [
             ('outputnode.subjects_dir', 'inputnode.subjects_dir'),
             ('outputnode.subject_id', 'inputnode.subject_id')]),
+        (fsnative2t1w_xfm, aseg_to_native_wf, [('out_reg_file', 'inputnode.fsnative2t1w_xfm')]),
         (inputnode, aparc_to_native_wf, [('corrected_t1', 'inputnode.in_file')]),
         (autorecon_resume_wf, aparc_to_native_wf, [
             ('outputnode.subjects_dir', 'inputnode.subjects_dir'),
             ('outputnode.subject_id', 'inputnode.subject_id')]),
+        (fsnative2t1w_xfm, aparc_to_native_wf, [('out_reg_file', 'inputnode.fsnative2t1w_xfm')]),
         (aseg_to_native_wf, refine, [('outputnode.out_file', 'in_aseg')]),
 
         # Output
@@ -502,6 +504,8 @@ def init_segs_to_native_wf(name='segs_to_native', segmentation='aseg'):
         FreeSurfer SUBJECTS_DIR
     subject_id
         FreeSurfer subject ID
+    fsnative2t1w_xfm
+        LTA-style affine matrix translating from FreeSurfer-conformed subject space to T1w
 
     Outputs
     -------
@@ -510,13 +514,15 @@ def init_segs_to_native_wf(name='segs_to_native', segmentation='aseg'):
 
     """
     workflow = Workflow(name='%s_%s' % (name, segmentation))
-    inputnode = pe.Node(niu.IdentityInterface([
-        'in_file', 'subjects_dir', 'subject_id']), name='inputnode')
+    inputnode = pe.Node(
+        niu.IdentityInterface(['in_file', 'subjects_dir', 'subject_id', 'fsnative2t1w_xfm']),
+        name='inputnode')
     outputnode = pe.Node(niu.IdentityInterface(['out_file']), name='outputnode')
     # Extract the aseg and aparc+aseg outputs
     fssource = pe.Node(nio.FreeSurferSource(), name='fs_datasource')
-    tonative = pe.Node(fs.Label2Vol(), name='tonative')
-    tonii = pe.Node(fs.MRIConvert(out_type='niigz', resample_type='nearest'), name='tonii')
+    # Resample from T1.mgz to T1w.nii.gz, applying any offset in fsnative2t1w_xfm,
+    # and convert to NIfTI while we're at it
+    resample = pe.Node(fs.ApplyVolTransform(transformed_file='seg.nii.gz'), name='resample')
 
     if segmentation.startswith('aparc'):
         if segmentation == 'aparc_aseg':
@@ -531,12 +537,10 @@ def init_segs_to_native_wf(name='segs_to_native', segmentation='aseg'):
         (inputnode, fssource, [
             ('subjects_dir', 'subjects_dir'),
             ('subject_id', 'subject_id')]),
-        (inputnode, tonii, [('in_file', 'reslice_like')]),
-        (fssource, tonative, [(segmentation, 'seg_file'),
-                              ('rawavg', 'template_file'),
-                              ('aseg', 'reg_header')]),
-        (tonative, tonii, [('vol_label_file', 'in_file')]),
-        (tonii, outputnode, [('out_file', 'out_file')]),
+        (inputnode, resample, [('in_file', 'target_file'),
+                               ('fsnative2t1w_xfm', 'lta_file')]),
+        (fssource, resample, [(segmentation, 'source_file')]),
+        (resample, outputnode, [('transformed_file', 'out_file')]),
     ])
     return workflow

@mgxd
Copy link
Collaborator

mgxd commented Jun 12, 2020

@effigies does this also close #200?

@effigies
Copy link
Member Author

No.

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.

aparc/aseg do not appear to be adjusted for any fsnative2t1w differences
3 participants