FIX: Re-add cohort identifier to template name

[mgxd]

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]

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 ⚠️

Additional details and impacted files

@@             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	Coverage Δ
ds005	∅ <ø> (?)
ds054	45.97% <11.53%> (?)
pytest	68.16% <91.30%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[oesteban]

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]

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]

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

[oesteban]

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]

Mostly LGTM.

[effigies]

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

[mgxd]

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]

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

[mgxd]

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