[ENH] Ensure that headers are consistent along the process #743

oesteban · 2017-10-07T01:00:24Z

This PR fixes #710.

The problem was the corruption of xform matrices (sform, qform). Requires nipreps/niworkflows#202

…workflows pin

effigies

Reviewed this first. A couple quick comments, but I think this will probably be fine when nipreps/niworkflows#202 goes in.

effigies · 2017-10-08T20:14:58Z

fmriprep/interfaces/itk.py

@@ -31,8 +32,8 @@ class MCFLIRT2ITKInputSpec(BaseInterfaceInputSpec):
                        desc='input image for spatial reference')
    in_source = File(exists=True, mandatory=True,
                     desc='input image for spatial source')
-    nprocs = traits.Int(1, usedefault=True, nohash=True,
-                        desc='number of parallel processes')
+    n_procs = traits.Int(1, usedefault=True, nohash=True,


If you set this to num_threads, then it will be synced with node.n_procs.

effigies · 2017-10-08T20:16:02Z

fmriprep/interfaces/itk.py

@@ -71,10 +87,13 @@ class MultiApplyTransformsInputSpec(ApplyTransformsInputSpec):
                                      ' through the fourth dimension')
    nprocs = traits.Int(1, usedefault=True, nohash=True,
                        desc='number of parallel processes')


Same here with num_threads.

effigies · 2017-10-09T01:59:15Z

fmriprep/workflows/util.py

@@ -81,10 +82,13 @@ def init_enhance_and_skullstrip_bold_wf(name='enhance_and_skullstrip_bold_wf',
                            name='combine_masks')
    apply_mask = pe.Node(fsl.ApplyMask(),
                         name='apply_mask')
+    copy_xform = pe.Node(CopyXForm(), name='copy_xform',
+                         mem_gb=0.1, run_without_submitting=True)


Just looked at this again. The only ANTs tool here is N4BiasFieldCorrection, which we already updated to have copy_header=True option built-in in nipype. I think this node isn't actually needed?

That surprised me the first time. If you check output of this workflow, you'll see that it actually changes xforms. A suspicious 'descrip' field of 'FSL 5.0' makes me think that bet, apply mask or image maths may also change xforms. I don't see a problem on explicitly making sure that xforms are OK with this node.

This change of xform was the origin of #710.

That's maddening. I'm going to need to get mad about this again in the morning. Sorry if I've been asking dumb questions.

It's actually unifize that seems to be causing issues here:

$ fslhd skullstrip_first_pass/ref_image_corrected_brain.nii.gz | grep -A 14 qform_name qform_name Scanner Anat qform_code 1 qto_xyz:1 -3.125000 0.000000 -0.000000 97.931519 qto_xyz:2 0.000000 3.125000 -0.000000 -68.019211 qto_xyz:3 0.000000 0.000000 4.000000 -106.650566 qto_xyz:4 0.000000 0.000000 0.000000 1.000000 qform_xorient Right-to-Left qform_yorient Posterior-to-Anterior qform_zorient Inferior-to-Superior sform_name Aligned Anat sform_code 2 sto_xyz:1 -3.125000 0.000000 0.000000 98.909752 sto_xyz:2 0.000000 3.125000 0.000000 -110.716141 sto_xyz:3 0.000000 0.000000 4.000000 -51.800713 sto_xyz:4 0.000000 0.000000 0.000000 1.000000

$ fslhd unifize/uni.nii.gz | grep -A 14 qform_name qform_name Talairach qform_code 3 qto_xyz:1 -3.125000 -0.000000 -0.000000 98.909752 qto_xyz:2 0.000000 3.125000 -0.000000 -110.716141 qto_xyz:3 -0.000000 0.000000 4.000000 -51.800713 qto_xyz:4 0.000000 0.000000 0.000000 1.000000 qform_xorient Right-to-Left qform_yorient Posterior-to-Anterior qform_zorient Inferior-to-Superior sform_name Talairach sform_code 3 sto_xyz:1 -3.125000 -0.000000 -0.000000 98.909752 sto_xyz:2 -0.000000 3.125000 -0.000000 -110.716141 sto_xyz:3 0.000000 0.000000 4.000000 -51.800713 sto_xyz:4 0.000000 0.000000 0.000000 1.000000

... What?

I've submitted an issue (afni/afni#65), but unless neurodebian starts updating AFNI regularly (or we start using a more recent neurodocker build), we're still going to need to work around this for the foreseeable future.

Great job! For the 'descrip' header I assumed it was FSL, but that wouldn't rule out unifize.

Okay, it looks like this is a WONTFIX. We'll want to consider how we interface with AFNI tools in general.

Thinking more long-term, I've proposed nipy/nipype#2223.

oesteban · 2017-10-09T22:33:23Z

I'm double checking this on the dataset with freesurfer on. Also checking if this fixes also #708

oesteban · 2017-10-10T01:49:18Z

Bad news: this PR either does not fix #710 when freesurfer is used or I'm seeing #703 again for reusing the pre-computed freesurfer. Either way I need to get to the bottom before merging this.

oesteban · 2017-10-10T15:55:08Z

So, with the new code I don't see the problem of #708 on ds101, sub-06. I'll check more subjects from that PR.

On the other hand, I repeated (reusing pre-computed freesurfer) on ds001, sub-02 and this issue is still happening.

oesteban · 2017-10-10T16:51:17Z

I am considering that we are now dealing with a side effect of #741 for two reasons:

I've checked the headers of the inputs (the reference BOLD for bbregister and the split volumes for the resampling step) and now they match correctly. This was wrong before the changes in this PR.
The BBR path seems to work well (as a consequence of the previous fix) so we are probably looking at a bbregister problem and the conversion of matrices to ITK.

WDYT @effigies?

effigies · 2017-10-10T16:54:06Z

Ugh. That's possible. (Though at least on my tests, I get the exact same results whether we go through your branch or 0.6.5.)

effigies · 2017-10-10T17:02:26Z

Can you update this line?

https://github.com/poldracklab/fmriprep/blob/2a2ac713bfa61c36123ea05aa227759879c58eb5/fmriprep/workflows/util.py#L239

With the latest nipype updates, we can use:

fs.ConcatenateLTA(out_type='RAS2RAS')

I don't think it should make a difference, since it should do the RAS2RAS conversion in the process of creating the ITK affine, but just to check...

effigies · 2017-10-10T17:26:56Z

Oh, I think I see the issue. The bbregister-targeted file is T1w, but the FLIRT-targeted file is T1w downsampled to BOLD resolution. There's been no update step targeting the BOLD-resolution T1w file.

oesteban · 2017-10-10T17:36:06Z

The resulting ITK transform should be in mm (meaning, the resolution of the target space is not relevant). I'm going to check apply the transform using the full-resolution T1w as reference and I bet we will get the same.

effigies · 2017-10-10T17:37:47Z

Yeah, I just did that. Made no difference...

oesteban · 2017-10-10T17:40:45Z

Basically, what the affine transform does is: for each node of the sampling grid, get the coordinates in mm (physical space) and find the coordinates in mm that correspond to that point in moving space.

Therefore, changing the fixed image resolution you only change the location of the points you want to find in moving space. The transform is the same for both images (if it is defined in mm like ITK, this changes if you define transforms in pixels like legacy registration algorithms).

oesteban · 2017-10-10T17:43:31Z

The out_type='RAS2RAS' did not make any difference :(

effigies · 2017-10-10T17:46:48Z

Okay, if I manually lta_convert to FSL and then c3d_affine_tool to ITK, then I get alignment. Want me to send you a patch, or should we keep going until we find something more elegant?

oesteban · 2017-10-10T18:00:00Z

I'm fine with that patch, but you put a lot of work on those LTA, so I'm not going to rush for a fix. It's your call :)

…

On Oct 10, 2017 10:46, "Chris Markiewicz" ***@***.***> wrote: Okay, if I manually lta_convert to FSL and then c3d_affine_tool to ITK, then I get alignment. Want me to send you a patch, or should we keep going until we find something more elegant? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#743 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkhxgJWF0oNevED-hKVavbSfDtwNU4vks5sq62KgaJpZM4PxLmI> .

effigies · 2017-10-10T18:50:00Z

I don't love it, but I added a hacky solution in oesteban#2. I've verified that it fixes alignment for me.

I'm going to ping FreeSurfer to see if lta_convert can get fixed by 6.0.1.

Also, so we have it written down somewhere, c3d_affine_tool can do both fwd and inv transforms in a single shot. I just didn't want to go through another round of nipype editing to get this in.

c3d_affine_tool -ref <ref> -src <src> in_xfm.mat -fsl2ras -oitk itk.txt -inv -oitk itk_inv.txt

Turns out order of operations matters a whole lot, though, which also makes designing an update to the interface a little weird. I guess you could add an inverse_itk_transform = File(argstr='-inv -oitk %s', position=6).

oesteban · 2017-10-10T20:48:29Z

Just note that the fix worked well on ds001, sub-02. I'll test more subjects from #708

oesteban · 2017-10-11T01:41:14Z

Bummer, I just noticed this error in the end of the report https://320-53608443-gh.circle-artifacts.com/0/home/ubuntu/ds054/out/fmriprep/sub-100185.html

oesteban and others added 11 commits October 6, 2017 11:21

refactor MultiApplyTransforms interface, remove joblib dep, update ni…

7796de8

…workflows pin

fix reference xforms

03595c0

pin to a current niworkflows commit

553d4ca

Merge remote-tracking branch 'upstream/master' into fix/710

fe1b3b2

Merge remote-tracking branch 'upstream/master' into fix/710

868aaa5

remove joblib import

43856ef

remove Parallel

7f031c4

fix passing parameters on _mat2itk

8865d90

Merge branch 'master' into fix/710

6d404d6

Merge remote-tracking branch 'upstream/master' into fix/710

007e0e8

update niworkflows pin

5f40706

oesteban requested a review from effigies October 8, 2017 05:40

oesteban added 2 commits October 7, 2017 23:01

store antsApplyTransform command lines

fee6bbe

remove input in time

594aee6

oesteban mentioned this pull request Oct 8, 2017

[ENH] XForm headers nipreps/niworkflows#202

Merged

effigies reviewed Oct 8, 2017

View reviewed changes

oesteban added 2 commits October 8, 2017 19:00

update niworkflows pin

6a1704b

change n_procs and variants to num_threads where necessary

ff66b17

effigies reviewed Oct 9, 2017

View reviewed changes

oesteban added 5 commits October 9, 2017 09:11

update niworkflows pin

84a4e2e

Use GenerateSamplingReference from niworkflows

3ea9c54

Merge branch 'fix/710' of github.com:oesteban/fmriprep into fix/710

9f262b0

update niworkflows pin

3172195

fix error in MultiApplyTransforms

854e056

FIX: Convert LTA to ITK by way of FSL

3234261

effigies mentioned this pull request Oct 10, 2017

lta_convert does not produce ITK transforms that are correctly applied by antsApplyTransforms freesurfer/freesurfer#169

Closed

oesteban mentioned this pull request Oct 10, 2017

anat affine appears off for ds000114 between data and fmriprep output #624

Closed

pin latest niworkflows

ba6db66

oesteban mentioned this pull request Oct 10, 2017

Fall-back registration for BBR failures #681

Closed

oesteban added 2 commits October 10, 2017 15:45

import SimpleInterface from nipype

d2d2018

update niworkflows dependency

256e9af

effigies merged commit 4d25027 into nipreps:master Oct 11, 2017

oesteban deleted the fix/710 branch October 11, 2017 01:42

This was referenced Oct 11, 2017

Make sure ComputeDVARS get the appropriate mask #751

Closed

Misaligned contours in SyN-SDC reports #708

Closed

effigies mentioned this pull request Oct 21, 2017

Make sure the xform_code is correct when generating references #745

Closed

effigies mentioned this pull request Nov 8, 2017

FIX: Immediately fix headers of AFNI outputs #818

Merged

[ENH] Ensure that headers are consistent along the process #743

[ENH] Ensure that headers are consistent along the process #743

Uh oh!

Conversation

oesteban commented Oct 7, 2017

Uh oh!

effigies left a comment

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!

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!

oesteban commented Oct 9, 2017

Uh oh!

oesteban commented Oct 10, 2017

Uh oh!

oesteban commented Oct 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

effigies commented Oct 10, 2017

Uh oh!

effigies commented Oct 10, 2017

Uh oh!

effigies commented Oct 10, 2017

Uh oh!

oesteban commented Oct 10, 2017

Uh oh!

effigies commented Oct 10, 2017

Uh oh!

oesteban commented Oct 10, 2017

Uh oh!

oesteban commented Oct 10, 2017

Uh oh!

effigies commented Oct 10, 2017

Uh oh!

oesteban commented Oct 10, 2017 via email

Uh oh!

effigies commented Oct 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

oesteban commented Oct 11, 2017

Uh oh!

Uh oh!

oesteban commented Oct 10, 2017 •

edited

Loading

oesteban commented Oct 10, 2017 •

edited

Loading