Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Support named derivative paths #3264

Merged
merged 13 commits into from
Mar 29, 2024
31 changes: 26 additions & 5 deletions fmriprep/cli/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,23 @@ 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 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[name] = loc
setattr(namespace, self.dest, d)

def _path_exists(path, parser):
"""Ensure a given path exists."""
if path is None or not Path(path).exists():
Expand Down Expand Up @@ -222,11 +239,15 @@ def _slice_time_ref(value, parser):
g_bids.add_argument(
'-d',
'--derivatives',
action='store',
metavar='PATH',
type=Path,
nargs='*',
help='Search PATH(s) for pre-computed derivatives.',
action=ToDict,
metavar='PACKAGE=PATH',
type=str,
nargs='+',
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',
Expand Down
42 changes: 42 additions & 0 deletions fmriprep/cli/tests/test_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,3 +228,45 @@ 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 == {'smriprep': bids_path / 'derivatives/smriprep'}
_reset_config()

# Providing --derivatives with names should use them
temp_args = args + [
'--derivatives',
f'anat={str(bids_path / "derivatives/smriprep")}',
]
opts = parser.parse_args(temp_args)
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()
2 changes: 1 addition & 1 deletion fmriprep/cli/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
]

if config.execution.derivatives:
init_msg += [f'Searching for derivatives: {config.execution.derivatives}.']
init_msg += [f'Searching for derivatives: {list(config.execution.derivatives.values())}.']

Check warning on line 119 in fmriprep/cli/workflow.py

View check run for this annotation

Codecov / codecov/patch

fmriprep/cli/workflow.py#L119

Added line #L119 was not covered by tests

if config.execution.fs_subjects_dir:
init_msg += [f"Pre-run FreeSurfer's SUBJECTS_DIR: {config.execution.fs_subjects_dir}."]
Expand Down
6 changes: 3 additions & 3 deletions fmriprep/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@

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."""
Expand Down Expand Up @@ -526,8 +526,8 @@
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

Check warning on line 530 in fmriprep/config.py

View check run for this annotation

Codecov / codecov/patch

fmriprep/config.py#L530

Added line #L530 was not covered by tests
cls.dataset_links = dataset_links

if 'all' in cls.debug:
Expand Down
4 changes: 2 additions & 2 deletions fmriprep/workflows/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@

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():

Check warning on line 252 in fmriprep/workflows/base.py

View check run for this annotation

Codecov / codecov/patch

fmriprep/workflows/base.py#L252

Added line #L252 was not covered by tests
anatomical_cache.update(
collect_anat_derivatives(
derivatives_dir=deriv_dir,
Expand Down Expand Up @@ -653,7 +653,7 @@

entities = extract_entities(bold_series)

for deriv_dir in config.execution.derivatives:
for deriv_dir in config.execution.derivatives.values():

Check warning on line 656 in fmriprep/workflows/base.py

View check run for this annotation

Codecov / codecov/patch

fmriprep/workflows/base.py#L656

Added line #L656 was not covered by tests
functional_cache.update(
collect_derivatives(
derivatives_dir=deriv_dir,
Expand Down
2 changes: 1 addition & 1 deletion wrapper/src/fmriprep_docker/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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('{}=/deriv/{}'.format(deriv, deriv))
tsalo marked this conversation as resolved.
Show resolved Hide resolved

if opts.work_dir:
command.extend(['-v', ':'.join((opts.work_dir, '/scratch'))])
Expand Down