ENH: Side-track sMRIPrep if a path of anatomical derivatives is given #2036

oesteban · 2020-03-14T00:31:56Z

Summary

This PR utilizes nipreps/smriprep#107 to allow a fast skip of the
anatomical workflow evaluation, and the need of maintaining the work
directory for it.

Closes #1175

Changes proposed in this pull request

Added an --anat-derivatives flag to the command line.
Integrated the new flag with the new config module.
Estimate existing derivatives using sMRIPrep's tooling.
Pass in the existing derivatives and ENH: Enable anatomical fast track reusing existing derivatives smriprep#107
skips the anatomical workflow fulfilling the workflow API.

Potential problems

This PR does not contain any measures to preserve the reproducibility of
fMRIPrep if the fast-track is used.

auto-comment · 2020-03-14T00:32:01Z

Thank your for raising your pull request.

Some of the fMRIPRep maintainers will review your changes as soon as time permits.
I'm attaching below a Review Checklist for the reviewers, to check off during the
review.

PR Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

As the reviewer I confirm that there are no conflicts of interest for me to review this work.

Please check what applies in the following aspects of the PR:

Code documentation

New functions and modules are documented following the guidelines.
Existing code required amendments in documentation and has been updated.
Sufficient inline comments ensure readability by other contributors in the long term.

Documentation site

The modifications to fMRIPrep in this PR do not require extra documentation. This is a common situation when a bug fix that does not change the code base substantially is submitted.
This PR requires documentation. The corresponding documentation will be submitted as an additional PR in the future.
This PR requires extra documentation, and will be included within this PR. Please check the boxes that apply best:
- An existing feature is substantially modified, changing the interface and/or logic of a module.
- A new feature is being added, therefore full documentation of it will be included
- This PR addresses an error in existing documentation.
Yes, all expected sections of documentation were generated by the CI build, and look as expected

Tests

The new functionalities in this PR are covered by the existing tests
New test batteries have been included with this PR

Data

This PR does not require any additional data to be uploaded to OSF.
This PR requires additional data for tests.

Dependencies: smriprep

This PR does not require any update on smriprep; or
smriprep has correctly been pinned.

Dependencies: niworkflows

This PR does not require any update on niworkflows; or
niworkflows has correctly been pinned.

Dependencies: sdcflows

This PR does not require any update on sdcflows; or
sdcflows has correctly been pinned.

Dependencies: Nipype

This PR does not require any update on nipype; or
nipype has correctly been pinned.

Dependencies: other

This PR does not require any update on other dependencies; or
other dependencies have correctly been pinned.

Reports generated within CI tests

Yes, I have checked the reports of ds005 and they are not modified or the changes are as expected
Yes, I have checked the reports of ds054 and they are not modified or the changes are as expected
Yes, I have checked the reports of ds010 and they are not modified or the changes are as expected

oesteban · 2020-03-18T16:25:03Z

I'm hitting this #2041 because sMRIPrep stores the fsnative-to-T1w (and inverse) matrices in ITK format to the output folder, but fMRIPrep expects them to be LTA for LTA concatenation (with the bbregister's LTA).

I think this is the best reason to push nitransforms forward (and actually the reason why we started with it at all originally). It should be capable of:

Replacing the LTA -> ITK conversions currently done with Freesurfer tools (sMRIPrep)
Replacing the LTA -> FSL -> ITK conversions with FreeSurfer and C3D (fMRIPrep)
Replacing all LTA concatenations and storing ITK concatenated (or lists, if we want to keep full provenance and full floating-point accuracy) transforms (both sMRIPrep and fMRIPrep).

Preparing nitransforms for this (generating a package, fixing nipy/nitransforms#64) will require a fair amount of work.

WDYT? - should we try to get over with this asap, or find a workaround (i.e., convert back to LTA with FS tools) and finalize later?

oesteban · 2020-03-30T06:42:09Z

Unless I'm missing something, this report shows results using NiTransforms instead of mri_concatenate_lta, LTA convert and c3d (for LTA -> FSL -> ITK). I'll check in more depth tomorrow, but they look good at first glance.

The final step is to alter the smoke tests to use the new flag (and the proposed feature). It's working locally on ds005, though.

Before that final step, it might be appropriate to have a first review here (cc/ @mgxd, @effigies).

oesteban · 2020-04-02T18:50:39Z

@effigies, @mgxd this seems to be settling in. Let's see the latest CircleCI workflow. If that comes out green, I'll add the fasttrack to ds054 and ds210.

I think it's about time to finalize nipreps/smriprep#107 and get that one merged - WDYT?

Another thought is that this particular feature will make it easier to quickly tests out things on other extensions (i.e., infants). If we are given a dataset with all the anatomical derivatives we need, we can work right away on the adaptations to the functional workflow (hopefully just the sampling of CIFTI2 files) without being held up by anatomical issues.

oesteban · 2020-04-04T17:28:24Z

Okay, the --no-fs-reconall jobs are not properly working on Circle. I believe the ordering of the segments of the segmentation is not correct. Will look into this asap

effigies

I don't think I finished reviewing but I won't get back to this before the afternoon, so might as well hit submit.

wrapper/fmriprep_docker.py

effigies · 2020-04-03T17:53:29Z

fmriprep/workflows/bold/resampling.py

@@ -113,7 +112,8 @@ def select_target(subject_id, space):
                         mem_gb=DEFAULT_MEMORY_MIN_GB)
    resampling_xfm = pe.Node(LTAConvert(in_lta='identity.nofile', out_lta=True),
                             name='resampling_xfm')
-    set_xfm_source = pe.Node(ConcatenateLTA(out_type='RAS2RAS'), name='set_xfm_source')
+    merge_xfm = pe.Node(niu.Merge(2), name="merge_xfm", run_without_submitting=True)
+    concat_xfm = pe.Node(ConcatenateXFMs(out_fmt="fs"), name="concat_xfm")


Does ConcatenateXFMs with out_fmt=='fs' ensure that no line exceeds 255 characters?

I think it's left empty because the filename = metadata is not set. But there's no failsafe for that in nitransforms right now, so one could think of setting it and getting bitten by this.

docs/faq.rst

fmriprep/cli/parser.py

… given Summary ------- This PR utilizes nipreps/smriprep#107 to allow a fast skip of the anatomical workflow evaluation, and the need of maintaining the work directory for it. Closes nipreps#1175 Changes suggested ----------------- * [x] Added an ``--anat-derivatives`` flag to the command line. * [x] Integrated the new flag with the new config module. * [x] Estimate existing derivatives using *sMRIPrep*'s tooling. * [x] Pass in the existing derivatives and nipreps/smriprep#107 skips the anatomical workflow fulfilling the workflow API. Potential problems ------------------ This PR does not contain any measures to preserve the reproducibility of *fMRIPrep* if the fast-track is used.

Also, update sdcflows, nitransforms pinned versions

This will test the correct ordering of TPMs when computing confounds

@effigies

As requested by @effigies.

oesteban

I think this is in a mergeable state now. Considering to set a merge freeze and get it in at last. LMKWYT

oesteban · 2020-04-18T03:54:43Z

docs/faq.rst

@@ -271,3 +266,22 @@ Therefore, if your *fMRIPrep* process crashes and you attempt to re-run it reusi
 as much as it could from the previous run, you can either make sure that
 the default ``$PWD/work/`` points to a reasonable, reusable path in your environment or
 configure a better location on your with ``-w <PATH>``.
+
+Can I use *fMRIPrep* for longitudinal studies?


I don't really know why I added this. If you feel this is worth splitting out to another PR, happy to do it.

oesteban · 2020-04-18T03:55:47Z

fmriprep/workflows/bold/base.py

+        mem_gb=mem_gb['resampled'],
+        name='bold_reg_wf',
+        omp_nthreads=omp_nthreads,
+        sloppy=config.execution.debug,


Added this sloppy mode argument downstream.

oesteban · 2020-04-18T04:00:28Z

fmriprep/workflows/bold/registration.py

+    if sloppy is True:
+        downsample = pe.Node(niu.Function(
+            function=_conditional_downsampling, output_names=["out_file", "out_mask"]),
+            name='downsample')
+        workflow.connect([
+            (inputnode, downsample, [("t1w_brain", "in_file")]),
+            (wm_mask, downsample, [("out", "in_mask")]),
+            (downsample, flt_bbr, [('out_file', 'reference'),
+                                   ('out_mask', 'wm_seg')]),


With the utilization of cached, high-resolution anatomical derivatives, the time it took for flirt/bbr to run exploded. With this patch, I minimized flirt/bbr from ~5min to ~11sec (on my desktop, it must be a bit longer on CircleCI). Some other nodes have extended their duration a bit as when used pre-computed anatomical derivatives, but I think now the timing is reasonable.

I used this simple script: https://gist.github.com/oesteban/0c17459936deb18ac81377db4b4763c2 to measure times. We should consider running something like that in fmriprep to get some stats about execution etc.

oesteban · 2020-04-18T04:01:50Z

setup.cfg

    numpy
    pandas
    psutil >= 5.4
-    pybids >= 0.10.0
+    pybids >= 0.10.2


This is necessary (uses build_path)

oesteban · 2020-04-27T17:53:48Z

@effigies, @mgxd can you find some time window to take a second pass on this? Let's try to get it merged before Friday - WDYT?

effigies · 2020-04-27T18:45:30Z

Yup. Will have a look this afternoon.

effigies

Mostly rewording documentation. One question about a potential qform/sform footgun.

docs/usage.rst

fmriprep/workflows/bold/registration.py

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

mgxd

minor comments, LGTM

docs/usage.rst

mgxd · 2020-04-27T21:18:16Z

setup.cfg

@@ -25,14 +26,15 @@ install_requires =
    nibabel ~= 3.0
    nipype ~= 1.4
    nitime
+    nitransforms >= 20.0.0rc3,<20.2


should we pin this to master as well?

we'll need to set a release pin to cut the RC, so I'd leave this. 20.2 should be tolerant enough to allow nitransforms progress without breaking anything used in here.

wrapper/fmriprep_docker.py

Co-Authored-By: Mathias Goncalves <goncalves.mathias@gmail.com>

oesteban · 2020-04-28T00:19:11Z

oesteban force-pushed the enh/1175-smriprep-fasttrack branch from ecdd708 to cedbfbe Compare March 14, 2020 01:29

oesteban mentioned this pull request Mar 17, 2020

ENH: Add a --skull-strip-t1w argument to skip brain extraction #2039

Merged

oesteban added this to the 20.1.0 milestone Mar 18, 2020

oesteban force-pushed the enh/1175-smriprep-fasttrack branch from cedbfbe to 657edac Compare March 18, 2020 02:02

oesteban modified the milestones: 20.1.x, 20.1.0 LTS Mar 25, 2020

oesteban force-pushed the enh/1175-smriprep-fasttrack branch 2 times, most recently from c1736b7 to 8dfc8e9 Compare March 30, 2020 03:11

oesteban changed the title ~~ENH/WIP: Side-track *sMRIPrep* if a path of anatomical derivatives is given~~ ENH: Side-track *sMRIPrep* if a path of anatomical derivatives is given Mar 30, 2020

oesteban mentioned this pull request Mar 30, 2020

ENH: Enable anatomical fast track reusing existing derivatives nipreps/smriprep#107

Merged

5 tasks

oesteban marked this pull request as ready for review April 2, 2020 18:46

oesteban force-pushed the enh/1175-smriprep-fasttrack branch from 8b03e9f to 687df86 Compare April 3, 2020 08:01

effigies reviewed Apr 6, 2020

View reviewed changes

mgxd reviewed Apr 6, 2020

View reviewed changes

fmriprep/cli/parser.py Outdated Show resolved Hide resolved

oesteban force-pushed the enh/1175-smriprep-fasttrack branch from f84ef4c to 0ab1a70 Compare April 17, 2020 00:53

oesteban added 8 commits April 17, 2020 00:34

feat: incoporate nitransforms to replace mri_concatenate_lta

3903bee

enh: add fasttrack to ds005 smoke test

66e0b6e

Also, update sdcflows, nitransforms pinned versions

enh: update fmriprep-wrapper to handle --anat-derivatives

05b88cd

fix(wrapper): add --ants-derivatives to whitelist of clobbering opts

c32e2dd

fix(ds005): update the outputs conformity check

0fb82ec

docs(faq): add two FAQs about the new flag, and longitudinal studies

fc93731

test: add --anat-derivatives to ds054 and ds210 too

2926a8c

oesteban force-pushed the enh/1175-smriprep-fasttrack branch from 0ab1a70 to 7a5ebe7 Compare April 17, 2020 07:45

fix(ds054): amend entry in outputs list that was removed by mistake

08c4a29

oesteban force-pushed the enh/1175-smriprep-fasttrack branch from 7a5ebe7 to 08c4a29 Compare April 17, 2020 07:49

oesteban marked this pull request as draft April 17, 2020 17:10

oesteban added 4 commits April 17, 2020 10:51

chore: pin smriprep to tip commit of nipreps/smriprep#179

25b7563

This will test the correct ordering of TPMs when computing confounds

enh: downsample inputs on sloppy mode for FSL FLIRT-bbr

173cf24

fix: address @mgxd's comment

4641481

docs: remove new FAQ and create a new section under usage

66c4766

As requested by @effigies.

oesteban marked this pull request as ready for review April 18, 2020 03:53

oesteban commented Apr 18, 2020

View reviewed changes

chore: roll-back change on niworkflows pin / sty change

29b05be

effigies reviewed Apr 27, 2020

View reviewed changes

Apply suggestions from code review

ea4e7b4

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies approved these changes Apr 27, 2020

View reviewed changes

mgxd approved these changes Apr 27, 2020

View reviewed changes

Apply suggestions from code review

3edced6

Co-Authored-By: Mathias Goncalves <goncalves.mathias@gmail.com>

oesteban merged commit 9509b31 into nipreps:master Apr 28, 2020

oesteban deleted the enh/1175-smriprep-fasttrack branch April 28, 2020 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Side-track sMRIPrep if a path of anatomical derivatives is given #2036

ENH: Side-track sMRIPrep if a path of anatomical derivatives is given #2036

oesteban commented Mar 14, 2020

auto-comment bot commented Mar 14, 2020

oesteban commented Mar 18, 2020

oesteban commented Mar 30, 2020

oesteban commented Apr 2, 2020

oesteban commented Apr 4, 2020

effigies left a comment

effigies Apr 3, 2020

oesteban Apr 8, 2020

oesteban left a comment

oesteban Apr 18, 2020

oesteban Apr 18, 2020

oesteban Apr 18, 2020

oesteban Apr 18, 2020

oesteban commented Apr 27, 2020

effigies commented Apr 27, 2020

effigies left a comment

mgxd left a comment

mgxd Apr 27, 2020

oesteban Apr 27, 2020

oesteban commented Apr 28, 2020

ENH: Side-track *sMRIPrep* if a path of anatomical derivatives is given #2036

ENH: Side-track *sMRIPrep* if a path of anatomical derivatives is given #2036

Conversation

oesteban commented Mar 14, 2020

Summary

Changes proposed in this pull request

Potential problems

auto-comment bot commented Mar 14, 2020

PR Review

Code documentation

Documentation site

Tests

Data

Dependencies: smriprep

Dependencies: niworkflows

Dependencies: sdcflows

Dependencies: Nipype

Dependencies: other

Reports generated within CI tests

oesteban commented Mar 18, 2020

oesteban commented Mar 30, 2020

oesteban commented Apr 2, 2020

oesteban commented Apr 4, 2020

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban commented Apr 27, 2020

effigies commented Apr 27, 2020

effigies left a comment

Choose a reason for hiding this comment

mgxd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban commented Apr 28, 2020

ENH: Side-track sMRIPrep if a path of anatomical derivatives is given #2036

ENH: Side-track sMRIPrep if a path of anatomical derivatives is given #2036