From 0e0ebfbc01ff64746944c94e8dfaf45b481bde0a Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Thu, 28 Mar 2024 14:56:13 -0400 Subject: [PATCH 01/13] Support named derivative paths. --- fmriprep/cli/parser.py | 22 +++++++++++++++++++--- fmriprep/config.py | 6 +++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/fmriprep/cli/parser.py b/fmriprep/cli/parser.py index 195706610..7ae36407e 100644 --- a/fmriprep/cli/parser.py +++ b/fmriprep/cli/parser.py @@ -22,6 +22,7 @@ # """Parser.""" +import os import sys from .. import config @@ -62,6 +63,17 @@ def __call__(self, parser, namespace, values, option_string=None): print(msg, file=sys.stderr) delattr(namespace, self.dest) + class ToDict(Action): + def __call__(self, parser, namespace, values, option_string=None): + d = {} + for i_kv, kv in enumerate(values): + if '=' not in kv: + k = f'deriv-{i_kv}' + else: + k, v = kv.split('=') + d[k] = os.path.abspath(v) + setattr(namespace, self.dest, d) + def _path_exists(path, parser): """Ensure a given path exists.""" if path is None or not Path(path).exists(): @@ -222,11 +234,15 @@ def _slice_time_ref(value, parser): g_bids.add_argument( '-d', '--derivatives', - action='store', - metavar='PATH', + action=ToDict, + metavar='PACKAGE=PATH', type=Path, nargs='*', - help='Search PATH(s) for pre-computed derivatives.', + help=( + 'Search PATH(s) for pre-computed derivatives. ' + 'These may be provided as named folders ' + '(e.g., `--derivatives smriprep=/path/to/smriprep`).' + ), ) g_bids.add_argument( '--bids-database-dir', diff --git a/fmriprep/config.py b/fmriprep/config.py index 08fbb51ce..4ba1ead2e 100644 --- a/fmriprep/config.py +++ b/fmriprep/config.py @@ -380,7 +380,7 @@ class execution(_Config): bids_dir = None """An existing path to the dataset, which must be BIDS-compliant.""" - derivatives = [] + derivatives = {} """Path(s) to search for pre-computed derivatives""" bids_database_dir = None """Path to the directory containing SQLite database indices for the input BIDS dataset.""" @@ -526,8 +526,8 @@ def _process_value(value): cls.bids_filters[acq][k] = _process_value(v) dataset_links = {'raw': cls.bids_dir} - for i_deriv, deriv_path in enumerate(cls.derivatives): - dataset_links[f'deriv-{i_deriv}'] = deriv_path + for deriv_name, deriv_path in cls.derivatives.items(): + dataset_links[deriv_name] = deriv_path cls.dataset_links = dataset_links if 'all' in cls.debug: From b3933b4c36932b1803045c5bc6a9397d06b5b986 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Thu, 28 Mar 2024 15:10:05 -0400 Subject: [PATCH 02/13] Leverage the naming in fmriprep-docker. --- wrapper/src/fmriprep_docker/__main__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrapper/src/fmriprep_docker/__main__.py b/wrapper/src/fmriprep_docker/__main__.py index 86bf7c48f..71ae27221 100755 --- a/wrapper/src/fmriprep_docker/__main__.py +++ b/wrapper/src/fmriprep_docker/__main__.py @@ -521,7 +521,7 @@ def main(): unknown_args.append('--derivatives') for deriv, deriv_path in opts.derivatives.items(): command.extend(['-v', '{}:/deriv/{}:ro'.format(deriv_path, deriv)]) - unknown_args.append('/deriv/{}'.format(deriv)) + unknown_args.append(f'{deriv}=/deriv/{deriv}') if opts.work_dir: command.extend(['-v', ':'.join((opts.work_dir, '/scratch'))]) From aacef03259b753f36a25abf03d035a352619e3a3 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Thu, 28 Mar 2024 15:32:35 -0400 Subject: [PATCH 03/13] Fix parameter type. --- fmriprep/cli/parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fmriprep/cli/parser.py b/fmriprep/cli/parser.py index 7ae36407e..77fbb63c2 100644 --- a/fmriprep/cli/parser.py +++ b/fmriprep/cli/parser.py @@ -71,7 +71,7 @@ def __call__(self, parser, namespace, values, option_string=None): k = f'deriv-{i_kv}' else: k, v = kv.split('=') - d[k] = os.path.abspath(v) + d[k] = Path(v) setattr(namespace, self.dest, d) def _path_exists(path, parser): @@ -236,7 +236,7 @@ def _slice_time_ref(value, parser): '--derivatives', action=ToDict, metavar='PACKAGE=PATH', - type=Path, + type=str, nargs='*', help=( 'Search PATH(s) for pre-computed derivatives. ' From b27e889f791bd3628cd94ca68eefc04d1dcddd60 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Thu, 28 Mar 2024 15:33:36 -0400 Subject: [PATCH 04/13] Remove unused import. --- fmriprep/cli/parser.py | 1 - 1 file changed, 1 deletion(-) diff --git a/fmriprep/cli/parser.py b/fmriprep/cli/parser.py index 77fbb63c2..b5fd2d6e3 100644 --- a/fmriprep/cli/parser.py +++ b/fmriprep/cli/parser.py @@ -22,7 +22,6 @@ # """Parser.""" -import os import sys from .. import config From 9f16c3e97b5033fb3236beac6295d18842586701 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Thu, 28 Mar 2024 16:02:33 -0400 Subject: [PATCH 05/13] Try fixing remaining problems. --- fmriprep/cli/workflow.py | 2 +- fmriprep/workflows/base.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/fmriprep/cli/workflow.py b/fmriprep/cli/workflow.py index 89310590b..5d62545e6 100644 --- a/fmriprep/cli/workflow.py +++ b/fmriprep/cli/workflow.py @@ -116,7 +116,7 @@ def build_workflow(config_file, retval): ] if config.execution.derivatives: - init_msg += [f'Searching for derivatives: {config.execution.derivatives}.'] + init_msg += [f'Searching for derivatives: {list(config.execution.derivatives.values())}.'] if config.execution.fs_subjects_dir: init_msg += [f"Pre-run FreeSurfer's SUBJECTS_DIR: {config.execution.fs_subjects_dir}."] diff --git a/fmriprep/workflows/base.py b/fmriprep/workflows/base.py index df3268a5d..d7387b5ee 100644 --- a/fmriprep/workflows/base.py +++ b/fmriprep/workflows/base.py @@ -249,7 +249,7 @@ def init_single_subject_wf(subject_id: str): std_spaces = spaces.get_spaces(nonstandard=False, dim=(3,)) std_spaces.append('fsnative') - for deriv_dir in config.execution.derivatives: + for deriv_dir in config.execution.derivatives.values(): anatomical_cache.update( collect_anat_derivatives( derivatives_dir=deriv_dir, @@ -653,7 +653,7 @@ def init_single_subject_wf(subject_id: str): entities = extract_entities(bold_series) - for deriv_dir in config.execution.derivatives: + for deriv_dir in config.execution.derivatives.values(): functional_cache.update( collect_derivatives( derivatives_dir=deriv_dir, From 5360ede63461ca8b3a7f297e338f54d7f0084d43 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Thu, 28 Mar 2024 16:34:57 -0400 Subject: [PATCH 06/13] Update __main__.py --- wrapper/src/fmriprep_docker/__main__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrapper/src/fmriprep_docker/__main__.py b/wrapper/src/fmriprep_docker/__main__.py index 71ae27221..80f975b7a 100755 --- a/wrapper/src/fmriprep_docker/__main__.py +++ b/wrapper/src/fmriprep_docker/__main__.py @@ -521,7 +521,7 @@ def main(): unknown_args.append('--derivatives') for deriv, deriv_path in opts.derivatives.items(): command.extend(['-v', '{}:/deriv/{}:ro'.format(deriv_path, deriv)]) - unknown_args.append(f'{deriv}=/deriv/{deriv}') + unknown_args.append('{}=/deriv/{}'.format(deriv, deriv)) if opts.work_dir: command.extend(['-v', ':'.join((opts.work_dir, '/scratch'))]) From 5c6e71c274597c881ad6af7a81784bcfba1a9372 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 29 Mar 2024 09:30:40 -0400 Subject: [PATCH 07/13] Write a test for the `--derivatives` parameter. --- fmriprep/cli/tests/test_parser.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/fmriprep/cli/tests/test_parser.py b/fmriprep/cli/tests/test_parser.py index 371fe7fb5..c21f8d100 100644 --- a/fmriprep/cli/tests/test_parser.py +++ b/fmriprep/cli/tests/test_parser.py @@ -228,3 +228,34 @@ def test_use_syn_sdc(tmp_path, args, expectation): assert opts.use_syn_sdc == expectation _reset_config() + + +def test_derivatives(tmp_path): + """Check the correct parsing of the derivatives argument.""" + bids_path = tmp_path / 'data' + out_path = tmp_path / 'out' + args = [str(bids_path), str(out_path), 'participant'] + bids_path.mkdir() + + parser = _build_parser() + + # Providing --derivatives without a path should raise an error + temp_args = args + ['--derivatives'] + with pytest.raises((SystemExit, ArgumentError)): + parser.parse_args(temp_args) + _reset_config() + + # Providing --derivatives without names should automatically label them + temp_args = args + ['--derivatives', str(bids_path / 'derivatives/smriprep')] + opts = parser.parse_args(temp_args) + assert opts.derivatives == {'deriv-0': str(bids_path / 'derivatives/smriprep')} + _reset_config() + + # Providing --derivatives with names should use them + temp_args = args + [ + '--derivatives', + f'smriprep={str(bids_path / 'derivatives/smriprep')}', + ] + opts = parser.parse_args(temp_args) + assert opts.derivatives == {'smriprep': str(bids_path / 'derivatives/smriprep')} + _reset_config() From 1b560c4be14330de46b1b36a7367da9d3627ca60 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 29 Mar 2024 09:46:04 -0400 Subject: [PATCH 08/13] Fix mistake. --- fmriprep/cli/tests/test_parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fmriprep/cli/tests/test_parser.py b/fmriprep/cli/tests/test_parser.py index c21f8d100..3b488ee5a 100644 --- a/fmriprep/cli/tests/test_parser.py +++ b/fmriprep/cli/tests/test_parser.py @@ -254,7 +254,7 @@ def test_derivatives(tmp_path): # Providing --derivatives with names should use them temp_args = args + [ '--derivatives', - f'smriprep={str(bids_path / 'derivatives/smriprep')}', + f'smriprep={str(bids_path / "derivatives/smriprep")}', ] opts = parser.parse_args(temp_args) assert opts.derivatives == {'smriprep': str(bids_path / 'derivatives/smriprep')} From 0cb12cc80ed9efbf0236a0b019eb817262180a22 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 29 Mar 2024 10:17:16 -0400 Subject: [PATCH 09/13] Change nargs for --derivatives. --- fmriprep/cli/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fmriprep/cli/parser.py b/fmriprep/cli/parser.py index b5fd2d6e3..b356a0aed 100644 --- a/fmriprep/cli/parser.py +++ b/fmriprep/cli/parser.py @@ -236,7 +236,7 @@ def _slice_time_ref(value, parser): action=ToDict, metavar='PACKAGE=PATH', type=str, - nargs='*', + nargs='+', help=( 'Search PATH(s) for pre-computed derivatives. ' 'These may be provided as named folders ' From e05038bc36aa2e618108acf50f9bc8efcc375e52 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 29 Mar 2024 10:22:30 -0400 Subject: [PATCH 10/13] Fix. --- fmriprep/cli/parser.py | 1 + 1 file changed, 1 insertion(+) diff --git a/fmriprep/cli/parser.py b/fmriprep/cli/parser.py index b356a0aed..88702311a 100644 --- a/fmriprep/cli/parser.py +++ b/fmriprep/cli/parser.py @@ -68,6 +68,7 @@ def __call__(self, parser, namespace, values, option_string=None): for i_kv, kv in enumerate(values): if '=' not in kv: k = f'deriv-{i_kv}' + v = kv else: k, v = kv.split('=') d[k] = Path(v) From 7bbb0748ed5d2b2c9710ea5912e3702084bc3b5d Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 29 Mar 2024 10:30:05 -0400 Subject: [PATCH 11/13] Update fmriprep/cli/parser.py Co-authored-by: Chris Markiewicz --- fmriprep/cli/parser.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/fmriprep/cli/parser.py b/fmriprep/cli/parser.py index 88702311a..d633f59ad 100644 --- a/fmriprep/cli/parser.py +++ b/fmriprep/cli/parser.py @@ -63,14 +63,17 @@ def __call__(self, parser, namespace, values, option_string=None): delattr(namespace, self.dest) class ToDict(Action): - def __call__(self, parser, namespace, values, option_string=None): d = {} - for i_kv, kv in enumerate(values): - if '=' not in kv: - k = f'deriv-{i_kv}' - v = kv - else: - k, v = kv.split('=') + for spec in values: + try: + name, loc = spec.split('=') + loc = Path(loc) + except ValueError: + loc = Path(spec) + name = loc.name + + if name in d: + raise ValueError(f"Received duplicate derivative name: {name}") d[k] = Path(v) setattr(namespace, self.dest, d) From cedf5bc0f0855d07109469eb64bb77dbeb33b1d3 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 29 Mar 2024 10:33:26 -0400 Subject: [PATCH 12/13] Fix and test. --- fmriprep/cli/parser.py | 6 ++++-- fmriprep/cli/tests/test_parser.py | 17 ++++++++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/fmriprep/cli/parser.py b/fmriprep/cli/parser.py index d633f59ad..61922b6ee 100644 --- a/fmriprep/cli/parser.py +++ b/fmriprep/cli/parser.py @@ -63,6 +63,7 @@ def __call__(self, parser, namespace, values, option_string=None): delattr(namespace, self.dest) class ToDict(Action): + def __call__(self, parser, namespace, values, option_string=None): d = {} for spec in values: try: @@ -73,8 +74,9 @@ class ToDict(Action): name = loc.name if name in d: - raise ValueError(f"Received duplicate derivative name: {name}") - d[k] = Path(v) + raise ValueError(f'Received duplicate derivative name: {name}') + + d[name] = loc setattr(namespace, self.dest, d) def _path_exists(path, parser): diff --git a/fmriprep/cli/tests/test_parser.py b/fmriprep/cli/tests/test_parser.py index 3b488ee5a..ce84fb60f 100644 --- a/fmriprep/cli/tests/test_parser.py +++ b/fmriprep/cli/tests/test_parser.py @@ -248,14 +248,25 @@ def test_derivatives(tmp_path): # Providing --derivatives without names should automatically label them temp_args = args + ['--derivatives', str(bids_path / 'derivatives/smriprep')] opts = parser.parse_args(temp_args) - assert opts.derivatives == {'deriv-0': str(bids_path / 'derivatives/smriprep')} + assert opts.derivatives == {'smriprep': bids_path / 'derivatives/smriprep'} _reset_config() # Providing --derivatives with names should use them temp_args = args + [ '--derivatives', - f'smriprep={str(bids_path / "derivatives/smriprep")}', + f'anat={str(bids_path / "derivatives/smriprep")}', ] opts = parser.parse_args(temp_args) - assert opts.derivatives == {'smriprep': str(bids_path / 'derivatives/smriprep')} + assert opts.derivatives == {'anat': bids_path / 'derivatives/smriprep'} + _reset_config() + + # Providing multiple unlabeled derivatives with the same name should raise an error + temp_args = args + [ + '--derivatives', + str(bids_path / 'derivatives_01/smriprep'), + str(bids_path / 'derivatives_02/smriprep'), + ] + with pytest.raises(ValueError, match='Received duplicate derivative name'): + parser.parse_args(temp_args) + _reset_config() From 30a70830bf9f3437432d362a28babd9a0d34dbca Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 29 Mar 2024 12:41:02 -0400 Subject: [PATCH 13/13] Update wrapper/src/fmriprep_docker/__main__.py Co-authored-by: Chris Markiewicz --- wrapper/src/fmriprep_docker/__main__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrapper/src/fmriprep_docker/__main__.py b/wrapper/src/fmriprep_docker/__main__.py index 80f975b7a..86bf7c48f 100755 --- a/wrapper/src/fmriprep_docker/__main__.py +++ b/wrapper/src/fmriprep_docker/__main__.py @@ -521,7 +521,7 @@ def main(): unknown_args.append('--derivatives') for deriv, deriv_path in opts.derivatives.items(): command.extend(['-v', '{}:/deriv/{}:ro'.format(deriv_path, deriv)]) - unknown_args.append('{}=/deriv/{}'.format(deriv, deriv)) + unknown_args.append('/deriv/{}'.format(deriv)) if opts.work_dir: command.extend(['-v', ':'.join((opts.work_dir, '/scratch'))])