nipreps / smriprep

Structural MRI PREProcessing (sMRIPrep) workflows for NIPreps (NeuroImaging PREProcessing tools)
https://nipreps.github.io/smriprep
Apache License 2.0
134 stars 39 forks source link

FIX: Re-add cohort identifier to template name #416

Closed mgxd closed 8 months ago

mgxd commented 9 months ago

Fixes #415

We have been stripping out the cohort identifier in split_desc, and as a result losing the information when saving out transforms. This re-enables the output transforms to include the cohort information, i.e. ...from-MNIPediatricAsym+3_to-T1w_mode-image_xfm.h5.

However, this change exposes another problem: collect_derivatives cannot collect these transformations, as the current BIDSLayout configurations do not account for the + character during parsing. Since BIDS derivatives conventions are still a bit murky, I figured we want to just create a modified fmriprep-derivatives config to catch this convention when parsing.

codecov[bot] commented 9 months ago

Codecov Report

Attention: Patch coverage is 91.30435% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 78.53%. Comparing base (51da19b) to head (a624b2a).

Files Patch % Lines
smriprep/workflows/fit/registration.py 33.33% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #416 +/- ## =========================================== + Coverage 61.63% 78.53% +16.89% =========================================== Files 24 30 +6 Lines 1903 2073 +170 Branches 241 242 +1 =========================================== + Hits 1173 1628 +455 + Misses 702 413 -289 - Partials 28 32 +4 ``` | [Flag](https://app.codecov.io/gh/nipreps/smriprep/pull/416/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipreps) | Coverage Δ | | |---|---|---| | [ds005](https://app.codecov.io/gh/nipreps/smriprep/pull/416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipreps) | `∅ <ø> (?)` | | | [ds054](https://app.codecov.io/gh/nipreps/smriprep/pull/416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipreps) | `45.97% <11.53%> (?)` | | | [pytest](https://app.codecov.io/gh/nipreps/smriprep/pull/416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipreps) | `68.16% <91.30%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipreps#carryforward-flags-in-the-pull-request-comment) to find out more.

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

oesteban commented 9 months ago

Looks good in general. Perhaps relying on the + semantic is too risky. Since the proposal was done also including - as allowed (and I believe also underscore), it will not fly anytime soon. And would honestly not will to try to get only + accepted.

What do you think about from-MNIPediatricAsymCohort3 or, alternatively and potentially generating conflicts down the line, from-MNIPediatricAsym_to-T1w_desc-cohort3_xfm.h5?

mgxd commented 9 months ago

TBH I don't love either option - at first I figured we could just add the cohort- entity directly, but that will clobber cases of inter-cohort xfms. IMO the + seems to be the cleanest / least ambiguous...

mgxd commented 8 months ago

@effigies if you have a minute, would appreciate a review

oesteban commented 8 months ago

TBH I don't love either option - at first I figured we could just add the cohort- entity directly, but that will clobber cases of inter-cohort xfms. IMO the + seems to be the cleanest / least ambiguous...

In that case, I believe space- could be used: space-MNIPediatricAsym_from-cohort1_to-cohort2_xfm.h5, but this honestly feels like a templateflow's issue, not a plausible output of sMRIPrep

effigies commented 8 months ago

Is this considered a bugfix or a feature for versioning purposes?

mgxd commented 8 months ago

I consider it more of a bug fix to get cohort processing / --derivatives working, as the changes will really only affect those cases. But this will invalidate the cache starting from the fit.registration workflow so maybe worth a minor bump?

effigies commented 8 months ago

Either way. We can go ahead and jump to fmriprep 24.0 if needed.

mgxd commented 8 months ago

Given that we're almost in Q2 of 2024 let's cut 24.0