From 994ac33a1eec034d439dfe80fe97a3183b4e6ee4 Mon Sep 17 00:00:00 2001 From: mathiasg Date: Thu, 3 Sep 2026 15:32:45 -0400 Subject: [PATCH 1/2] fix: honor sessions when finding estimators ``find_estimators`` discarded the caller's ``sessions`` whenever any ``bids_filters`` were given, because the session key was popped unconditionally and defaulted to None. --- sdcflows/utils/tests/test_wrangler.py | 74 +++++++++++++++++++++++++++ sdcflows/utils/wrangler.py | 34 +++++++----- 2 files changed, 96 insertions(+), 12 deletions(-) diff --git a/sdcflows/utils/tests/test_wrangler.py b/sdcflows/utils/tests/test_wrangler.py index d933625eca..dd48ac49e8 100644 --- a/sdcflows/utils/tests/test_wrangler.py +++ b/sdcflows/utils/tests/test_wrangler.py @@ -433,6 +433,47 @@ def gen_layout(bids_dir, database_dir=None): } +phasediff_b0ids = { + '01': [ + { + 'session': session, + 'anat': [{'suffix': 'T1w', 'metadata': {'EchoTime': 1}}], + 'fmap': [ + { + 'suffix': 'phasediff', + 'metadata': { + 'EchoTime1': 1.2, + 'EchoTime2': 1.4, + 'B0FieldIdentifier': f'gre{session}', + }, + }, + { + 'suffix': 'magnitude1', + 'metadata': {'EchoTime': 1.2, 'B0FieldIdentifier': f'gre{session}'}, + }, + { + 'suffix': 'magnitude2', + 'metadata': {'EchoTime': 1.4, 'B0FieldIdentifier': f'gre{session}'}, + }, + ], + 'func': [ + { + 'task': 'rest', + 'suffix': 'bold', + 'metadata': { + 'RepetitionTime': 0.8, + 'TotalReadoutTime': 0.5, + 'PhaseEncodingDirection': 'j', + 'B0FieldSource': f'gre{session}', + }, + } + ], + } + for session in ('01', '02') + ] +} + + filters = { 'fmap': { 'datatype': 'fmap', @@ -496,6 +537,39 @@ def test_wrangler_URIs(tmpdir, name, skeleton, session, estimations, total_estim clear_registry() +@pytest.mark.parametrize('bids_filters', [None, {'datatype': 'fmap'}]) +def test_sessionwise_queries(tmp_path, bids_filters): + """One query per session must find that session's fieldmap.""" + bids_dir = tmp_path / 'bids' + generate_bids_skeleton(bids_dir, phasediff) + layout = gen_layout(bids_dir) + + for session in ('01', '02', '03'): + est = find_estimators( + layout=layout, + subject='01', + sessions=[session], + bids_filters=bids_filters, + ) + assert len(est) == 1 + assert all(f'ses-{session}' in str(source.path) for source in est[0].sources) + + clear_registry() + + +def test_sessionwise_queries_b0ids(tmp_path): + """Session-specific ``B0FieldIdentifier``s are not leaked across queries.""" + bids_dir = tmp_path / 'bids' + generate_bids_skeleton(bids_dir, phasediff_b0ids) + layout = gen_layout(bids_dir) + + for session in ('01', '02'): + est = find_estimators(layout=layout, subject='01', sessions=[session]) + assert [estimator.bids_id for estimator in est] == [f'gre{session}'] + + clear_registry() + + def test_single_reverse_pedir(tmp_path): bids_dir = tmp_path / 'bids' generate_bids_skeleton(bids_dir, pepolar) diff --git a/sdcflows/utils/wrangler.py b/sdcflows/utils/wrangler.py index f22725167f..7a2b194469 100644 --- a/sdcflows/utils/wrangler.py +++ b/sdcflows/utils/wrangler.py @@ -332,9 +332,10 @@ def find_estimators( if bids_filters: filters = bids_filters.copy() # copy to avoid altering in place - if 'session' in bids_filters and sessions is not None: - raise ValueError('Filters include session, but session is already defined.') - sessions = listify(filters.pop('session', None)) + if 'session' in filters: + if sessions is not None: + raise ValueError('Filters include session, but session is already defined.') + sessions = listify(filters.pop('session')) base_entities.update(filters) subject_root = Path(layout.root) / f'sub-{subject}' @@ -346,12 +347,13 @@ def find_estimators( estimators = [] # Step 1. Use B0FieldIdentifier metadata + b0_entities = {**base_entities, 'session': sessions} b0_ids = () with suppress(BIDSEntityError): # flatten lists from json (tupled in pybids for hashing), then unique b0_ids = reduce( set.union, - (listify(ids) for ids in layout.get_B0FieldIdentifiers(**base_entities)), + (listify(ids) for ids in layout.get_B0FieldIdentifiers(**b0_entities)), set(), ) @@ -363,12 +365,9 @@ def find_estimators( for b0_id in b0_ids: # Found B0FieldIdentifier metadata entries - b0_entities = base_entities.copy() - b0_entities['B0FieldIdentifier'] = b0_id - - bare_ids = layout.get(**base_entities, B0FieldIdentifier=b0_id) + bare_ids = layout.get(**b0_entities, B0FieldIdentifier=b0_id) listed_ids = layout.get( - **base_entities, + **b0_entities, B0FieldIdentifier=f'"{b0_id}"', # Double quotes to match JSON, not Python repr regex_search=True, ) @@ -382,7 +381,12 @@ def find_estimators( ) except (ValueError, TypeError) as err: _log_debug_estimator_fail( - logger, b0_id, bare_ids + listed_ids, layout.root, str(err) + logger, + b0_id, + bare_ids + listed_ids, + layout.root, + str(err), + level=logging.WARNING, ) else: _log_debug_estimation(logger, e, layout.root) @@ -645,10 +649,16 @@ def _log_debug_estimation( def _log_debug_estimator_fail( - logger: logging.Logger, b0_id: str, files: list[BIDSFile], bids_root: str, message: str + logger: logging.Logger, + b0_id: str, + files: list[BIDSFile], + bids_root: str, + message: str, + level: int = logging.DEBUG, ) -> None: """A helper function to log failures to build an estimator when running with verbosity.""" - logger.debug( + logger.log( + level, 'Failed to construct %s estimation from %d sources:\n- %s\nError: %s', b0_id, len(files), From 00213995117eb0c3cc0ef0782eeacbbb444b18ee Mon Sep 17 00:00:00 2001 From: mathiasg Date: Tue, 15 Sep 2026 09:03:15 -0400 Subject: [PATCH 2/2] fix: b0fieldidentifier are participant-unique --- sdcflows/utils/tests/test_wrangler.py | 54 --------------------------- sdcflows/utils/wrangler.py | 7 ++-- 2 files changed, 3 insertions(+), 58 deletions(-) diff --git a/sdcflows/utils/tests/test_wrangler.py b/sdcflows/utils/tests/test_wrangler.py index dd48ac49e8..c0a395063f 100644 --- a/sdcflows/utils/tests/test_wrangler.py +++ b/sdcflows/utils/tests/test_wrangler.py @@ -433,47 +433,6 @@ def gen_layout(bids_dir, database_dir=None): } -phasediff_b0ids = { - '01': [ - { - 'session': session, - 'anat': [{'suffix': 'T1w', 'metadata': {'EchoTime': 1}}], - 'fmap': [ - { - 'suffix': 'phasediff', - 'metadata': { - 'EchoTime1': 1.2, - 'EchoTime2': 1.4, - 'B0FieldIdentifier': f'gre{session}', - }, - }, - { - 'suffix': 'magnitude1', - 'metadata': {'EchoTime': 1.2, 'B0FieldIdentifier': f'gre{session}'}, - }, - { - 'suffix': 'magnitude2', - 'metadata': {'EchoTime': 1.4, 'B0FieldIdentifier': f'gre{session}'}, - }, - ], - 'func': [ - { - 'task': 'rest', - 'suffix': 'bold', - 'metadata': { - 'RepetitionTime': 0.8, - 'TotalReadoutTime': 0.5, - 'PhaseEncodingDirection': 'j', - 'B0FieldSource': f'gre{session}', - }, - } - ], - } - for session in ('01', '02') - ] -} - - filters = { 'fmap': { 'datatype': 'fmap', @@ -557,19 +516,6 @@ def test_sessionwise_queries(tmp_path, bids_filters): clear_registry() -def test_sessionwise_queries_b0ids(tmp_path): - """Session-specific ``B0FieldIdentifier``s are not leaked across queries.""" - bids_dir = tmp_path / 'bids' - generate_bids_skeleton(bids_dir, phasediff_b0ids) - layout = gen_layout(bids_dir) - - for session in ('01', '02'): - est = find_estimators(layout=layout, subject='01', sessions=[session]) - assert [estimator.bids_id for estimator in est] == [f'gre{session}'] - - clear_registry() - - def test_single_reverse_pedir(tmp_path): bids_dir = tmp_path / 'bids' generate_bids_skeleton(bids_dir, pepolar) diff --git a/sdcflows/utils/wrangler.py b/sdcflows/utils/wrangler.py index 7a2b194469..63e06691dc 100644 --- a/sdcflows/utils/wrangler.py +++ b/sdcflows/utils/wrangler.py @@ -347,13 +347,12 @@ def find_estimators( estimators = [] # Step 1. Use B0FieldIdentifier metadata - b0_entities = {**base_entities, 'session': sessions} b0_ids = () with suppress(BIDSEntityError): # flatten lists from json (tupled in pybids for hashing), then unique b0_ids = reduce( set.union, - (listify(ids) for ids in layout.get_B0FieldIdentifiers(**b0_entities)), + (listify(ids) for ids in layout.get_B0FieldIdentifiers(**base_entities)), set(), ) @@ -365,9 +364,9 @@ def find_estimators( for b0_id in b0_ids: # Found B0FieldIdentifier metadata entries - bare_ids = layout.get(**b0_entities, B0FieldIdentifier=b0_id) + bare_ids = layout.get(**base_entities, B0FieldIdentifier=b0_id) listed_ids = layout.get( - **b0_entities, + **base_entities, B0FieldIdentifier=f'"{b0_id}"', # Double quotes to match JSON, not Python repr regex_search=True, )