Skip to content

Conversation

@tsalo
Copy link
Member

@tsalo tsalo commented May 21, 2025

Closes none, but addresses a bug detected by @effigies.

In minimal mode, ASLPrep runs the CBF workflow, but doesn't currently write out the CBF derivatives. This is a bug. ASLPrep should write out aslref-space CBF derivatives no matter what, since they're the main output of interest.

Changes proposed in this pull request

  • Modify init_ds_asl_native_wf in the following ways:
    • Stop writing out the aslref-space ASL brain mask. This is handled by init_asl_fit_wf.
    • Write out CBF and ATT derivatives by default. Previously they were only written out if the parameter asl_output was True. Now, that parameter only determines if the aslref ASL file is written out.
  • Move init_ds_asl_native_wf to before the workflow is returned when minimal mode is enabled.
  • Call init_ds_asl_native_wf if aslref outputs are requested and full mode is enabled or minimal/resampling mode is enabled. This way, if you're running in full mode and you don't want aslref outputs, you don't get them. However, if you're running in minimal or resampling mode, you get out aslref CBF and ATT outputs no matter what.

@tsalo tsalo added the bug Something isn't working label May 21, 2025
@effigies
Copy link
Contributor

Does this include reports? Generally, the more reports you can generate without rerunning, the better.

@tsalo
Copy link
Member Author

tsalo commented May 21, 2025

I currently have CBF reports and confounds as part of the "resampling" level. If I move those up to "minimal" I don't think I'll have anything that's specific to "resampling". Happy to do it, but not sure what resampling should be used for.

@effigies
Copy link
Contributor

I pretty much only have "resampling" there for tedana users or other experimenters. Confounds are in full, for fmriprep.

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.18%. Comparing base (1729618) to head (72c815c).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #522      +/-   ##
==========================================
- Coverage   83.24%   83.18%   -0.07%     
==========================================
  Files          38       38              
  Lines        4101     4086      -15     
==========================================
- Hits         3414     3399      -15     
  Misses        687      687              

☔ 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.

@tsalo
Copy link
Member Author

tsalo commented May 22, 2025

That works for me. I'll make a note that "resampling" is the same as "minimal" (for now) in the documentation and I'll move the report generation to the minimal level.

@tsalo tsalo requested a review from effigies May 22, 2025 17:22
return workflow
# If we want aslref-space outputs, then call the appropriate workflow
aslref_out = bool(nonstd_spaces.intersection(('func', 'run', 'asl', 'aslref', 'sbref')))
aslref_out &= config.workflow.level == 'full'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this logical and...

@tsalo tsalo merged commit da4c844 into main May 22, 2025
26 checks passed
@tsalo tsalo deleted the minimal-cbf branch May 22, 2025 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants