From 6fe5b38541f618008974fe31944bc539a64205b2 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Wed, 5 Jun 2024 12:49:02 -0400 Subject: [PATCH 1/3] Reapply "Remove non-alphanumeric characters from workflow names and output entities" This reverts commit a8ccd27872b1298904758897cd5d2a9bee43384d. --- sdcflows/fieldmaps.py | 3 ++- sdcflows/workflows/base.py | 11 +++++++---- sdcflows/workflows/fit/base.py | 8 ++++++-- sdcflows/workflows/outputs.py | 6 ++++-- sdcflows/workflows/tests/test_base.py | 6 +++++- 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/sdcflows/fieldmaps.py b/sdcflows/fieldmaps.py index 0909592ee8..c03b52890e 100644 --- a/sdcflows/fieldmaps.py +++ b/sdcflows/fieldmaps.py @@ -446,7 +446,8 @@ def get_workflow(self, set_inputs=True, **kwargs): return self._wf # Override workflow name - kwargs["name"] = f"wf_{self.bids_id}" + clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', self.bids_id) + kwargs["name"] = f"wf_{clean_bids_id}" if self.method in (EstimatorType.MAPPED, EstimatorType.PHASEDIFF): from .workflows.fit.fieldmap import init_fmap_wf diff --git a/sdcflows/workflows/base.py b/sdcflows/workflows/base.py index 7c9c609d10..4a9f632fdd 100644 --- a/sdcflows/workflows/base.py +++ b/sdcflows/workflows/base.py @@ -21,6 +21,8 @@ # https://www.nipreps.org/community/licensing/ # """Estimate fieldmaps for :abbr:`SDC (susceptibility distortion correction)`.""" +import re + from nipype import logging from nipype.pipeline import engine as pe from nipype.interfaces import utility as niu @@ -106,6 +108,7 @@ def init_fmap_preproc_wf( ) for n, estimator in enumerate(estimators, 1): + clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', estimator.bids_id) est_wf = estimator.get_workflow( omp_nthreads=omp_nthreads, debug=debug, @@ -116,7 +119,7 @@ def init_fmap_preproc_wf( ] out_map = pe.Node( - niu.IdentityInterface(fields=out_fields), name=f"out_{estimator.bids_id}" + niu.IdentityInterface(fields=out_fields), name=f"out_{clean_bids_id}" ) out_map.inputs.fmap_id = estimator.bids_id @@ -124,7 +127,7 @@ def init_fmap_preproc_wf( output_dir=str(output_dir), write_coeff=True, bids_fmap_id=estimator.bids_id, - name=f"fmap_derivatives_wf_{estimator.bids_id}", + name=f"fmap_derivatives_wf_{clean_bids_id}", ) fmap_derivatives_wf.inputs.inputnode.source_files = source_files fmap_derivatives_wf.inputs.inputnode.fmap_meta = [ @@ -135,7 +138,7 @@ def init_fmap_preproc_wf( output_dir=str(output_dir), fmap_type=str(estimator.method).rpartition(".")[-1].lower(), bids_fmap_id=estimator.bids_id, - name=f"fmap_reports_wf_{estimator.bids_id}", + name=f"fmap_reports_wf_{clean_bids_id}", ) fmap_reports_wf.inputs.inputnode.source_files = source_files @@ -143,7 +146,7 @@ def init_fmap_preproc_wf( fields = INPUT_FIELDS[estimator.method] inputnode = pe.Node( niu.IdentityInterface(fields=fields), - name=f"in_{estimator.bids_id}", + name=f"in_{clean_bids_id}", ) # fmt:off workflow.connect([ diff --git a/sdcflows/workflows/fit/base.py b/sdcflows/workflows/fit/base.py index 1010b8b65f..09fc928221 100644 --- a/sdcflows/workflows/fit/base.py +++ b/sdcflows/workflows/fit/base.py @@ -25,6 +25,8 @@ def init_sdcflows_wf(): """Create a multi-subject, multi-estimator *SDCFlows* workflow.""" + import re + from nipype.pipeline.engine import Workflow from niworkflows.utils.bids import collect_participants @@ -51,6 +53,8 @@ def init_sdcflows_wf(): for subject, sub_estimators in estimators_record.items(): for estim in sub_estimators: + clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', estim.bids_id) + estim_wf = estim.get_workflow( omp_nthreads=config.nipype.omp_nthreads, sloppy=False, @@ -61,7 +65,7 @@ def init_sdcflows_wf(): output_dir=config.execution.output_dir, bids_fmap_id=estim.bids_id, write_coeff=True, - name=f"fmap_derivatives_{estim.bids_id}", + name=f"fmap_derivatives_{clean_bids_id}", ) source_paths = [ @@ -76,7 +80,7 @@ def init_sdcflows_wf(): fmap_type=estim.method, output_dir=config.execution.output_dir, bids_fmap_id=estim.bids_id, - name=f"fmap_reports_{estim.bids_id}", + name=f"fmap_reports_{clean_bids_id}", ) reportlets_wf.inputs.inputnode.source_files = source_paths diff --git a/sdcflows/workflows/outputs.py b/sdcflows/workflows/outputs.py index 24e3d559bf..35e0c72004 100644 --- a/sdcflows/workflows/outputs.py +++ b/sdcflows/workflows/outputs.py @@ -21,6 +21,8 @@ # https://www.nipreps.org/community/licensing/ # """Writing out outputs.""" +import re + from nipype.pipeline import engine as pe from nipype.interfaces import utility as niu from niworkflows.interfaces.bids import DerivativesDataSink as _DDS @@ -77,7 +79,7 @@ def init_fmap_reports_wf( custom_entities = custom_entities or {} if bids_fmap_id: - custom_entities["fmapid"] = bids_fmap_id.replace("_", "") + custom_entities["fmapid"] = re.sub(r'[^a-zA-Z0-9]', '', bids_fmap_id) workflow = pe.Workflow(name=name) inputnode = pe.Node( @@ -156,7 +158,7 @@ def init_fmap_derivatives_wf( """ custom_entities = custom_entities or {} if bids_fmap_id: - custom_entities["fmapid"] = bids_fmap_id.replace("_", "") + custom_entities["fmapid"] = re.sub(r'[^a-zA-Z0-9]', '', bids_fmap_id) workflow = pe.Workflow(name=name) inputnode = pe.Node( diff --git a/sdcflows/workflows/tests/test_base.py b/sdcflows/workflows/tests/test_base.py index b31222fecc..ac782a0bc0 100644 --- a/sdcflows/workflows/tests/test_base.py +++ b/sdcflows/workflows/tests/test_base.py @@ -23,7 +23,10 @@ """Test the base workflow.""" from pathlib import Path import os +import re + import pytest + from sdcflows import fieldmaps as fm from sdcflows.utils.wrangler import find_estimators from sdcflows.workflows.base import init_fmap_preproc_wf @@ -55,7 +58,8 @@ def test_fmap_wf(tmpdir, workdir, outdir, bids_layouts, dataset, subject): if estimator.method != fm.EstimatorType.PEPOLAR: continue - inputnode = wf.get_node(f"in_{estimator.bids_id}") + clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', estimator.bids_id) + inputnode = wf.get_node(f"in_{clean_bids_id}") inputnode.inputs.in_data = [str(f.path) for f in estimator.sources] inputnode.inputs.metadata = [f.metadata for f in estimator.sources] From c674fc1012e5c9e067fd10fc372aac0a1fa91a6e Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Wed, 5 Jun 2024 23:16:17 -0400 Subject: [PATCH 2/3] RF: Add sanitized_id field to FieldmapEstimation --- sdcflows/fieldmaps.py | 10 ++++++++-- sdcflows/workflows/base.py | 11 ++++------- sdcflows/workflows/fit/base.py | 8 ++------ sdcflows/workflows/tests/test_base.py | 6 +----- 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/sdcflows/fieldmaps.py b/sdcflows/fieldmaps.py index c03b52890e..b087731a07 100644 --- a/sdcflows/fieldmaps.py +++ b/sdcflows/fieldmaps.py @@ -300,6 +300,9 @@ class FieldmapEstimation: bids_id = attr.ib(default=None, kw_only=True, type=str, on_setattr=_id_setter) """The unique ``B0FieldIdentifier`` field of this fieldmap.""" + sanitized_id = attr.ib(init=False, repr=False) + """Sanitized version of the bids_id with special characters replaced by underscores.""" + _wf = attr.ib(init=False, default=None, repr=False) """Internal pointer to a workflow.""" @@ -436,6 +439,10 @@ def __attrs_post_init__(self): for intent_file in intents_meta: _intents[intent_file].add(self.bids_id) + # Provide a sanitized identifier that can be used in cases where + # special characters are not allowed. + self.sanitized_id = re.sub(r'[^a-zA-Z0-9]', '_', self.bids_id) + def paths(self): """Return a tuple of paths that are sorted.""" return tuple(sorted(str(f.path) for f in self.sources)) @@ -446,8 +453,7 @@ def get_workflow(self, set_inputs=True, **kwargs): return self._wf # Override workflow name - clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', self.bids_id) - kwargs["name"] = f"wf_{clean_bids_id}" + kwargs["name"] = f"wf_{self.sanitized_id}" if self.method in (EstimatorType.MAPPED, EstimatorType.PHASEDIFF): from .workflows.fit.fieldmap import init_fmap_wf diff --git a/sdcflows/workflows/base.py b/sdcflows/workflows/base.py index 4a9f632fdd..ae6cc2229f 100644 --- a/sdcflows/workflows/base.py +++ b/sdcflows/workflows/base.py @@ -21,8 +21,6 @@ # https://www.nipreps.org/community/licensing/ # """Estimate fieldmaps for :abbr:`SDC (susceptibility distortion correction)`.""" -import re - from nipype import logging from nipype.pipeline import engine as pe from nipype.interfaces import utility as niu @@ -108,7 +106,6 @@ def init_fmap_preproc_wf( ) for n, estimator in enumerate(estimators, 1): - clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', estimator.bids_id) est_wf = estimator.get_workflow( omp_nthreads=omp_nthreads, debug=debug, @@ -119,7 +116,7 @@ def init_fmap_preproc_wf( ] out_map = pe.Node( - niu.IdentityInterface(fields=out_fields), name=f"out_{clean_bids_id}" + niu.IdentityInterface(fields=out_fields), name=f"out_{estimator.bids_id}" ) out_map.inputs.fmap_id = estimator.bids_id @@ -127,7 +124,7 @@ def init_fmap_preproc_wf( output_dir=str(output_dir), write_coeff=True, bids_fmap_id=estimator.bids_id, - name=f"fmap_derivatives_wf_{clean_bids_id}", + name=f"fmap_derivatives_wf_{estimator.sanitized_id}", ) fmap_derivatives_wf.inputs.inputnode.source_files = source_files fmap_derivatives_wf.inputs.inputnode.fmap_meta = [ @@ -138,7 +135,7 @@ def init_fmap_preproc_wf( output_dir=str(output_dir), fmap_type=str(estimator.method).rpartition(".")[-1].lower(), bids_fmap_id=estimator.bids_id, - name=f"fmap_reports_wf_{clean_bids_id}", + name=f"fmap_reports_wf_{estimator.sanitized_id}", ) fmap_reports_wf.inputs.inputnode.source_files = source_files @@ -146,7 +143,7 @@ def init_fmap_preproc_wf( fields = INPUT_FIELDS[estimator.method] inputnode = pe.Node( niu.IdentityInterface(fields=fields), - name=f"in_{clean_bids_id}", + name=f"in_{estimator.sanitized_id}", ) # fmt:off workflow.connect([ diff --git a/sdcflows/workflows/fit/base.py b/sdcflows/workflows/fit/base.py index 09fc928221..5720cb8059 100644 --- a/sdcflows/workflows/fit/base.py +++ b/sdcflows/workflows/fit/base.py @@ -25,8 +25,6 @@ def init_sdcflows_wf(): """Create a multi-subject, multi-estimator *SDCFlows* workflow.""" - import re - from nipype.pipeline.engine import Workflow from niworkflows.utils.bids import collect_participants @@ -53,8 +51,6 @@ def init_sdcflows_wf(): for subject, sub_estimators in estimators_record.items(): for estim in sub_estimators: - clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', estim.bids_id) - estim_wf = estim.get_workflow( omp_nthreads=config.nipype.omp_nthreads, sloppy=False, @@ -65,7 +61,7 @@ def init_sdcflows_wf(): output_dir=config.execution.output_dir, bids_fmap_id=estim.bids_id, write_coeff=True, - name=f"fmap_derivatives_{clean_bids_id}", + name=f"fmap_derivatives_{estim.sanitized_id}", ) source_paths = [ @@ -80,7 +76,7 @@ def init_sdcflows_wf(): fmap_type=estim.method, output_dir=config.execution.output_dir, bids_fmap_id=estim.bids_id, - name=f"fmap_reports_{clean_bids_id}", + name=f"fmap_reports_{estim.sanitized_id}", ) reportlets_wf.inputs.inputnode.source_files = source_paths diff --git a/sdcflows/workflows/tests/test_base.py b/sdcflows/workflows/tests/test_base.py index ac782a0bc0..b31222fecc 100644 --- a/sdcflows/workflows/tests/test_base.py +++ b/sdcflows/workflows/tests/test_base.py @@ -23,10 +23,7 @@ """Test the base workflow.""" from pathlib import Path import os -import re - import pytest - from sdcflows import fieldmaps as fm from sdcflows.utils.wrangler import find_estimators from sdcflows.workflows.base import init_fmap_preproc_wf @@ -58,8 +55,7 @@ def test_fmap_wf(tmpdir, workdir, outdir, bids_layouts, dataset, subject): if estimator.method != fm.EstimatorType.PEPOLAR: continue - clean_bids_id = re.sub(r'[^a-zA-Z0-9]', '', estimator.bids_id) - inputnode = wf.get_node(f"in_{clean_bids_id}") + inputnode = wf.get_node(f"in_{estimator.bids_id}") inputnode.inputs.in_data = [str(f.path) for f in estimator.sources] inputnode.inputs.metadata = [f.metadata for f in estimator.sources] From fb55b6545d967ff64ad3d2ef0bf74dbc4668ff72 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Thu, 6 Jun 2024 12:54:35 -0400 Subject: [PATCH 3/3] TEST: Expected behavior with safe and unsafe bids_ids --- sdcflows/tests/test_fieldmaps.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/sdcflows/tests/test_fieldmaps.py b/sdcflows/tests/test_fieldmaps.py index 4d66b8ccc3..e4f0db155f 100644 --- a/sdcflows/tests/test_fieldmaps.py +++ b/sdcflows/tests/test_fieldmaps.py @@ -111,6 +111,7 @@ def test_FieldmapEstimation(dsA_dir, inputfiles, method, nsources, raises): assert fe.method == method assert len(fe.sources) == nsources assert fe.bids_id is not None and fe.bids_id.startswith("auto_") + assert fe.bids_id == fe.sanitized_id # Auto-generated IDs are sanitized # Attempt to change bids_id with pytest.raises(ValueError): @@ -243,6 +244,33 @@ def test_FieldmapEstimationIdentifier(monkeypatch, dsA_dir): fm.clear_registry() + fe = fm.FieldmapEstimation( + [ + fm.FieldmapFile( + dsA_dir / "sub-01" / "fmap/sub-01_fieldmap.nii.gz", + metadata={ + "Units": "Hz", + "B0FieldIdentifier": "fmap-with^special#chars", + "IntendedFor": ["file1.nii.gz", "file2.nii.gz"], + }, + ), + fm.FieldmapFile( + dsA_dir / "sub-01" / "fmap/sub-01_magnitude.nii.gz", + metadata={"Units": "Hz", "B0FieldIdentifier": "fmap-with^special#chars"}, + ), + ] + ) + assert fe.bids_id == "fmap-with^special#chars" + assert fe.sanitized_id == "fmap_with_special_chars" + # The unsanitized ID is used for lookups + assert fm.get_identifier("file1.nii.gz") == ("fmap-with^special#chars",) + assert fm.get_identifier("file2.nii.gz") == ("fmap-with^special#chars",) + + wf = fe.get_workflow() + assert wf.name == "wf_fmap_with_special_chars" + + fm.clear_registry() + def test_type_setter(): """Cover the _type_setter routine."""