Skip to content

Conversation

tsalo
Copy link
Collaborator

@tsalo tsalo commented Jul 31, 2025

Based on @effigies' idea in nipreps/fmriprep#3491. This new output should make it possible to project data to the surface just using computed derivatives.

Changes proposed:

  • Move roi output out of init_hcp_morphometrics_wf.
  • Calculate that output in a new workflow, init_cortex_mask_wf.
  • Add step to init_anat_fit_wf that calls init_cortex_mask_wf.

@tsalo tsalo changed the title Generate cortex mask in anat_fit_wf. Generate cortex mask in anat_fit_wf Jul 31, 2025
Copy link

codecov bot commented Jul 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.21%. Comparing base (7679ad3) to head (04643a4).
⚠️ Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
+ Coverage   70.82%   75.21%   +4.39%     
==========================================
  Files          25       25              
  Lines        2029     2062      +33     
  Branches      264      266       +2     
==========================================
+ Hits         1437     1551     +114     
+ Misses        537      444      -93     
- Partials       55       67      +12     
Flag Coverage Δ
ds054 45.01% <ø> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@effigies
Copy link
Member

If you want to use the thickness/midthickness GIFTIs as the sources, you're going to need to reorder things so that instead of going generate_surfaces -> surfaces_buffer -> datasink_workflows you do generate_surfaces -> datasink_workflows -> surfaces_buffer.

@effigies
Copy link
Member

effigies commented Jul 31, 2025

e.g.,

-            (gifti_surfaces_wf, surfaces_buffer, [
-                (f'outputnode.{surf}', surf) for surf in surfs
-            ]),
             (sourcefile_buffer, ds_surfaces_wf, [('source_files', 'inputnode.source_files')]),
             (gifti_surfaces_wf, ds_surfaces_wf, [
                 (f'outputnode.{surf}', f'inputnode.{surf}') for surf in surfs
             ]),
+            (ds_surfaces_wf, surfaces_buffer, [
+                (f'outputnode.{surf}', surf) for surf in surfs
+            ]),
         ])
         # fmt:on

and

-            (gifti_morph_wf, surfaces_buffer, [
-                (f'outputnode.{metric}', metric) for metric in metrics
-            ]),
             (sourcefile_buffer, ds_morph_wf, [('source_files', 'inputnode.source_files')]),
             (gifti_morph_wf, ds_morph_wf, [
                 (f'outputnode.{metric}', f'inputnode.{metric}') for metric in metrics
             ]),
+            (ds_morph_wf, surfaces_buffer, [
+                (f'outputnode.{metric}', metric) for metric in metrics
+            ]),

tsalo and others added 5 commits August 1, 2025 09:00
I realize that using KeySelect is probably cleaner, but I am not very familiar with that approach.
@tsalo
Copy link
Collaborator Author

tsalo commented Aug 1, 2025

I opened nipreps/niworkflows#962 to add the filename pattern for this mask. I don't know if desc-roi_mask.label.gii is the best name though. It seems a little too generic, but I don't know what would be better. desc-cortex also seems too generic.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 1, 2025

I thought sMRIPrep used niworkflows@master, and I waited until niworkflows had deployed. Am I missing something?

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 4, 2025

Thanks for fixing that @effigies! I think this'll be good to go once CI passes, but I am still not sure desc-roi is the best name.

@tsalo tsalo changed the title Generate cortex mask in anat_fit_wf ENH: Generate cortex mask in anat_fit_wf Aug 4, 2025
@tsalo tsalo requested review from effigies and mgxd August 4, 2025 18:42
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.

personally i prefer desc-cortex over roi, since the sidecar metadata type is set to roi

@tsalo tsalo requested a review from mgxd August 5, 2025 15:29
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.

LGTM

mgxd

This comment was marked as duplicate.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 6, 2025

@effigies I think this is ready for one last review. Also, is there anyone else I should ping for a review?

@effigies
Copy link
Member

effigies commented Aug 6, 2025

@tsalo I wanted to suggest going back to iterables from explicit loops, but the changes would be too extensive to make a suggestion without testing. Please see the latest three commits and let me know if you're good with these changes.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 6, 2025

TBH I knew using iterables would be cleaner, but I never use them in my own code so I figured I'd mess it up. The only thing I want to check is that the Sources are correct in the output JSONs.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 6, 2025

@effigies thank you for refactoring the workflows. I looked at the desc-cortex_mask.json files in the ds005 derivatives and they looked good, so I think this is ready to merge.

@effigies effigies merged commit 29f0def into nipreps:master Aug 6, 2025
21 checks passed
@tsalo tsalo deleted the cortex-mask branch August 7, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants