Skip to content

Conversation

tsalo
Copy link
Collaborator

@tsalo tsalo commented Jul 31, 2025

Closes #3433.

Changes proposed in this pull request

  • Collect cortex mask GIFTIs from anat_fit_wf instead of hcp_morphometrics_wf.

Copy link

codecov bot commented Jul 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.31%. Comparing base (c439ea6) to head (0d9c10e).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3491      +/-   ##
==========================================
- Coverage   73.50%   73.31%   -0.20%     
==========================================
  Files          60       60              
  Lines        4582     4549      -33     
  Branches      585      486      -99     
==========================================
- Hits         3368     3335      -33     
- Misses       1085     1087       +2     
+ Partials      129      127       -2     

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

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

If this is a necessary output, should we move this calculation into anat_fit_wf, look for it in precomputed derivatives, and pass it to hcp_morphometrics_wf?

Co-authored-by: Chris Markiewicz <[email protected]>
@tsalo
Copy link
Collaborator Author

tsalo commented Jul 31, 2025

I think moving it into anat_fit_wf would involve duplicating a lot of stuff from init_hcp_morphometrics_wf. Unfortunately, the cortex mask from that seems to be the last thing that gets calculated, so it might not be useful as an input. It would be useful for the BOLD fsLR projection workflow though.

@effigies
Copy link
Member

I think the only necessary duplication is abs(thickness), since the ROI stuff is otherwise not used by hcp_morphometrics_wf.

@tsalo
Copy link
Collaborator Author

tsalo commented Jul 31, 2025

I opened a PR to sMRIPrep. I can change this PR to use the output of anat_fit_wf once we're happy with that one.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 7, 2025

The current error is

File "/opt/conda/envs/fmriprep/lib/python3.12/site-packages/niworkflows/interfaces/nitransforms.py", line 81, in _run_interface
  concatenate_xfms(
File "/opt/conda/envs/fmriprep/lib/python3.12/site-packages/niworkflows/interfaces/nitransforms.py", line 98, in concatenate_xfms
  [load_affine(f, fmt=XFM_FMT[Path(f).suffix]) for f in in_files]
                      ~~~~~~~^^^^^^^^^^^^^^^^
KeyError: '.h5'

It looks like the list of supported transform extensions in niworkflows' dictionary (https://github.com/nipreps/niworkflows/blob/551b50f8272f2172084ce6ccd8964c86b93c4282/niworkflows/interfaces/nitransforms.py#L37-L42) doesn't include .h5. I'm not sure why this didn't come up in other PRs.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 7, 2025

Checking the failing node's report.rst, it looks like the second input to concat_xfm should be fsnative2t1w_xfm, but is instead '/deriv/anat/sub-01/anat/sub-01_from-MNI152NLin2009cAsym_to-T1w_mode-image_xfm.h5'. I think the fsnative transform that should be collected by sMRIPrep isn't the right one.

If I'm right, there must be a bug in sMRIPrep's collect_derivatives function.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 7, 2025

I just checked with the container. Running smriprep.utils.bids.collect_derivatives with the anat derivatives dataset in the ds005 test, I get the following.

With spaces=['MNI152NLin6Asym', 'fsnative'], I get:

{'curv': ['/deriv/anat/sub-01/anat/sub-01_hemi-L_curv.shape.gii',
          '/deriv/anat/sub-01/anat/sub-01_hemi-R_curv.shape.gii'],
 'midthickness': ['/deriv/anat/sub-01/anat/sub-01_hemi-L_midthickness.surf.gii',
                  '/deriv/anat/sub-01/anat/sub-01_hemi-R_midthickness.surf.gii'],
 'pial': ['/deriv/anat/sub-01/anat/sub-01_hemi-L_pial.surf.gii',
          '/deriv/anat/sub-01/anat/sub-01_hemi-R_pial.surf.gii'],
 'sulc': ['/deriv/anat/sub-01/anat/sub-01_hemi-L_sulc.shape.gii',
          '/deriv/anat/sub-01/anat/sub-01_hemi-R_sulc.shape.gii'],
 't1w_dseg': '/deriv/anat/sub-01/anat/sub-01_dseg.nii.gz',
 't1w_mask': '/deriv/anat/sub-01/anat/sub-01_desc-brain_mask.nii.gz',
 't1w_preproc': '/deriv/anat/sub-01/anat/sub-01_desc-preproc_T1w.nii.gz',
 't1w_tpms': ['/deriv/anat/sub-01/anat/sub-01_label-GM_probseg.nii.gz',
              '/deriv/anat/sub-01/anat/sub-01_label-WM_probseg.nii.gz',
              '/deriv/anat/sub-01/anat/sub-01_label-CSF_probseg.nii.gz'],
 'thickness': ['/deriv/anat/sub-01/anat/sub-01_hemi-L_thickness.shape.gii',
               '/deriv/anat/sub-01/anat/sub-01_hemi-R_thickness.shape.gii'],
 'transforms': {'MNI152NLin6Asym': {'forward': '/deriv/anat/sub-01/anat/sub-01_from-T1w_to-MNI152NLin6Asym_mode-image_xfm.h5',
                                    'reverse': '/deriv/anat/sub-01/anat/sub-01_from-MNI152NLin6Asym_to-T1w_mode-image_xfm.h5'},
                'fsnative': {'forward': '/deriv/anat/sub-01/anat/sub-01_from-T1w_to-MNI152NLin6Asym_mode-image_xfm.h5',
                             'reverse': '/deriv/anat/sub-01/anat/sub-01_from-MNI152NLin6Asym_to-T1w_mode-image_xfm.h5'}}}

Note the fsnative transforms are NLin6Asym!

If I use spaces=['MNI152NLin2009cAsym', 'fsnative'], I get:

...
 'transforms': {'MNI152NLin2009cAsym': {'forward': '/deriv/anat/sub-01/anat/sub-01_from-T1w_to-MNI152NLin2009cAsym_mode-image_xfm.h5',
                                        'reverse': '/deriv/anat/sub-01/anat/sub-01_from-MNI152NLin2009cAsym_to-T1w_mode-image_xfm.h5'},
                'fsnative': {'forward': '/deriv/anat/sub-01/anat/sub-01_from-T1w_to-MNI152NLin2009cAsym_mode-image_xfm.h5',
                             'reverse': '/deriv/anat/sub-01/anat/sub-01_from-MNI152NLin2009cAsym_to-T1w_mode-image_xfm.h5'}}}

The fsnative transform list is just the other space's transform list.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 7, 2025

Since I switched to using smriprep@master in this PR and other PRs are not affected, something merged after 0.18.0 must be the culprit.

EDIT: Maybe nipreps/smriprep@7460c38?

@effigies
Copy link
Member

effigies commented Aug 7, 2025

Can you write a quick script to test? If so, we could bisect it.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 7, 2025

Okay last comment on this. I ran a check using Python 3.12, and spec is being updated in place in collect_derivatives:

from pprint import pprint
from json import loads
from pathlib import Path

spec, _ = tuple(loads(Path('/Users/taylor/Documents/tsalo/smriprep/src/smriprep/data/io_spec.json').read_text()).values())
qry_base = {'subject': '01'}

pprint(spec['transforms'])

std_spaces = ['MNI152NLin2009cAsym', 'fsnative', 'fsLR']
derivs_cache = {}
transforms = derivs_cache.setdefault('transforms', {})
for _space in std_spaces:
    space = _space.replace(':cohort-', '+')
    print(space)
    for key, qry in spec['transforms'].items():
        # For a quick and dirty workaround, adding qry = qry.copy() here fixes the issue.
        pprint(qry)
        qry |= qry_base
        qry['from'] = qry['from'] or space
        qry['to'] = qry['to'] or space
        pprint(qry)

pprint(spec['transforms'])

In spec['transforms'], the first space is present in the query after it's updated.

EDIT: lol perfect timing @effigies

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 7, 2025

I opened nipreps/smriprep#486 with a proper script that can be bisected (I think).

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 7, 2025

It's finally running all the way through, but there's a mismatch in the expected outputs:

> sub-01/anat/sub-01_hemi-R_desc-cortex_mask.json         # my bad- I'll add this
> sub-01/anat/sub-01_hemi-R_desc-cortex_mask.label.gii    # my bad- I'll add this
> sub-01/anat/sub-01_hemi-R_desc-preproc_sphere.surf.gii  # ???
> sub-01/anat/sub-01_hemi-R_desc-preproc_white.surf.gii   # ???
< sub-01/anat/sub-01_hemi-R_sphere.surf.gii
< sub-01/anat/sub-01_hemi-R_sulc.shape.gii
< sub-01/anat/sub-01_hemi-R_thickness.shape.gii
< sub-01/anat/sub-01_hemi-R_white.surf.gii
< sub-01/anat/sub-01_label-CSF_probseg.nii.gz
< sub-01/anat/sub-01_label-GM_probseg.nii.gz
< sub-01/anat/sub-01_label-WM_probseg.nii.gz

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM.

@effigies effigies marked this pull request as ready for review August 8, 2025 13:30
@tsalo
Copy link
Collaborator Author

tsalo commented Aug 8, 2025

@effigies what about the weird sMRIPrep outputs? I know this PR isn't responsible for them, but they'll be an issue at some point.

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 8, 2025

Oh never mind it's passing now 😕

@effigies
Copy link
Member

effigies commented Aug 8, 2025

The desc-preproc ones? They're because sub-01_desc-preproc_T1w.nii.gz found in derivatives is the source T1w image. I think it derives from being able to tolerate an absent raw T1w image if the preprocessed T1w image is found, but I haven't explored it much.

You're welcome to dig into it.

@effigies effigies merged commit c961b3c into nipreps:master Aug 8, 2025
20 of 21 checks passed
@tsalo tsalo deleted the cortex-mask branch August 8, 2025 13:44
tsalo added a commit to PennLINC/aslprep that referenced this pull request Sep 16, 2025
effigies added a commit that referenced this pull request Oct 1, 2025
25.2.0 (October 01, 2025)

New feature release in the 25.2.x series.

This release is an *fMRIPrep Long-Term Support (LTS)* release.
The planned support window is 4 years, until October 2029.

This release is an incremental improvement over 25.1.x, but includes some fixes
and improvements that do not fit within our
`bug-fix policy <https://www.nipreps.org/devs/releases/#bug-fix-releases>`__.

Importantly, the change in interpolation in 25.1.0 introduced artifacts for some datasets.
This release changes the default interpolation mode to ``grid-constant``, which
resolves those problems while not reintroducing the issue the previous release sought to fix.

This release also (finally) introduces per-session processing.
The ``--session-label`` flag selects the sessions to process,
and the ``--subject-anatomical-reference`` flag indicates whether and how
to combine across sessions.
Existing filters passed via ``--bids-filter-file`` may need to be updated or
removed in favor of using these flags to achieve the desired behavior.

We would like to thank the AMP-SCZ and ENIGMA consortia for testing out and providing
feedback on this release.

  * FIX: Clean up output report language (#3529)
  * FIX: Default to grid-constant interpolation mode (#3516)
  * FIX: Adapt to transposed ndcoords in nitransforms (#3517)
  * FIX: Write out Freesurfer-derived outputs (#3512)
  * FIX: Add kwargs to _warnings.py (#3483)
  * ENH: Resample BOLD data to any surface template space using the Connectome Workbench (#3461)
  * ENH: Add boldref / sbref to source metadata (#3532)
  * ENH: Add dedicated session filtering, alternative anatomical template options (#3495)
  * ENH: Write out goodvoxels mask (#3513)
  * ENH: Add registration metadata to boldref-to-anat transforms (#3500)
  * ENH: Write out cortex mask GIFTIs (#3491)
  * ENH: Update transforms.py according to new transform chain of nitransforms (#3494)
  * RF/DOC: Improve and document command-line parser defaults (#3487)
  * DOC: Explain better SDC and B0FieldSource requirement (#2768)
  * DOC: Document `freesurfer` parameter in BOLD confound workflow init (#3504)
  * DOC: Add myself to contributor list (#3506)
  * DOC: Fix non-standard Input/Output docstring section management (#3505)
  * MNT: Split Dockerfile into base and pixi layers (#3521)
  * MNT: Replace conda with pixi and lock (#3503)
  * MNT: Update license metadata using SPDX expression (#3486)
  * MNT: no need to re-run `ruff check` after `ruff format` (#3480)
  * MNT: Update pre-commit ruff legacy alias (#3479)
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.

Write out cortex mask used for CIFTI creation

2 participants