RF: Split anatomical workflow into fit and derivatives workflows #303

effigies · 2022-09-30T19:17:22Z

Starting on #300.

codecov-commenter · 2022-09-30T20:26:43Z

Codecov Report

Patch coverage: 52.25% and project coverage change: +22.56 🎉

Comparison is base (b174172) 31.18% compared to head (22f236d) 53.75%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #303       +/-   ##
===========================================
+ Coverage   31.18%   53.75%   +22.56%     
===========================================
  Files          20       22        +2     
  Lines        1456     1600      +144     
  Branches      206      251       +45     
===========================================
+ Hits          454      860      +406     
+ Misses        987      672      -315     
- Partials       15       68       +53

Flag	Coverage Δ
ds005	`∅ <ø> (∅)`
ds054	`46.31% <51.65%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
smriprep/utils/bids.py	`34.00% <4.54%> (-7.89%)`	⬇️
smriprep/workflows/surfaces.py	`12.23% <14.28%> (+0.25%)`	⬆️
smriprep/cli/run.py	`73.18% <42.85%> (+70.36%)`	⬆️
smriprep/workflows/anatomical.py	`45.41% <49.05%> (+28.85%)`	⬆️
smriprep/interfaces/surf.py	`42.85% <50.00%> (+7.24%)`	⬆️
smriprep/workflows/base.py	`75.00% <66.66%> (+45.00%)`	⬆️
smriprep/workflows/outputs.py	`55.76% <76.28%> (+43.55%)`	⬆️
smriprep/workflows/fit/registration.py	`92.50% <80.00%> (ø)`

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

effigies · 2022-11-15T16:47:10Z

Tests passing. Will start re-adding the workflow description.

effigies

@mgxd @oesteban This is passing and ready for review.

I don't think we want to try to squeeze it into 0.10.0, but I've left some comments to help guide a review.

.github/workflows/pythonpackage.yml

effigies · 2022-11-29T21:00:36Z

docs/conf.py

@@ -65,7 +65,6 @@
    "seaborn",
    "skimage",
    "svgutils",
-    "templateflow",


Hopefully we don't end up downloading things during builds, but if so, so be it.

effigies · 2022-11-29T21:02:06Z

smriprep/utils/bids.py

@@ -50,81 +51,6 @@ def get_outputnode_spec():
    return fields


-def predict_derivatives(subject_id, output_spaces, freesurfer):


This didn't seem to be used anywhere, and deleting this freed me up to change collect_derivatives to use BIDSLayout and use lists as "or" instead of "and" in io_spec.json.

effigies · 2022-11-29T21:04:35Z

smriprep/workflows/anatomical.py

    name="anat_preproc_wf",
-    skull_strip_fixed_seed=False,
+    skull_strip_fixed_seed: bool = False,


Type annotations grew out of frustration of not knowing what I was working with (particularly Reference and SpatialReference). The consequence of this was that I need to import Reference and SpatialReference at the module level, which meant that a mocked templateflow prevented the loading of spatial references.

smriprep/workflows/apply/resampling.py

effigies · 2022-11-29T21:27:54Z

smriprep/workflows/outputs.py

+    return workflow
+
+
+def init_ds_template_wf(


I feel like we need a better name, but this is the subject template, not MNI or other template spaces.

effigies · 2022-11-29T21:29:29Z

smriprep/workflows/outputs.py

    return workflow


-def init_anat_derivatives_wf(
+def init_anat_second_derivatives_wf(


This could definitely stand refactoring, but I'd prefer to keep that until later.

effigies · 2022-11-29T21:32:43Z

smriprep/workflows/outputs.py

@@ -655,12 +932,104 @@ def init_anat_derivatives_wf(
    return workflow


+def init_template_iterator_wf(*, spaces, name="template_iterator_wf"):


I like this a lot. I feel like what we're going to want one of these sitting in the subject level that then gets passed as inputs to the baseline derivatives (registrations only), reporting and full derivative workflows.

Need to spend some time to work out what that would look like, in a separate PR. For now we spin one of these up inside the reporting and full derivatives workflows.

effigies · 2022-11-29T21:33:54Z

smriprep/workflows/surfaces.py

+    if "fsnative" not in precomputed.get("transforms", {}):
+        fsnative2t1w_xfm = pe.Node(
+            RobustRegister(auto_sens=True, est_int_scale=True), name="fsnative2t1w_xfm"
+        )
+
+        # fmt:off
+        workflow.connect([
+            (inputnode, fsnative2t1w_xfm, [('t1w', 'target_file')]),
+            (autorecon1, fsnative2t1w_xfm, [('T1', 'source_file')]),
+            (fsnative2t1w_xfm, outputnode, [('out_reg_file', 'fsnative2t1w_xfm')]),
+        ])
+        # fmt:on


This was all that was needed to isolate this step, which drew attention to the fact that we're not mapping skull-strips correctly into fsnative. This should be resolved in another PR.

effigies · 2022-11-29T21:36:35Z

smriprep/workflows/surfaces.py

+        name="outputnode",
+    )
+
+    aseg_to_native_wf = init_segs_to_native_wf()


Duplicated here and in init_surface_derivatives_wf. Acceptably small duplication of effort for conceptual clarity, IMO.

…s.apply.resampling

Currently skipping report generation and LiterateWorkflow descriptions

I have verified that the sphere_reg_fsLR files is deterministic and also quick to compute, so there is no harm in pushing this out of fit.

effigies · 2023-06-19T19:30:26Z

@mgxd @oesteban I've reset the merge target to a next branch. My plan is to merge this there and make PRs against that. If we need more updates to master before the final merge, I'll merge that to next as well. Feel free to comment here even after merge (and of course PRs against next are welcome).

I will be doing a similar thing over on fMRIPrep.

effigies linked an issue Sep 30, 2022 that may be closed by this pull request

Fit/apply breakdown #300

Closed

30 tasks

effigies force-pushed the rf/split_workflow branch from d4db265 to 69ba77d Compare September 30, 2022 19:46

effigies force-pushed the rf/split_workflow branch 4 times, most recently from ca58281 to abe2c05 Compare October 14, 2022 16:41

effigies force-pushed the rf/split_workflow branch from abe2c05 to 9cbcb73 Compare November 1, 2022 16:01

effigies force-pushed the rf/split_workflow branch 3 times, most recently from 224b305 to 6be5c53 Compare November 9, 2022 13:24

effigies mentioned this pull request Nov 9, 2022

WRAPPER: Update patch location, use --patch syntax #309

Merged

effigies changed the title ~~RF: Split surface workflow into recon and derivative preparation~~ RF: Split anatomical workflow into fit and derivatives workflows Nov 9, 2022

effigies force-pushed the rf/split_workflow branch 2 times, most recently from 2b40799 to e7628a0 Compare November 10, 2022 11:48

effigies mentioned this pull request Nov 14, 2022

ENH: Allow DerivativesDataSink to handle output==input nipreps/niworkflows#758

Merged

effigies force-pushed the rf/split_workflow branch from ac6a5d9 to 0d8b632 Compare November 15, 2022 15:12

effigies force-pushed the rf/split_workflow branch from 0d8b632 to 7de4d09 Compare November 15, 2022 21:30

effigies force-pushed the rf/split_workflow branch 2 times, most recently from 8959b5e to 2bba697 Compare November 29, 2022 19:25

effigies commented Nov 29, 2022

View reviewed changes

effigies added 8 commits November 30, 2022 13:09

RF: Split surface workflow into recon and derivative preparation

8e8eea9

MNT: Update .gitignore

9dfa265

RF: Break derivatives worfklow into first and second derivatives

9b9a5ce

RF: Split workflows.norm into workflows.fit.registration and workflow…

fe60be3

…s.apply.resampling

RF: Use split workflows in anat_preproc_wf

de8e62b

RF: Factor out brainmask refinement workflow

5f152d9

RF: Reorganize anat into fit and derivatives

58b8a3a

Currently skipping report generation and LiterateWorkflow descriptions

FIX: Continue on missing fast-track items

37aade1

Drop apply workflows for now

8aa8e6c

effigies force-pushed the rf/split_workflow branch from 76b7362 to 8aa8e6c Compare November 30, 2022 18:09

effigies force-pushed the rf/split_workflow branch 2 times, most recently from e16d855 to 26ff900 Compare January 19, 2023 00:27

Merge remote-tracking branch 'upstream/master' into rf/split_workflow

653f728

effigies force-pushed the rf/split_workflow branch from bb39a29 to 653f728 Compare April 11, 2023 20:11

effigies and others added 7 commits April 11, 2023 16:59

ENH: Clean up t2w_preproc sinking

73be00f

CI: Drop unused fs-license context

b401197

FIX: Re-add anat_preproc_wf outputs

05f9cc9

FIX: Clearer skull strip variables

a5f64fc

ENH: Move anat ribbon out of primary fit workflow

066b531

FIX: Disable validation on derivative dataset

bfd418b

FIX: Connect ANTs segmentation from n4-only wf

811f52f

effigies force-pushed the rf/split_workflow branch from 9002119 to 811f52f Compare April 16, 2023 12:35

effigies and others added 4 commits April 16, 2023 19:09

Merge remote-tracking branch 'upstream/master' into rf/split_workflow

1dcac89

DOC: Add t2w arg in docstring

7f66113

Merge remote-tracking branch 'upstream/master' into rf/split_workflow

b07a423

RF: Move sphere_reg_wf into surface derivatives

39f5dea

I have verified that the sphere_reg_fsLR files is deterministic and also quick to compute, so there is no harm in pushing this out of fit.

effigies force-pushed the rf/split_workflow branch from 9b4f22c to 22f236d Compare June 19, 2023 12:56

effigies added 4 commits June 19, 2023 15:00

Merge remote-tracking branch 'upstream/master' into rf/split_workflow

a7a42e5

STY: black; adapt flake8 rules

4f3723b

STY: black; adapt flake8 rules

b771ae0

Merge remote-tracking branch 'upstream/master' into rf/split_workflow

c8853ef

effigies force-pushed the rf/split_workflow branch from 22f236d to c8853ef Compare June 19, 2023 19:09

effigies changed the base branch from master to next June 19, 2023 19:09

effigies merged commit c8853ef into nipreps:next Jun 19, 2023
16 checks passed

effigies deleted the rf/split_workflow branch June 19, 2023 19:42

effigies mentioned this pull request Jul 12, 2023

Split into separate fit & apply workflows nipreps/fmriprep#3058

Closed

effigies added the next label Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RF: Split anatomical workflow into fit and derivatives workflows #303

RF: Split anatomical workflow into fit and derivatives workflows #303

effigies commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 •

edited

effigies commented Nov 15, 2022

effigies left a comment

effigies Nov 29, 2022

effigies Nov 29, 2022

effigies Nov 29, 2022

effigies Nov 29, 2022

effigies Nov 29, 2022

effigies Nov 29, 2022

effigies Nov 29, 2022

effigies Nov 29, 2022

effigies commented Jun 19, 2023

		@@ -50,81 +51,6 @@ def get_outputnode_spec():
		return fields


		def predict_derivatives(subject_id, output_spaces, freesurfer):

		@@ -655,12 +932,104 @@ def init_anat_derivatives_wf(
		return workflow


		def init_template_iterator_wf(*, spaces, name="template_iterator_wf"):

RF: Split anatomical workflow into fit and derivatives workflows #303

RF: Split anatomical workflow into fit and derivatives workflows #303

Conversation

effigies commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 • edited

Codecov Report

effigies commented Nov 15, 2022

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

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

Choose a reason for hiding this comment

effigies commented Jun 19, 2023

codecov-commenter commented Sep 30, 2022 •

edited