qsiprep icon indicating copy to clipboard operation
qsiprep copied to clipboard

Allow configuration of `--no-csf` in SynthStrip

Open psadil opened this issue 2 months ago • 12 comments

Changes proposed in this pull request

Allow configuration of the --no-csf option in the SynthStrip workflow ~~and by default set it to True (currently implies False)~~. Keep the currently implied default (False).

Closes #954

Documentation that should be reviewed


[!NOTE] Adds a --no-csf CLI flag and workflow.no_csf config, plumbing the option into SynthStrip across anatomical, DWI, and fieldmap workflows.

  • CLI:
    • Import BooleanOptionalAction and add --no-csf flag in qsiprep/cli/parser.py to control SynthStrip CSF removal.
  • Config:
    • Introduce workflow.no_csf boolean in qsiprep/config.py.
  • Workflows:
    • Update init_synthstrip_wf to accept no_csf and pass it to FixHeaderSynthStrip/MockSynthStrip.
    • Propagate no_csf=config.workflow.no_csf in:
      • Anatomical volume workflows (init_anat_preproc_wf, T2w preprocessing).
      • DWI reference workflow (init_dwi_reference_wf).
      • PEPOLAR fieldmap workflow (init_extended_pepolar_report_wf).

Written by Cursor Bugbot for commit 93e66300baba920a7bf64d39781b4ce19f351c6c. This will update automatically on new commits. Configure here.

psadil avatar Nov 12 '25 22:11 psadil

Let me know if this should be configurable from the CLI

psadil avatar Nov 12 '25 22:11 psadil

Codecov Report

:x: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 46.18%. Comparing base (69be226) to head (5a443a6). :warning: Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
qsiprep/workflows/anatomical/volume.py 75.00% 1 Missing and 1 partial :warning:
qsiprep/workflows/fieldmap/pepolar.py 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1000      +/-   ##
==========================================
+ Coverage   46.15%   46.18%   +0.02%     
==========================================
  Files          65       65              
  Lines        9793     9802       +9     
  Branches     1083     1084       +1     
==========================================
+ Hits         4520     4527       +7     
- Misses       5045     5046       +1     
- Partials      228      229       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov-commenter avatar Nov 12 '25 23:11 codecov-commenter

My intuition is that excluding CSF should be a straightforward improvement across the board, so it shouldn't need to be configurable, but I'll defer to @mattcieslak.

tsalo avatar Nov 13 '25 14:11 tsalo

I think we should keep it as an option because I'm not sure how it will interact with MSMT. If CSF is one of the tissues it needs to estimate, maybe it's useful to have some extra CSF in the brain mask? We should ask @36000 or @arokem if this is important. Many other models break down in the csf and we end up with implausible values, so keeping CSF out of the mask would be great

mattcieslak avatar Nov 13 '25 14:11 mattcieslak

If we need CSF in the brain mask for reconstruction, but it's causing problems for registration, then maybe we could produce two masks? fMRIPrep produces two boldrefs- one for motion correction and one for coregistration- so there's a precedent for that.

tsalo avatar Nov 13 '25 14:11 tsalo

@psadil I think we can try out creating two masks in a follow-up PR before the next release, if experts (@36000, @arokem) think CSF is useful for other steps and/or reconstruction, so just making no-csf configurable in the CLI should be sufficient for this PR.

tsalo avatar Nov 13 '25 14:11 tsalo

To fit MSMT, you do need to estimate response functions, which is usually done by looking at ROIs in the "pure" WM, GM, CSF. So this would break, but I think we can work around that by making our own brain mask internally that includes the CSF for just these calculations. I think pyAFQ would be fine with having no CSF in the brain mask. By default, we do not use MSMT. I can't speak for how other software does this though.

However, I still have two questions: (1) how reliable is this method? I would guess that removing the CSF is much trickier than doing a generic brain mask and (2) would it not be easier for readability and future software to have separate brain masks and CSF masks? Then QSIprep could pass the brain+CSF mask to certain pipelines and only the brain mask to other pipelines.

36000 avatar Nov 13 '25 16:11 36000

However, I still have two questions: (1) how reliable is this method? I would guess that removing the CSF is much trickier than doing a generic brain mask and (2) would it not be easier for readability and future software to have separate brain masks and CSF masks? Then QSIprep could pass the brain+CSF mask to certain pipelines and only the brain mask to other pipelines.

  1. I haven't see direct measurements of SynthStrip as a way to generate brain masks specifically (that is, with --no-csf), only those that apply to it's default behavior as a skull-stripping tool.

  2. Happy to implement whatever's wanted.

psadil avatar Nov 13 '25 18:11 psadil

MRtrix workflows that do MSMT might break if you do not have a CSF-inclusive mask.

araikes avatar Nov 17 '25 20:11 araikes

It sounds like having two brain masks- one for coregistration and one for steps like MSMT- might be the way to go. My initial idea that it would be fine to address that in a separate PR might have been too hasty.

tsalo avatar Nov 19 '25 14:11 tsalo

Are there preferences or opinions about how to name the masks? Currently, there is the single *desc-brain_mask.nii.gz. I assume that should be kept (and should refer to the same kind of mask currently produced, w/ CSF). For the other, how about something like *desc-brainnocsf_mask.nii.gz ? Not sure if it would be better to specify how this one is used.

psadil avatar Dec 03 '25 23:12 psadil

@tsalo , @mattcieslak , sorry, but I'm hitting a small snag. I only just realized that the version of mri_synthstrip bundled into the qsiprep_build image doesn't have the --no-csf option. It was introduced in mri_synthstrip 1.2. What would be the preferred way to handle that? Options I see include (ordered roughly from smallest to largest change)

psadil avatar Dec 05 '25 03:12 psadil