Allow configuration of `--no-csf` in SynthStrip
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-csfCLI flag andworkflow.no_csfconfig, plumbing the option into SynthStrip across anatomical, DWI, and fieldmap workflows.
- CLI:
- Import
BooleanOptionalActionand add--no-csfflag inqsiprep/cli/parser.pyto control SynthStrip CSF removal.- Config:
- Introduce
workflow.no_csfboolean inqsiprep/config.py.- Workflows:
- Update
init_synthstrip_wfto acceptno_csfand pass it toFixHeaderSynthStrip/MockSynthStrip.- Propagate
no_csf=config.workflow.no_csfin:
- 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.
Let me know if this should be configurable from the CLI
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.
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.
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
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.
@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.
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.
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.
-
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. -
Happy to implement whatever's wanted.
MRtrix workflows that do MSMT might break if you do not have a CSF-inclusive mask.
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.
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.
@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)
- bundle the nocsf model
- have
qsiprep_buildpullmri_synthstripfrom the freesurfer/synthstrip image, instead of the freesurfer/freesurfer one - update the FreeSurfer version in
qsiprep_build- e.g., v7.3.2 would do it