REF/FIX: Simplify transform aggregation in resampling, pass identity transforms for multi-echo cases

[emdupre]

Closes #2236
Closes #2238

Changes proposed in this pull request

• Simplifies transformation aggregation (i.e., no longer passing variable number of xforms; unused transforms are passed as identity)
• Refactor MEEPI connections for improved legibility
• Removes redundant fieldwarp application for MEEPI data

Documentation that should be reviewed

N/A

[effigies]

Could you rebase onto maint/20.1.x? As a bug-fix, this should patch smoothly. (If it does not, I can change back to master.)

[effigies]

LGTM. Thanks!

[oesteban]

Seems like the multi-echo smoke test is smoking... Looks like the split node is hanging.

[effigies]

Do we need to reset a Circle cache?

[effigies]

Hmm. It looks like we're getting caught on a bold_split inside bold_t1_trans_wf:

200812-03:15:04,302 nipype.workflow INFO:
	 [MultiProc] Running 1 tasks, and 0 jobs ready. Free memory (GB): 4.09/4.10, Free processors: 1/2.
                     Currently running:
                       * fmriprep_wf.single_subject_02_wf.func_preproc_task_cuedSGT_run_01_echo_1_wf.bold_t1_trans_wf.bold_split

[effigies]

https://github.com/poldracklab/fmriprep/blob/c717c6b216886be5de75627c067018700f0efd75/fmriprep/workflows/bold/registration.py#L349-L358

[pep8speaks]

Hello @emdupre! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-14 02:46:11 UTC

[emdupre]

@effigies is this what you meant by passing an 'identity' transform to ANTs ?

[effigies]

It will need to be directly entered, not through a connection.

[effigies]

Here are suggestions to demonstrate what I was going for. We actually do want to connect things in the sub-workflow, but set the values directly in the input to the workflow.

Also, this is now a refactor, not a bug fix, so we should rebase back onto master. I will set the target branch now.

[effigies]

I think what we want in general is the following

    merge_xforms = pe.Node(niu.Merge(3), name='merge_xforms',
                           run_without_submitting=True, mem_gb=DEFAULT_MEMORY_MIN_GB)
    workflow.connect([
        # merge transforms
        (inputnode, merge_xforms, [
            ('hmc_xforms' , 'in3'),
            ('fieldwarp', 'in2'),
            ('itk_bold_to_t1', 'in1')]),
        (merge_xforms, bold_to_t1w_transform, [('out', 'transforms')]),
        (inputnode, bold_to_t1w_transform, [('bold_split', 'input_image')]),
    ])

[emdupre]

We still want to vary the number of transforms depending on whether or not use_fieldwarp is active, no ?

[effigies]

Oh, sorry, I didn't see this. No, the idea is that we make that input be identity.

[effigies]

Opened: nipreps/sdcflows#108

[effigies]

Did a little cleaning up. If you haven't already pushed forward, could you fetch my remote and cherry-pick bbdb59645548f510458ee4d11ac3c34b94edf4f6?

Otherwise, I'll rebase on whatever you've done.

[emdupre]

OK, so it seems that the merge_xforms node in bold_t1_trans_wf is correctly merging the transforms for ds210:

> from nipype.utils.filemanip import loadpkl                                       

> res = loadpkl('result_merge_xforms.pklz')  
> res.outputs
out = ['/scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/bold_reg_wf/fsl_bbr_wf/fsl2itk_fwd/affine.txt', 'identity', 'identity']

the problem is that the workflow can't actually apply this merged transform:

nipype.workflow CRITICAL:
    fMRIPrep failed: /scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/bold_t1_trans_wf/bold_to_t1w_transform/result_bold_to_t1w_transform.pklz

Will this involve an update to MultiApplyTransforms ? That's the interface that's being called as bold_to_t1w_transform here:

https://github.com/poldracklab/fmriprep/blob/c717c6b216886be5de75627c067018700f0efd75/fmriprep/workflows/bold/registration.py#L320-L322

[effigies]

Lol, yup.

[effigies]

I think this is ready to go. Since I contributed, a separate reviewer would be good.

Significant changes since

[tsalo]

Looks good to me. But just to clarify, it now appears that HMC+SDC xforms were applied to multi-echo data not twice, but three times?

[tsalo]

The code changes look good (as mentioned in my review), but I did notice that the reports don't seem to have improved. If I look at the ds210 report from this PR and compare it to the one from #2185, the SDC and coregistration sections look exactly the same. Does that make sense?

[effigies]

It should only be once. We're now passing identity transforms if multi-echo is enabled.

What should be changing in the reports?

[tsalo]

Sorry, I mean that I thought the xforms were being applied three times before this fix. Once before T2* estimation, then again in the bold_to_T1w workflow, and then again in the bold_to_std workflow.

I just realized that the bold_to_T1w workflow doesn't feed into the bold_to_std workflow, but the HMC and SDC xforms should still have been applied twice before this fix (once before T2* estimation and once during either bold_to_t1w or bold_to_std, depending on the output), right? I assumed that the reports would show less distortion correction after the fix, possibly solving #2230 (see #2230 (comment)).

[effigies]

@tsalo HMC and SDC are not being reapplied in bold_t1_trans_wf in the multi-echo case.

[effigies]

@tsalo Similarly for bold_std_trans_wf

[tsalo]

Because that's the specific issue that this is fixing (i.e., #2236), right? Therefore, distortion correction on multi-echo data should have been exaggerated before the fix and should be better now.

[effigies]

Yes.

[emdupre]

The data in the reports don't have any fieldmaps collected. I wonder if it's more visible if we compared reports from ME data with fieldmaps ?

[tsalo]

@emdupre Possibly, but shouldn't the SyN warp, when applied twice, result in exaggerated distortion correction?

[effigies]

For the reports, it looks like we show the BOLD reference before and after. Both images are generated pre bold_bold_trans_wf, and so won't reflect any changes to the inputs of any of the *_trans_wfs.

https://github.com/poldracklab/fmriprep/blob/c2228f009af081137b375cb61a9c2d0a98793234/fmriprep/workflows/bold/base.py#L544-L549

[tsalo]

Oh okay. That makes sense. Instead of looking at the reports, then, we need to look at the preprocessed data to see if it's better. I downloaded the MNI-space preproc data from both runs and I think the difference is substantial (although not very easy to see without being able to toggle between them in fslview).

Before fix

After fix

[effigies]

Agreed. I downloaded them, and I would say the new one looks noticeably less distorted. I'm going to merge, and we can ask @JulioAPeraza to rerun with :unstable once the CI completes.

[tsalo]

@JulioAPeraza, it looks like nipreps/niworkflows#556 is preventing the DockerHub build from passing, so you may need to wait until that's fixed before you can test this change.