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.

Sign up for GitHub

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

Jump to bottom

ENH: Enable anatomical fast track reusing existing derivatives #107

Merged
merged 12 commits into from
Apr 4, 2020
Merged

ENH: Enable anatomical fast track reusing existing derivatives #107

merged 12 commits into from
Apr 4, 2020

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Sep 12, 2019 •
edited
Loading

Summary

These changes modify the workflow construction function init_anat_preproc_wf to add a checkpoint on the new named argument existing_derivatives at the beginning of the process.
When existing_derivatives is a dictionary containing all the anatomical derivatives to be computed, then the workflow is short-cut and only the reportlets are re-generated.

Changes

  • Implementation of the fast-track.
  • New reportlets generation

Other changes in this PR include:

  • Boilerplate on the smriprep executable to populate existing_derivatives that may serve as pattern reference to include this characteristic in fmriprep and dmriprep. The boilerplate includes a new --force-build argument to smriprep's command line that allows dismissing the check on existing derivatives, hence running the full workflow with standard nipype operation as regards caching.
  • A new smriprep.utils.bids.collect_derivatives function that populates existing_derivatives, which can (should) be imported to use the feature in f/dMRIPrep.
  • Modifications to the CircleCI interface, so that commits tagged with [force-build] are added the new --force-build tag. Commits to master are automatically added the flag.

Related issues

Closes #105.

@oesteban oesteban force-pushed the enh/105-derivatives-peeking branch 9 times, most recently from c9a5383 to 11e1e8c Compare September 13, 2019 23:39
@oesteban oesteban force-pushed the enh/105-derivatives-peeking branch 5 times, most recently from a83f3d1 to 1489776 Compare September 15, 2019 02:19
oesteban added a commit to oesteban/smriprep that referenced this pull request Sep 15, 2019
@oesteban
ENH: Reduce friction when iterating over target templates
4d06de1 
Spatial normalization defines selected ``--output-spaces`` as an
iterable to the workflow.
This PR removes several ad-hoc function nodes and replaces them with a
new nipype interface to query templates.
These changes will facilitate concluding nipreps#107.
@oesteban oesteban mentioned this pull request Sep 15, 2019
Merged
oesteban added a commit to oesteban/smriprep that referenced this pull request Sep 17, 2019
@oesteban
ENH: Decoupling anatomical reports
a48d949 
This PR delays report generation to the ``anat_report_wf`` workflow
which is staged at the end.
This decoupling will allow creating reportlets from existing derivatives
in the context of nipreps#107.
@oesteban oesteban mentioned this pull request Sep 17, 2019
Merged
oesteban added a commit to oesteban/smriprep that referenced this pull request Sep 17, 2019
@oesteban
ENH: Decoupling anatomical reports
8c10fab 
This PR delays report generation to the ``anat_report_wf`` workflow
which is staged at the end.
This decoupling will allow creating reportlets from existing derivatives
in the context of nipreps#107.
@oesteban oesteban changed the title ENH: Enable anatomical fast track reusing existing derivatives [WIP] ENH: Enable anatomical fast track reusing existing derivatives Sep 17, 2019
@oesteban oesteban mentioned this pull request Sep 17, 2019
Merged
@effigies
Copy link
Member

effigies commented Oct 7, 2019

@oesteban I'm assuming this is still WIP?

@oesteban
Copy link
Member Author

oesteban commented Oct 7, 2019

Yup, it is. Although I wasn't too far away from finishing up.

@effigies
Copy link
Member

effigies commented Oct 7, 2019

Okay. Just clicked through and saw you requested a review, so wanted to make sure you weren't waiting on that.

@oesteban
Copy link
Member Author

oesteban commented Oct 7, 2019

The review could be very helpful. I'm not blocked by that though - I hit a couple of edge cases - just need to make sure all the derivatives we then reuse are generated (e.g., one of the tk2ras or vice-versa for FreeSurfer was missing).
If you prefer to hold until the PR is in a mergeable state, that's fine.

@oesteban oesteban mentioned this pull request Nov 26, 2019
Closed
@adelavega adelavega mentioned this pull request Dec 20, 2019
Closed
@oesteban oesteban mentioned this pull request Mar 14, 2020
Merged
4 tasks
@oesteban oesteban mentioned this pull request Mar 17, 2020
Merged
@oesteban oesteban force-pushed the enh/105-derivatives-peeking branch from 0bbc87a to 2de3b5b Compare March 18, 2020 17:51
@oesteban
fix(api-changes): do not expose listified joint_ outputs
6292298 
This commit stops exposing the parameterized outputs (by iterables),
namely ``template``, ``anat2std_xfm`` and ``std2anat_xfm``.
@oesteban oesteban force-pushed the enh/105-derivatives-peeking branch from 2de3b5b to 6292298 Compare March 18, 2020 22:34
@oesteban oesteban added this to the 0.6.0 milestone Mar 18, 2020
@oesteban oesteban requested a review from mgxd March 30, 2020 06:46
@oesteban
Copy link
Member Author

I think this is finished, currently working with nipreps/fmriprep#2036 -- I was waiting on that one to be finished or very nearly finished before pushing on this one for the final stretch.

mgxd
mgxd approved these changes Mar 30, 2020
View reviewed changes
Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

looking good

@oesteban
Copy link
Member Author

oesteban commented Apr 2, 2020

Last call for comments - this is going in otherwise... :D

@effigies
Copy link
Member

effigies commented Apr 2, 2020

I've started looking a couple times, but won't be able to finish until tomorrow afternoon, so if you need it in before then, go ahead without me.

@oesteban
Copy link
Member Author

oesteban commented Apr 2, 2020

I am looking forward to merging this asap, but I also think your vetting is more important. Waiting for one more day will not hurt at all, and not waiting will potentially bite back. I'll hold on.

effigies
effigies approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

So it looks like an all-or-nothing thing. If any expected derivative is not found, then the whole thing is rerun. I would assume the long-term goal is to allow individual steps to be replaced, but this seems like a good first step.

Thanks for your patience.

@oesteban
Copy link
Member Author

oesteban commented Apr 4, 2020

So it looks like an all-or-nothing thing. If any expected derivative is not found, then the whole thing is rerun.

Correct.

I would assume the long-term goal is to allow individual steps to be replaced, but this seems like a good first step.

Yup, that'd be the plan. There are a lot of details we need to figure out to continue with it. And yes, the scope of this is just one first step.

Thanks for your patience.

Thanks for your time!

@oesteban oesteban merged commit 1cdf321 into nipreps:master Apr 4, 2020
@oesteban oesteban deleted the enh/105-derivatives-peeking branch April 4, 2020 00:07
@oesteban oesteban mentioned this pull request Apr 15, 2020
Closed
oesteban added a commit to oesteban/fmriprep that referenced this pull request Apr 17, 2020
@oesteban
ENH/WIP: Side-track *sMRIPrep* if a path of anatomical derivatives is…
75a1507 
… 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.
@oesteban oesteban mentioned this pull request Apr 17, 2020
Closed
oesteban added a commit to oesteban/dmriprep that referenced this pull request May 6, 2020
@oesteban
ENH: Port the new *anatomical fast-track* from *fMRIPrep*
7890f64 
Uses nipreps/smriprep#107 to skip precalculated anatomical
derivatives, just setting the ``--anat-derivatives <path>`` command line
argument.
@oesteban oesteban mentioned this pull request May 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgxd mgxd mgxd approved these changes

@effigies effigies effigies approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.

Modular sMRIPrep with derivatives peeking option
5 participants
@oesteban @effigies @satra @pep8speaks @mgxd