FIX: Normalize BIDS-URIs to subject-relative

[effigies]

Fixes #456.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.72%. Comparing base (18e5e95) to head (f9ac03c).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   83.66%   83.72%   +0.05%     
==========================================
  Files          32       32              
  Lines        2816     2826      +10     
  Branches      377      380       +3     
==========================================
+ Hits         2356     2366      +10     
  Misses        390      390              
  Partials       70       70              

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

[effigies]

@mgxd @oesteban A review would be appreciated.

[oesteban]

It looks like we are reading the metadata too many times right?

sdcflows/sdcflows/utils/wrangler.py

Lines 427 to 432 in 08e198f

	has_intended = layout.get(
	**{
	**base_entities,
	**{'suffix': 'epi', 'IntendedFor': Query.REQUIRED, 'session': sessions}
	}
	)

Then in 445 we use the _resolve_intent() filter, which is similar

sdcflows/sdcflows/utils/wrangler.py

Line 445 in 08e198f

target = layout.get_file(_resolve_intent(intent, layout, subject))

Perhaps we should try to avoid the second metadata read and use this one at FieldmapFile instantiation? (meaning, your fix looks good but the old code remains a bit brittle)

[effigies]

We're not actually duplicating reads because of:

sdcflows/sdcflows/utils/wrangler.py

Lines 434 to 437 in 08e198f

	for epi_fmap in has_intended:
	if epi_fmap.path in fm._estimators.sources:
	logger.debug("Skipping fieldmap %s (already in use)", epi_fmap.relpath)
	continue # skip EPI images already considered above

Fieldmaps affected by this PR never reach the later code.

The issue here is that SDCflows will happily create an estimator for a "whole" fieldmap (doesn't need a bold/dwi/m0scan to compete it) that points to a B0FieldIdentifier, a subject-relative IntendedFor or a BIDS-URI. But when it's time to look up what estimator I should use for my BOLD file (get_identifier()), if I look for a subject-relative path, then I miss the BIDS-URIs.

So anyway, it could be that the better solution is for fMRIPrep to try bids::$DS_RELPATH along with $SUB_RELPATH, but when I started this it felt like SDCflows should expose the estimators the same way regardless.

[effigies]

@oesteban Just checking back in on this comment. It says outdated because we removed the unused layout parameter, but the structure didn't change.

[mgxd]

Minor comment but looks good to me - I believe I tried breaking up the logic in the wrangler module sometime ago but glancing over it again and it was not enough...

[effigies]

Going to merge, and we can address any brittleness in a future PR.