Fix SBRef sorting when EchoTime is missing

[2593803287-hue]

Closes #3646.

Summary

• Handle SBRef files that are missing EchoTime metadata when sorting matches.
• Keep SBRefs with EchoTime sorted by echo time and fall back to filename for missing values.
• Add a regression test covering multiple SBRefs with missing EchoTime metadata.

Test plan

• python3 -B -m pytest fmriprep/workflows/bold/tests/test_fit.py::test_get_sbrefs_handles_missing_echo_time

[effigies]

Ah, thanks for tracking down the root cause here. I'm not sure that lexical sorting is the best option. The reason we use echotime is that we've run across datasets where they don't match.

The simple thing would be to reject sbrefs with no echo time, giving a clear warning that this was being done.

The more complex but probably okay thing would be to infer the echo time from the corresponding bold files.

I lean toward rejection, as there's no danger of making a mistake.

[tsalo]

EchoTime is required for files with the echo entity in the filename, so I also lean toward rejection.

[effigies]

So this is probably the time we should finally upgrade the validator...

[2593803287-hue]

Addressed the review feedback.

• get_sbrefs() now drops SBRefs that are missing EchoTime metadata and emits a warning instead of falling back to filename ordering.
• Updated the regression test to cover the new drop+warning behavior.

Local verification:

• python3 -B -m pytest fmriprep/workflows/bold/tests/test_fit.py -k 'rejects_missing_echo_time' -q
• python3 -B -m pytest fmriprep/workflows/bold/tests/test_fit.py::test_bold_fit_precomputes -q

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.54%. Comparing base (69ca63c) to head (abab9b9).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3648      +/-   ##
==========================================
+ Coverage   74.41%   74.54%   +0.12%     
==========================================
  Files          62       62              
  Lines        4987     5012      +25     
  Branches      637      639       +2     
==========================================
+ Hits         3711     3736      +25     
  Misses       1142     1142              
  Partials      134      134              

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

[effigies]

LGTM, thanks!

[effigies]

Actually, I think this will break in the case of a single sbref that's missing an echo time. Can you add a test for that case? That can probably just be handled by checking len(sbref_files) == 1. (sorted() handled the 0- and 1-file cases by not having any comparisons to make.)

[2593803287-hue]

I think there’s still one edge case missing here: a single SBRef with no EchoTime.

Right now the new logic drops any SBRef missing EchoTime, so if layout.get(...) returns exactly one SBRef and its metadata is missing EchoTime, this would return []. Before this change, sorted() handled the 0- and 1-file cases naturally because there were no comparisons to make.

Could you add a regression test for that case? I think the implementation can probably just special-case it with something like if len(sbref_files) == 1: return sbref_files before the filtering/sorting logic.

That would keep the multi-file ambiguity fix while preserving the previous behavior for the degenerate single-file case.

[2593803287-hue]

Addressed — I updated the PR to preserve the single-SBRef case and added a regression test for it.

Changes:

• Return early when len(sbref_files) == 1
• Add a test covering a single SBRef with missing EchoTime

This keeps the multi-file missing-EchoTime filtering behavior while preserving the old 0/1-file behavior.