From f9917405b5b2130939e41fa2407253b1d7b424d8 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Tue, 1 Oct 2024 22:30:51 -0400 Subject: [PATCH 01/15] update from entity to match value created by fmriprep --- fmriprep/data/io_spec.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fmriprep/data/io_spec.json b/fmriprep/data/io_spec.json index 5d90f184c..44b684563 100644 --- a/fmriprep/data/io_spec.json +++ b/fmriprep/data/io_spec.json @@ -41,7 +41,7 @@ }, "boldref2fmap": { "datatype": "func", - "from": "orig", + "from": "boldref", "mode": "image", "suffix": "xfm", "extension": ".txt" From dd198ad48e9a110e10b51425d5b1077aa39fad1d Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Tue, 1 Oct 2024 22:32:48 -0400 Subject: [PATCH 02/15] Repair search for precomputed transforms --- fmriprep/utils/bids.py | 22 ++++++++-- fmriprep/utils/tests/test_derivative_cache.py | 44 +++++++++++++++++++ 2 files changed, 62 insertions(+), 4 deletions(-) create mode 100644 fmriprep/utils/tests/test_derivative_cache.py diff --git a/fmriprep/utils/bids.py b/fmriprep/utils/bids.py index 0bdc03ff1..3f5a27a05 100644 --- a/fmriprep/utils/bids.py +++ b/fmriprep/utils/bids.py @@ -68,14 +68,28 @@ def collect_derivatives( continue derivs_cache[f'{k}_boldref'] = item[0] if len(item) == 1 else item + transforms_cache = {} for xfm, q in spec['transforms'].items(): - query = {**q, **entities} + # Transform extension will often not match provided entities + # (e.g., ".nii.gz" vs ".txt"). + # And transform suffixes will be "xfm", + # whereas relevant src file will be "bold". + query = { + **q, + **{ + k: v + for k, v in entities.items() + if k not in ['suffix', 'extension'] + }, + } if xfm == 'boldref2fmap': - query['to'] = fieldmap_id - item = layout.get(return_type='filename', **q) + # fieldmaps have ids like auto_00000 + query['to'] = fieldmap_id.replace('_', '') + item = layout.get(return_type='filename', **query) if not item: continue - derivs_cache[xfm] = item[0] if len(item) == 1 else item + transforms_cache[xfm] = item[0] if len(item) == 1 else item + derivs_cache['transforms'] = transforms_cache return derivs_cache diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py new file mode 100644 index 000000000..b69777205 --- /dev/null +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -0,0 +1,44 @@ +import json +from pathlib import Path + +import pytest + +from fmriprep.data import load as load_data +from fmriprep.utils import bids + + +@pytest.mark.parametrize('xfm', ['boldref2fmap', 'boldref2anat', 'hmc']) +def test_transforms_found_as_str(tmp_path: Path, xfm: str): + spec = ( + json.loads(load_data.readable('io_spec.json').read_text()) + .get('queries') + .get('transforms') + .get(xfm) + ) + entities = { + 'subject': '0', + 'task': 'rest', + 'suffix': 'bold', + 'extension': '.nii.gz', + } + if xfm == 'boldref2fmap': + to_find = f'sub-{entities['subject']}_task-{entities['task']} \ + _from-{spec['from']}_to-auto00000_mode-image_xfm.txt' + else: + to_find = f'sub-{entities['subject']}_task-{entities['task']} \ + _from-{spec['from']}_to-{spec['to']}_mode-image_xfm.txt' + + funcd = tmp_path / f'sub-{entities['subject']}' / 'func' + funcd.mkdir(parents=True) + (funcd / to_find).touch() + + derivs = bids.collect_derivatives( + derivatives_dir=tmp_path, + entities=entities, + fieldmap_id='auto_00000', + ) + transforms_in_derivs = 'transforms' in derivs + xfm_in_transforms = xfm in derivs.get('transforms') + transform_is_str = isinstance(derivs.get('transforms').get(xfm), str) + assert all((transforms_in_derivs, xfm_in_transforms, transform_is_str)) + From 69695ffd945ed4b82929a511aab79b48c8661965 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Tue, 1 Oct 2024 22:48:16 -0400 Subject: [PATCH 03/15] lint --- fmriprep/utils/bids.py | 6 +----- fmriprep/utils/tests/test_derivative_cache.py | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/fmriprep/utils/bids.py b/fmriprep/utils/bids.py index 3f5a27a05..64be39715 100644 --- a/fmriprep/utils/bids.py +++ b/fmriprep/utils/bids.py @@ -76,11 +76,7 @@ def collect_derivatives( # whereas relevant src file will be "bold". query = { **q, - **{ - k: v - for k, v in entities.items() - if k not in ['suffix', 'extension'] - }, + **{k: v for k, v in entities.items() if k not in ['suffix', 'extension']}, } if xfm == 'boldref2fmap': # fieldmaps have ids like auto_00000 diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index b69777205..92d963afa 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -41,4 +41,3 @@ def test_transforms_found_as_str(tmp_path: Path, xfm: str): xfm_in_transforms = xfm in derivs.get('transforms') transform_is_str = isinstance(derivs.get('transforms').get(xfm), str) assert all((transforms_in_derivs, xfm_in_transforms, transform_is_str)) - From 0f2078d442d70095de8f111ed0ee67b7de13b943 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Tue, 1 Oct 2024 23:19:45 -0400 Subject: [PATCH 04/15] stick with long lines --- fmriprep/utils/tests/test_derivative_cache.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index 92d963afa..c10d584eb 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -22,11 +22,9 @@ def test_transforms_found_as_str(tmp_path: Path, xfm: str): 'extension': '.nii.gz', } if xfm == 'boldref2fmap': - to_find = f'sub-{entities['subject']}_task-{entities['task']} \ - _from-{spec['from']}_to-auto00000_mode-image_xfm.txt' + to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-auto00000_mode-image_xfm.txt' else: - to_find = f'sub-{entities['subject']}_task-{entities['task']} \ - _from-{spec['from']}_to-{spec['to']}_mode-image_xfm.txt' + to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-{spec['to']}_mode-image_xfm.txt' funcd = tmp_path / f'sub-{entities['subject']}' / 'func' funcd.mkdir(parents=True) From d2a79a5c9396337130ad02187146208ddd4f7bea Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Tue, 1 Oct 2024 23:20:44 -0400 Subject: [PATCH 05/15] silence warnings --- fmriprep/utils/tests/test_derivative_cache.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index c10d584eb..6aebf5d90 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -22,9 +22,9 @@ def test_transforms_found_as_str(tmp_path: Path, xfm: str): 'extension': '.nii.gz', } if xfm == 'boldref2fmap': - to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-auto00000_mode-image_xfm.txt' + to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-auto00000_mode-image_xfm.txt' # noqa: E501 else: - to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-{spec['to']}_mode-image_xfm.txt' + to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-{spec['to']}_mode-image_xfm.txt' # noqa: E501 funcd = tmp_path / f'sub-{entities['subject']}' / 'func' funcd.mkdir(parents=True) From bd290fc7d9f213e791920b05307c0f55c75629f5 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Tue, 1 Oct 2024 23:24:57 -0400 Subject: [PATCH 06/15] repair quotes --- fmriprep/utils/tests/test_derivative_cache.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index 6aebf5d90..5641434ee 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -22,11 +22,11 @@ def test_transforms_found_as_str(tmp_path: Path, xfm: str): 'extension': '.nii.gz', } if xfm == 'boldref2fmap': - to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-auto00000_mode-image_xfm.txt' # noqa: E501 + to_find = f'sub-{entities["subject"]}_task-{entities["task"]}_from-{spec["from"]}_to-auto00000_mode-image_xfm.txt' # noqa: E501 else: - to_find = f'sub-{entities['subject']}_task-{entities['task']}_from-{spec['from']}_to-{spec['to']}_mode-image_xfm.txt' # noqa: E501 + to_find = f'sub-{entities["subject"]}_task-{entities["task"]}_from-{spec["from"]}_to-{spec["to"]}_mode-image_xfm.txt' # noqa: E501 - funcd = tmp_path / f'sub-{entities['subject']}' / 'func' + funcd = tmp_path / f'sub-{entities["subject"]}' / 'func' funcd.mkdir(parents=True) (funcd / to_find).touch() From a288deab58fe13f7e9ef12ab246e2db0d776fbca Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 07:40:52 -0400 Subject: [PATCH 07/15] only modify fieldmap_id when not None --- fmriprep/utils/bids.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fmriprep/utils/bids.py b/fmriprep/utils/bids.py index 64be39715..9f745b412 100644 --- a/fmriprep/utils/bids.py +++ b/fmriprep/utils/bids.py @@ -78,7 +78,7 @@ def collect_derivatives( **q, **{k: v for k, v in entities.items() if k not in ['suffix', 'extension']}, } - if xfm == 'boldref2fmap': + if xfm == 'boldref2fmap' and fieldmap_id: # fieldmaps have ids like auto_00000 query['to'] = fieldmap_id.replace('_', '') item = layout.get(return_type='filename', **query) From 2ceb3e7a58545b44868eee0a0e1e7d09bc27e549 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 07:53:55 -0400 Subject: [PATCH 08/15] fix thinko on boldref2anat --- fmriprep/data/io_spec.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fmriprep/data/io_spec.json b/fmriprep/data/io_spec.json index 44b684563..364cff7e3 100644 --- a/fmriprep/data/io_spec.json +++ b/fmriprep/data/io_spec.json @@ -33,7 +33,7 @@ }, "boldref2anat": { "datatype": "func", - "from": "orig", + "from": "boldref", "to": "anat", "mode": "image", "suffix": "xfm", From 758caaf391d5de4e434f9495d6569b6e5a6db340 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 09:28:17 -0400 Subject: [PATCH 09/15] Update fmriprep/utils/tests/test_derivative_cache.py Avoid possible bugs in `io_spec.json` during test Co-authored-by: Chris Markiewicz --- fmriprep/utils/tests/test_derivative_cache.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index 5641434ee..28c3c03cf 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -9,12 +9,6 @@ @pytest.mark.parametrize('xfm', ['boldref2fmap', 'boldref2anat', 'hmc']) def test_transforms_found_as_str(tmp_path: Path, xfm: str): - spec = ( - json.loads(load_data.readable('io_spec.json').read_text()) - .get('queries') - .get('transforms') - .get(xfm) - ) entities = { 'subject': '0', 'task': 'rest', From bef2a21de2dbf487d7754b7697db9158db6a55ad Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 09:29:42 -0400 Subject: [PATCH 10/15] Update fmriprep/utils/tests/test_derivative_cache.py Simplify assertion in test for loaded transforms Co-authored-by: Chris Markiewicz --- fmriprep/utils/tests/test_derivative_cache.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index 28c3c03cf..234792f6d 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -29,7 +29,4 @@ def test_transforms_found_as_str(tmp_path: Path, xfm: str): entities=entities, fieldmap_id='auto_00000', ) - transforms_in_derivs = 'transforms' in derivs - xfm_in_transforms = xfm in derivs.get('transforms') - transform_is_str = isinstance(derivs.get('transforms').get(xfm), str) - assert all((transforms_in_derivs, xfm_in_transforms, transform_is_str)) + assert derivs == {'transforms': {xfm: str(to_find)}} From 97f637a0cd44ccbf8ba688bb21339a8779b09031 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 09:30:42 -0400 Subject: [PATCH 11/15] use variables while populating the directory and a dictionary when preparing to call collect_derivatives Co-authored-by: Chris Markiewicz --- fmriprep/utils/tests/test_derivative_cache.py | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index 234792f6d..c54c6255b 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -9,20 +9,28 @@ @pytest.mark.parametrize('xfm', ['boldref2fmap', 'boldref2anat', 'hmc']) def test_transforms_found_as_str(tmp_path: Path, xfm: str): + sub = '0' + task = 'rest' + fromto = { + 'hmc': 'from-orig_to-boldref', + 'boldref2fmap': 'from-boldref_to-auto00000', + 'boldref2anat': 'from-boldref_to-anat', + }[xfm] + + to_find = tmp_path.joinpath( + f'sub-{subject}', + 'func', + f'sub-{subject}_task-{task}_{fromto}_mode-image_xfm.txt' + ) + to_find.parent.mkdir(parents=True) + to_find.touch() + entities = { - 'subject': '0', - 'task': 'rest', + 'subject': subject, + 'task': task, 'suffix': 'bold', 'extension': '.nii.gz', } - if xfm == 'boldref2fmap': - to_find = f'sub-{entities["subject"]}_task-{entities["task"]}_from-{spec["from"]}_to-auto00000_mode-image_xfm.txt' # noqa: E501 - else: - to_find = f'sub-{entities["subject"]}_task-{entities["task"]}_from-{spec["from"]}_to-{spec["to"]}_mode-image_xfm.txt' # noqa: E501 - - funcd = tmp_path / f'sub-{entities["subject"]}' / 'func' - funcd.mkdir(parents=True) - (funcd / to_find).touch() derivs = bids.collect_derivatives( derivatives_dir=tmp_path, From dc83e12f022e38ce268c4124e797f9c878a9c86f Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 09:33:02 -0400 Subject: [PATCH 12/15] Reorder merging of queries to simplify overrides Co-authored-by: Chris Markiewicz --- fmriprep/utils/bids.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fmriprep/utils/bids.py b/fmriprep/utils/bids.py index 9f745b412..b29275475 100644 --- a/fmriprep/utils/bids.py +++ b/fmriprep/utils/bids.py @@ -74,10 +74,7 @@ def collect_derivatives( # (e.g., ".nii.gz" vs ".txt"). # And transform suffixes will be "xfm", # whereas relevant src file will be "bold". - query = { - **q, - **{k: v for k, v in entities.items() if k not in ['suffix', 'extension']}, - } + query = {**entities, **q} if xfm == 'boldref2fmap' and fieldmap_id: # fieldmaps have ids like auto_00000 query['to'] = fieldmap_id.replace('_', '') From 1c9518962cce358b120cc4c6ad45796742600337 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 09:35:01 -0400 Subject: [PATCH 13/15] remove unused imports; fix variable name --- fmriprep/utils/tests/test_derivative_cache.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index c54c6255b..bd48bc7a4 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -1,15 +1,13 @@ -import json from pathlib import Path import pytest -from fmriprep.data import load as load_data from fmriprep.utils import bids @pytest.mark.parametrize('xfm', ['boldref2fmap', 'boldref2anat', 'hmc']) def test_transforms_found_as_str(tmp_path: Path, xfm: str): - sub = '0' + subject = '0' task = 'rest' fromto = { 'hmc': 'from-orig_to-boldref', From 74ad5a3f3bc80d43f20a5e558000d7934dec24c6 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 09:36:20 -0400 Subject: [PATCH 14/15] lint --- fmriprep/utils/tests/test_derivative_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index bd48bc7a4..3b255f38c 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -14,7 +14,7 @@ def test_transforms_found_as_str(tmp_path: Path, xfm: str): 'boldref2fmap': 'from-boldref_to-auto00000', 'boldref2anat': 'from-boldref_to-anat', }[xfm] - + to_find = tmp_path.joinpath( f'sub-{subject}', 'func', From 1a9ccce740e80154839e8657f5560134cf3acb62 Mon Sep 17 00:00:00 2001 From: Patrick Sadil Date: Wed, 2 Oct 2024 09:38:04 -0400 Subject: [PATCH 15/15] lint with ruff --- fmriprep/utils/tests/test_derivative_cache.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fmriprep/utils/tests/test_derivative_cache.py b/fmriprep/utils/tests/test_derivative_cache.py index 3b255f38c..1a2c59acc 100644 --- a/fmriprep/utils/tests/test_derivative_cache.py +++ b/fmriprep/utils/tests/test_derivative_cache.py @@ -16,10 +16,8 @@ def test_transforms_found_as_str(tmp_path: Path, xfm: str): }[xfm] to_find = tmp_path.joinpath( - f'sub-{subject}', - 'func', - f'sub-{subject}_task-{task}_{fromto}_mode-image_xfm.txt' - ) + f'sub-{subject}', 'func', f'sub-{subject}_task-{task}_{fromto}_mode-image_xfm.txt' + ) to_find.parent.mkdir(parents=True) to_find.touch()