From 3f02887866dd014b90d756b74516a5d364f9c261 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Thu, 13 Jun 2024 16:35:55 -0400 Subject: [PATCH] [ENH] Rename tabular file columns to `visit_id` and `session_id` (#263) * update manifest/doughnut columns (and DicomDirMap partially) * rename column `session` -> `session_id` in DicomDirMap * remove dicom_id and bids_id fields from doughnut * change sourcedata expected subdirectory names * rename check_session -> session_id_to_bids_id and participant_id_to_bids_id -> participant_id_to_bids_participant * refactor participant/session ID checking functions * rename bids_id -> bids_participant and session -> bids_session in invocations * update CLI arguments for participant/session * rename variables participant -> participant_id and session -> session_id * make test_default_config tests fail if replacement didn't work * rename column attributes in tabular classes (except Bagel) * rename VISIT/SESSION fields in global config * rename doughnut status columns * update raw dicom/imaging data directory to scratch/raw_imaging * add test for session_id_to_bids_session * address code review comments --- nipoppy_cli/nipoppy/cli/parser.py | 17 +- nipoppy_cli/nipoppy/cli/run.py | 12 +- nipoppy_cli/nipoppy/config/main.py | 34 ++-- .../descriptors/dcm2bids-3.1.0-convert.json | 6 +- .../descriptors/dcm2bids-3.1.0-prepare.json | 2 +- .../data/descriptors/fmriprep-20.2.7.json | 10 +- .../data/descriptors/fmriprep-23.1.3.json | 10 +- .../descriptors/heudiconv-0.12.2-convert.json | 6 +- .../descriptors/heudiconv-0.12.2-prepare.json | 6 +- .../data/descriptors/mriqc-23.1.0.json | 4 +- .../sample_global_config-all_pipelines.json | 8 +- ...sample_global_config-latest_pipelines.json | 8 +- .../nipoppy/data/examples/sample_manifest.csv | 10 +- .../sample_tracker_configs/mriqc-23.1.0.json | 4 +- .../nipoppy/data/layouts/layout-0.1.0.json | 2 +- .../nipoppy/data/layouts/layout-default.json | 6 +- nipoppy_cli/nipoppy/layout.py | 23 ++- nipoppy_cli/nipoppy/tabular/bagel.py | 72 ++++--- nipoppy_cli/nipoppy/tabular/dicom_dir_map.py | 46 ++--- nipoppy_cli/nipoppy/tabular/doughnut.py | 104 +++++----- nipoppy_cli/nipoppy/tabular/manifest.py | 78 ++++---- nipoppy_cli/nipoppy/utils.py | 137 +++++++------ nipoppy_cli/nipoppy/workflows/base.py | 10 +- .../nipoppy/workflows/bids_conversion.py | 32 +-- nipoppy_cli/nipoppy/workflows/dicom_reorg.py | 55 +++--- nipoppy_cli/nipoppy/workflows/doughnut.py | 2 +- nipoppy_cli/nipoppy/workflows/pipeline.py | 88 ++++----- nipoppy_cli/nipoppy/workflows/runner.py | 58 +++--- nipoppy_cli/nipoppy/workflows/tracker.py | 22 +-- nipoppy_cli/tests/conftest.py | 118 ++++++------ nipoppy_cli/tests/data/config1.json | 8 +- nipoppy_cli/tests/data/config2.json | 2 +- nipoppy_cli/tests/data/config3.json | 2 +- nipoppy_cli/tests/data/config_invalid1.json | 6 +- nipoppy_cli/tests/data/config_invalid2.json | 4 +- nipoppy_cli/tests/data/dicom_dir_map1.csv | 8 +- nipoppy_cli/tests/data/dicom_dir_map2.csv | 8 +- .../tests/data/dicom_dir_map_invalid1.csv | 8 +- .../tests/data/dicom_dir_map_invalid3.csv | 8 +- .../tests/data/dicom_dir_map_invalid4.csv | 8 +- nipoppy_cli/tests/data/doughnut1.csv | 10 +- nipoppy_cli/tests/data/doughnut2.csv | 10 +- nipoppy_cli/tests/data/doughnut_invalid1.csv | 10 +- nipoppy_cli/tests/data/doughnut_invalid2.csv | 10 +- nipoppy_cli/tests/data/layout1.json | 4 +- nipoppy_cli/tests/data/layout2.json | 4 +- nipoppy_cli/tests/data/layout_invalid1.json | 4 +- nipoppy_cli/tests/data/layout_invalid2.json | 4 +- nipoppy_cli/tests/data/manifest1.csv | 10 +- nipoppy_cli/tests/data/manifest2.csv | 6 +- nipoppy_cli/tests/data/manifest3.csv | 6 +- nipoppy_cli/tests/data/manifest_invalid1.csv | 6 +- nipoppy_cli/tests/data/manifest_invalid2.csv | 10 +- nipoppy_cli/tests/data/manifest_invalid3.csv | 12 +- nipoppy_cli/tests/data/manifest_invalid4.csv | 12 +- nipoppy_cli/tests/test_config_main.py | 20 +- nipoppy_cli/tests/test_default_config.py | 40 ++-- nipoppy_cli/tests/test_layout.py | 35 ++-- nipoppy_cli/tests/test_parser.py | 12 +- nipoppy_cli/tests/test_tabular_bagel.py | 88 ++++----- .../tests/test_tabular_dicom_dir_map.py | 8 +- nipoppy_cli/tests/test_tabular_doughnut.py | 114 +++++------ nipoppy_cli/tests/test_tabular_manifest.py | 81 ++++---- nipoppy_cli/tests/test_utils.py | 94 ++++----- nipoppy_cli/tests/test_workflow_base.py | 2 +- .../tests/test_workflow_bids_conversion.py | 65 ++----- .../tests/test_workflow_dicom_reorg.py | 182 ++++++++++-------- nipoppy_cli/tests/test_workflow_doughnut.py | 66 +++---- nipoppy_cli/tests/test_workflow_pipeline.py | 137 +++++++------ nipoppy_cli/tests/test_workflow_runner.py | 111 ++++++----- nipoppy_cli/tests/test_workflow_tracker.py | 64 +++--- 71 files changed, 1162 insertions(+), 1127 deletions(-) diff --git a/nipoppy_cli/nipoppy/cli/parser.py b/nipoppy_cli/nipoppy/cli/parser.py index c7492563..35ab68e0 100644 --- a/nipoppy_cli/nipoppy/cli/parser.py +++ b/nipoppy_cli/nipoppy/cli/parser.py @@ -5,12 +5,7 @@ from pathlib import Path from nipoppy.layout import DEFAULT_LAYOUT_INFO -from nipoppy.utils import ( - BIDS_SESSION_PREFIX, - BIDS_SUBJECT_PREFIX, - check_participant, - check_session, -) +from nipoppy.utils import BIDS_SESSION_PREFIX, BIDS_SUBJECT_PREFIX PROGRAM_NAME = "nipoppy" COMMAND_INIT = "init" @@ -51,16 +46,14 @@ def add_arg_simulate(parser: _ActionsContainer) -> _ActionsContainer: def add_args_participant_and_session(parser: _ActionsContainer) -> _ActionsContainer: - """Add --participant and --session arguments to the parser.""" + """Add --participant-id and --session-id arguments to the parser.""" parser.add_argument( - "--participant", - type=check_participant, + "--participant-id", required=False, help=f"Participant ID (with or without the {BIDS_SUBJECT_PREFIX} prefix).", ) parser.add_argument( - "--session", - type=check_session, + "--session-id", required=False, help=f"Session ID (with or without the {BIDS_SESSION_PREFIX} prefix).", ) @@ -211,7 +204,7 @@ def add_subparser_dicom_reorg( """Add subparser for reorg command.""" description = ( "(Re)organize raw (DICOM) files, from the raw DICOM directory " - f"({DEFAULT_LAYOUT_INFO.dpath_raw_dicom}) to the organized " + f"({DEFAULT_LAYOUT_INFO.dpath_raw_imaging}) to the organized " f"sourcedata directory ({DEFAULT_LAYOUT_INFO.dpath_sourcedata})." ) parser = subparsers.add_parser( diff --git a/nipoppy_cli/nipoppy/cli/run.py b/nipoppy_cli/nipoppy/cli/run.py index a02dcf77..352f6a5e 100644 --- a/nipoppy_cli/nipoppy/cli/run.py +++ b/nipoppy_cli/nipoppy/cli/run.py @@ -67,8 +67,8 @@ def cli(argv: Sequence[str] = None) -> None: pipeline_name=args.pipeline, pipeline_version=args.pipeline_version, pipeline_step=args.pipeline_step, - participant=args.participant, - session=args.session, + participant_id=args.participant_id, + session_id=args.session_id, simulate=args.simulate, **workflow_kwargs, ) @@ -78,8 +78,8 @@ def cli(argv: Sequence[str] = None) -> None: pipeline_name=args.pipeline, pipeline_version=args.pipeline_version, pipeline_step=args.pipeline_step, - participant=args.participant, - session=args.session, + participant_id=args.participant_id, + session_id=args.session_id, simulate=args.simulate, **workflow_kwargs, ) @@ -88,8 +88,8 @@ def cli(argv: Sequence[str] = None) -> None: dpath_root=dpath_root, pipeline_name=args.pipeline, pipeline_version=args.pipeline_version, - participant=args.participant, - session=args.session, + participant_id=args.participant_id, + session_id=args.session_id, **workflow_kwargs, ) else: diff --git a/nipoppy_cli/nipoppy/config/main.py b/nipoppy_cli/nipoppy/config/main.py index a84a2ca2..540bd180 100644 --- a/nipoppy_cli/nipoppy/config/main.py +++ b/nipoppy_cli/nipoppy/config/main.py @@ -16,8 +16,6 @@ BIDS_SESSION_PREFIX, StrOrPathLike, apply_substitutions_to_json, - check_session, - check_session_strict, load_json, ) @@ -26,8 +24,13 @@ class Config(SchemaWithContainerConfig): """Schema for dataset configuration.""" DATASET_NAME: str = Field(description="Name of the dataset") - VISITS: list[str] = Field(description="List of visits available in the study") - SESSIONS: Optional[list[str]] = Field( + VISIT_IDS: list[str] = Field( + description=( + "List of visits available in the study. A visit ID is an identifier " + "for a data collection event, not restricted to imaging data." + ) + ) + SESSION_IDS: Optional[list[str]] = Field( default=None, # will be a list after validation description=( "List of BIDS-compliant sessions available in the study" @@ -42,7 +45,7 @@ class Config(SchemaWithContainerConfig): ", to be used in the DICOM reorg step. Note: this field and " "DICOM_DIR_PARTICIPANT_FIRST cannot both be specified" f'. The CSV should have three columns: "{DicomDirMap.col_participant_id}"' - f' , "{DicomDirMap.col_session}"' + f' , "{DicomDirMap.col_session_id}"' f', and "{DicomDirMap.col_participant_dicom_dir}"' ), ) @@ -50,7 +53,7 @@ class Config(SchemaWithContainerConfig): default=None, description=( "Whether subdirectories under the raw dicom directory (default: " - f"{DEFAULT_LAYOUT_INFO.dpath_raw_dicom}) follow the pattern " + f"{DEFAULT_LAYOUT_INFO.dpath_raw_imaging}) follow the pattern " "/ (default) or /. Note: " "this field and and DICOM_DIR_MAP_FILE cannot both be specified" ), @@ -76,12 +79,6 @@ class Config(SchemaWithContainerConfig): model_config = ConfigDict(extra="forbid") - def _check_sessions_have_prefix(self) -> Self: - """Check that sessions have the BIDS prefix.""" - for session in self.SESSIONS: - check_session_strict(session) - return self - def _check_dicom_dir_options(self) -> Self: """Check that only one DICOM directory mapping option is given.""" if ( @@ -137,21 +134,18 @@ def _propagate(pipeline_configs: list[PipelineConfig]): @classmethod def check_input(cls, data: Any): """Validate the raw input.""" - key_sessions = "SESSIONS" - key_visits = "VISITS" + key_session_ids = "SESSION_IDS" + key_visit_ids = "VISIT_IDS" if isinstance(data, dict): - # if sessions are not given, infer from visits - if key_sessions not in data: - data[key_sessions] = [ - check_session(visit) for visit in data[key_visits] - ] + # if session_ids is not given, set to be the same as visit_ids + if key_session_ids not in data: + data[key_session_ids] = data[key_visit_ids] return data @model_validator(mode="after") def validate_and_process(self) -> Self: """Validate and process the configuration.""" - self._check_sessions_have_prefix() self._check_dicom_dir_options() self._check_no_duplicate_pipeline() return self diff --git a/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-convert.json b/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-convert.json index c60de73e..92c7e7eb 100644 --- a/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-convert.json +++ b/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-convert.json @@ -15,7 +15,7 @@ "command-line-flag": "-d", "value-key": "[DICOM_DIR]", "default-value": [ - "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT]]/[[NIPOPPY_SESSION]]" + "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT_ID]]/[[NIPOPPY_BIDS_SESSION]]" ] }, { @@ -26,7 +26,7 @@ "optional": false, "command-line-flag": "-p", "value-key": "[PARTICIPANT]", - "default-value": "[[NIPOPPY_PARTICIPANT]]" + "default-value": "[[NIPOPPY_PARTICIPANT_ID]]" }, { "name": "session", @@ -36,7 +36,7 @@ "optional": true, "command-line-flag": "-s", "value-key": "[SESSION]", - "default-value": "[[NIPOPPY_SESSION_SHORT]]" + "default-value": "[[NIPOPPY_SESSION_ID]]" }, { "name": "config", diff --git a/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-prepare.json b/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-prepare.json index f4df1f2f..615ab9ec 100644 --- a/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-prepare.json +++ b/nipoppy_cli/nipoppy/data/descriptors/dcm2bids-3.1.0-prepare.json @@ -15,7 +15,7 @@ "command-line-flag": "-d", "value-key": "[DICOM_DIR]", "default-value": [ - "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT]]/[[NIPOPPY_SESSION]]" + "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT_ID]]/[[NIPOPPY_BIDS_SESSION]]" ] }, { diff --git a/nipoppy_cli/nipoppy/data/descriptors/fmriprep-20.2.7.json b/nipoppy_cli/nipoppy/data/descriptors/fmriprep-20.2.7.json index 3f272ef3..79257e78 100644 --- a/nipoppy_cli/nipoppy/data/descriptors/fmriprep-20.2.7.json +++ b/nipoppy_cli/nipoppy/data/descriptors/fmriprep-20.2.7.json @@ -63,7 +63,7 @@ "list": true, "command-line-flag": "--participant-label", "default-value": [ - "[[NIPOPPY_PARTICIPANT]]" + "[[NIPOPPY_PARTICIPANT_ID]]" ] }, { @@ -451,7 +451,7 @@ "type": "String", "value-key": "[FS_SUBJECTS_DIR]", "command-line-flag": "--fs-subjects-dir", - "default-value": "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/6.0.1/output/[[NIPOPPY_SESSION]]" + "default-value": "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/6.0.1/output/[[NIPOPPY_BIDS_SESSION]]" }, { "id": "hires", @@ -606,14 +606,14 @@ "CONTAINER_CONFIG": { "ARGS": [ "--bind", - "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/6.0.1/output/[[NIPOPPY_SESSION]]" + "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/6.0.1/output/[[NIPOPPY_BIDS_SESSION]]" ] } }, "nipoppy_old": { "paths_to_tar": [ - "[[NIPOPPY_DPATH_PIPELINE_OUTPUT]]/fmriprep/sub-[[NIPOPPY_PARTICIPANT]]/ses-[[NIPOPPY_SESSION]]", - "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/6.0.1/output/[[NIPOPPY_SESSION]]/sub-[[NIPOPPY_PARTICIPANT]]" + "[[NIPOPPY_DPATH_PIPELINE_OUTPUT]]/fmriprep/sub-[[NIPOPPY_PARTICIPANT_ID]]/ses-[[NIPOPPY_BIDS_SESSION]]", + "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/6.0.1/output/[[NIPOPPY_BIDS_SESSION]]/sub-[[NIPOPPY_PARTICIPANT_ID]]" ] } } diff --git a/nipoppy_cli/nipoppy/data/descriptors/fmriprep-23.1.3.json b/nipoppy_cli/nipoppy/data/descriptors/fmriprep-23.1.3.json index 649dd08f..95c57a95 100644 --- a/nipoppy_cli/nipoppy/data/descriptors/fmriprep-23.1.3.json +++ b/nipoppy_cli/nipoppy/data/descriptors/fmriprep-23.1.3.json @@ -54,7 +54,7 @@ "list": true, "command-line-flag": "--participant-label", "default-value": [ - "[[NIPOPPY_PARTICIPANT]]" + "[[NIPOPPY_PARTICIPANT_ID]]" ] }, { @@ -509,7 +509,7 @@ "type": "String", "value-key": "[FS_SUBJECTS_DIR]", "command-line-flag": "--fs-subjects-dir", - "default-value": "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/7.3.2/output/[[NIPOPPY_SESSION]]" + "default-value": "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/7.3.2/output/[[NIPOPPY_BIDS_SESSION]]" }, { "id": "hires", @@ -663,14 +663,14 @@ "CONTAINER_CONFIG": { "ARGS": [ "--bind", - "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/7.3.2/output/[[NIPOPPY_SESSION]]" + "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/7.3.2/output/[[NIPOPPY_BIDS_SESSION]]" ] } }, "nipoppy_old": { "paths_to_tar": [ - "[[NIPOPPY_DPATH_PIPELINE_OUTPUT]]/sub-[[NIPOPPY_PARTICIPANT]]/ses-[[NIPOPPY_SESSION]]", - "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/7.3.2/output/[[NIPOPPY_SESSION]]/sub-[[NIPOPPY_PARTICIPANT]]" + "[[NIPOPPY_DPATH_PIPELINE_OUTPUT]]/sub-[[NIPOPPY_PARTICIPANT_ID]]/ses-[[NIPOPPY_BIDS_SESSION]]", + "[[NIPOPPY_DPATH_DERIVATIVES]]/freesurfer/7.3.2/output/[[NIPOPPY_BIDS_SESSION]]/sub-[[NIPOPPY_PARTICIPANT_ID]]" ] } } diff --git a/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-convert.json b/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-convert.json index d01e0d14..c73e7d65 100644 --- a/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-convert.json +++ b/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-convert.json @@ -22,7 +22,7 @@ "optional": true, "command-line-flag": "-d", "value-key": "[DICOM_DIR_TEMPLATE]", - "default-value": "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT]]/[[NIPOPPY_SESSION]]/*" + "default-value": "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT_ID]]/[[NIPOPPY_BIDS_SESSION]]/*" }, { "name": "files", @@ -41,7 +41,7 @@ "optional": true, "command-line-flag": "-s", "value-key": "[SUBJS]", - "default-value": "[[NIPOPPY_PARTICIPANT]]" + "default-value": "[[NIPOPPY_PARTICIPANT_ID]]" }, { "name": "converter", @@ -120,7 +120,7 @@ "optional": true, "command-line-flag": "-ss", "value-key": "[SESSION]", - "default-value": "[[NIPOPPY_SESSION]]" + "default-value": "[[NIPOPPY_BIDS_SESSION]]" }, { "name": "bids_options", diff --git a/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-prepare.json b/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-prepare.json index 81f61ca7..d0f32e40 100644 --- a/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-prepare.json +++ b/nipoppy_cli/nipoppy/data/descriptors/heudiconv-0.12.2-prepare.json @@ -22,7 +22,7 @@ "optional": true, "command-line-flag": "-d", "value-key": "[DICOM_DIR_TEMPLATE]", - "default-value": "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT]]/[[NIPOPPY_SESSION]]/*" + "default-value": "[[NIPOPPY_DPATH_SOURCEDATA]]/[[NIPOPPY_PARTICIPANT_ID]]/[[NIPOPPY_BIDS_SESSION]]/*" }, { "name": "files", @@ -41,7 +41,7 @@ "optional": true, "command-line-flag": "-s", "value-key": "[SUBJS]", - "default-value": "[[NIPOPPY_PARTICIPANT]]" + "default-value": "[[NIPOPPY_PARTICIPANT_ID]]" }, { "name": "converter", @@ -121,7 +121,7 @@ "optional": true, "command-line-flag": "-ss", "value-key": "[SESSION]", - "default-value": "[[NIPOPPY_SESSION]]" + "default-value": "[[NIPOPPY_BIDS_SESSION]]" }, { "name": "bids_options", diff --git a/nipoppy_cli/nipoppy/data/descriptors/mriqc-23.1.0.json b/nipoppy_cli/nipoppy/data/descriptors/mriqc-23.1.0.json index 16b81c2f..37d5b27f 100644 --- a/nipoppy_cli/nipoppy/data/descriptors/mriqc-23.1.0.json +++ b/nipoppy_cli/nipoppy/data/descriptors/mriqc-23.1.0.json @@ -82,7 +82,7 @@ "list": true, "command-line-flag": "--participant-label", "default-value": [ - "[[NIPOPPY_PARTICIPANT]]" + "[[NIPOPPY_PARTICIPANT_ID]]" ] }, { @@ -93,7 +93,7 @@ "type": "String", "value-key": "[SESSION_ID]", "command-line-flag": "--session-id", - "default-value": "[[NIPOPPY_SESSION_SHORT]]" + "default-value": "[[NIPOPPY_SESSION_ID]]" }, { "id": "run_id", diff --git a/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json b/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json index 18169506..1d94b209 100644 --- a/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json +++ b/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json @@ -1,12 +1,12 @@ { "DATASET_NAME": "", - "VISITS": [ + "VISIT_IDS": [ "", "" ], - "SESSIONS": [ - "ses-", - "ses-" + "SESSION_IDS": [ + "", + "" ], "SUBSTITUTIONS": { "[[NIPOPPY_DPATH_CONTAINERS]]": "", diff --git a/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json b/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json index 187dd2dc..84913130 100644 --- a/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json +++ b/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json @@ -1,12 +1,12 @@ { "DATASET_NAME": "", - "VISITS": [ + "VISIT_IDS": [ "", "" ], - "SESSIONS": [ - "ses-", - "ses-" + "SESSION_IDS": [ + "", + "" ], "SUBSTITUTIONS": { "[[NIPOPPY_DPATH_CONTAINERS]]": "", diff --git a/nipoppy_cli/nipoppy/data/examples/sample_manifest.csv b/nipoppy_cli/nipoppy/data/examples/sample_manifest.csv index 0411dba8..b45c6987 100644 --- a/nipoppy_cli/nipoppy/data/examples/sample_manifest.csv +++ b/nipoppy_cli/nipoppy/data/examples/sample_manifest.csv @@ -1,7 +1,7 @@ -participant_id,visit,session,datatype -01,BL,ses-BL,"['anat']" +participant_id,visit_id,session_id,datatype +01,BL,BL,"['anat']" 01,M06,, -01,M12,ses-M12,"['anat']" -02,BL,ses-BL,"['anat','dwi']" +01,M12,M12,"['anat']" +02,BL,BL,"['anat','dwi']" 02,M06,, -02,M12,ses-M12,"['anat','dwi']" +02,M12,M12,"['anat','dwi']" diff --git a/nipoppy_cli/nipoppy/data/examples/sample_tracker_configs/mriqc-23.1.0.json b/nipoppy_cli/nipoppy/data/examples/sample_tracker_configs/mriqc-23.1.0.json index d666c233..051cdd09 100644 --- a/nipoppy_cli/nipoppy/data/examples/sample_tracker_configs/mriqc-23.1.0.json +++ b/nipoppy_cli/nipoppy/data/examples/sample_tracker_configs/mriqc-23.1.0.json @@ -2,8 +2,8 @@ { "NAME": "pipeline_complete", "PATHS": [ - "[[NIPOPPY_BIDS_ID]]/[[NIPOPPY_SESSION]]/anat/[[NIPOPPY_BIDS_ID]]_[[NIPOPPY_SESSION]]_*_T1w.json", - "[[NIPOPPY_BIDS_ID]]_[[NIPOPPY_SESSION]]_*_T1w.html" + "[[NIPOPPY_BIDS_PARTICIPANT]]/[[NIPOPPY_BIDS_SESSION]]/anat/[[NIPOPPY_BIDS_PARTICIPANT]]_[[NIPOPPY_BIDS_SESSION]]_*_T1w.json", + "[[NIPOPPY_BIDS_PARTICIPANT]]_[[NIPOPPY_BIDS_SESSION]]_*_T1w.html" ] } ] diff --git a/nipoppy_cli/nipoppy/data/layouts/layout-0.1.0.json b/nipoppy_cli/nipoppy/data/layouts/layout-0.1.0.json index 58acf7b8..896122cd 100644 --- a/nipoppy_cli/nipoppy/data/layouts/layout-0.1.0.json +++ b/nipoppy_cli/nipoppy/data/layouts/layout-0.1.0.json @@ -55,7 +55,7 @@ "path": "scratch", "description": "This directory is for scratch data." }, - "dpath_raw_dicom": { + "dpath_raw_imaging": { "path": "scratch/raw_dicom", "description": "This directory is for raw, unorganized DICOM dumps." }, diff --git a/nipoppy_cli/nipoppy/data/layouts/layout-default.json b/nipoppy_cli/nipoppy/data/layouts/layout-default.json index e9c1a30f..a4ccbc53 100644 --- a/nipoppy_cli/nipoppy/data/layouts/layout-default.json +++ b/nipoppy_cli/nipoppy/data/layouts/layout-default.json @@ -55,9 +55,9 @@ "path": "scratch", "description": "This directory is for scratch data." }, - "dpath_raw_dicom": { - "path": "scratch/raw_dicom", - "description": "This directory is for raw, unorganized DICOM dumps." + "dpath_raw_imaging": { + "path": "scratch/raw_imaging", + "description": "This directory is for raw, unorganized imaging file dumps." }, "dpath_logs": { "path": "scratch/logs", diff --git a/nipoppy_cli/nipoppy/layout.py b/nipoppy_cli/nipoppy/layout.py index 3072ae5d..ae530369 100644 --- a/nipoppy_cli/nipoppy/layout.py +++ b/nipoppy_cli/nipoppy/layout.py @@ -90,8 +90,8 @@ class LayoutConfig(BaseModel): description="Directory for PyBIDS indexing configurations" ) dpath_scratch: DpathInfo = Field(description="Directory for temporary files") - dpath_raw_dicom: DpathInfo = Field( - description="Directory for raw, unorganized DICOM files" + dpath_raw_imaging: DpathInfo = Field( + description="Directory for raw, unorganized imaging files" ) dpath_logs: DpathInfo = Field(description="Directory for logs generated by Nipoppy") dpath_tabular: DpathInfo = Field(description="Directory for tabular data") @@ -185,7 +185,7 @@ def __init__( self.dpath_bids_db: Path self.dpath_bids_ignore_patterns: Path self.dpath_scratch: Path - self.dpath_raw_dicom: Path + self.dpath_raw_imaging: Path self.dpath_logs: Path self.dpath_tabular: Path self.dpath_assessments: Path @@ -270,8 +270,8 @@ def get_dpath_pipeline_work( self, pipeline_name: str, pipeline_version: str, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ) -> Path: """Return the path to a pipeline's working directory.""" return ( @@ -280,8 +280,8 @@ def get_dpath_pipeline_work( / get_pipeline_tag( pipeline_name, pipeline_version, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) ) @@ -303,12 +303,15 @@ def get_dpath_bids_db( self, pipeline_name: str, pipeline_version: str, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ) -> Path: """Return the path to a pipeline's BIDS database directory.""" dname = get_pipeline_tag( - pipeline_name, pipeline_version, participant=participant, session=session + pipeline_name, + pipeline_version, + participant_id=participant_id, + session_id=session_id, ) return self.dpath_bids_db / dname diff --git a/nipoppy_cli/nipoppy/tabular/bagel.py b/nipoppy_cli/nipoppy/tabular/bagel.py index a3a13358..78ffe4a1 100644 --- a/nipoppy_cli/nipoppy/tabular/bagel.py +++ b/nipoppy_cli/nipoppy/tabular/bagel.py @@ -5,7 +5,13 @@ from pydantic import Field, field_validator, model_validator from nipoppy.tabular.base import BaseTabular, BaseTabularModel -from nipoppy.utils import FIELD_DESCRIPTION_MAP, participant_id_to_bids_id +from nipoppy.utils import ( + FIELD_DESCRIPTION_MAP, + check_participant_id, + check_session_id, + participant_id_to_bids_participant, + session_id_to_bids_session, +) STATUS_SUCCESS = "SUCCESS" STATUS_FAIL = "FAIL" @@ -27,10 +33,16 @@ class BagelModel(BaseTabularModel): title="Participant ID", description=f"{FIELD_DESCRIPTION_MAP['participant_id']} (as in the manifest)", ) - bids_id: Optional[str] = Field( - default=None, title="BIDS ID", description=FIELD_DESCRIPTION_MAP["bids_id"] + bids_participant: Optional[str] = Field( + default=None, + title="BIDS participant ID", + description=FIELD_DESCRIPTION_MAP["bids_participant"], + ) + session_id: str = Field(description=FIELD_DESCRIPTION_MAP["session_id"]) + # TODO rename to bids_session (or remove) after updating digest + session: Optional[str] = Field( + default=None, description=FIELD_DESCRIPTION_MAP["bids_session"] ) - session: str = Field(description=FIELD_DESCRIPTION_MAP["session"]) pipeline_name: str = Field(description="The name of the pipeline being tracked") pipeline_version: str = Field( description="The version of the pipeline being tracked" @@ -56,10 +68,17 @@ def check_status(cls, value: str): return value @model_validator(mode="after") - def check_bids_id(self): - """Generate default value for optional BIDS ID field.""" - if self.bids_id is None: - self.bids_id = participant_id_to_bids_id(self.participant_id) + def validate_after(self): + """Check fields.""" + check_participant_id(self.participant_id, raise_error=True) + check_session_id(self.session_id, raise_error=True) + + if self.bids_participant is None: + self.bids_participant = participant_id_to_bids_participant( + self.participant_id + ) + if self.session is None: + self.session = session_id_to_bids_session(self.session_id) class Bagel(BaseTabular): @@ -67,8 +86,9 @@ class Bagel(BaseTabular): # column names col_participant_id = "participant_id" - col_bids_id = "bids_id" - col_session = "session" + col_bids_participant = "bids_participant" + col_session_id = "session_id" + col_bids_session = "session" col_pipeline_name = "pipeline_name" col_pipeline_version = "pipeline_version" col_pipeline_complete = "pipeline_complete" @@ -82,8 +102,8 @@ class Bagel(BaseTabular): # for sorting/comparing between bagels index_cols = [ col_participant_id, - col_bids_id, - col_session, + col_bids_participant, + col_session_id, col_pipeline_name, col_pipeline_version, ] @@ -91,7 +111,7 @@ class Bagel(BaseTabular): _metadata = BaseTabular._metadata + [ "col_participant_id", "col_bids_id", - "col_session", + "col_session_id", "col_pipeline_name", "col_pipeline_version", "col_pipeline_complete", @@ -109,31 +129,31 @@ def get_completed_participants_sessions( self, pipeline_name: str, pipeline_version: str, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ): """ Get participant-session pairs that have successfully completed a pipeline run. Can optionally filter within a specific participant and/or session. """ - if participant is None: - participants = set(self[self.col_participant_id]) + if participant_id is None: + participant_ids = set(self[self.col_participant_id]) else: - participants = {participant} - if session is None: - session = set(self[self.col_session]) + participant_ids = {participant_id} + if session_id is None: + session_ids = set(self[self.col_session_id]) else: - session = {session} + session_ids = {session_id} bagel_subset = self.loc[ (self[self.col_pipeline_name] == pipeline_name) & (self[self.col_pipeline_version] == pipeline_version) - & (self[self.col_participant_id].isin(participants)) - & (self[self.col_session].isin(session)) + & (self[self.col_participant_id].isin(participant_ids)) + & (self[self.col_session_id].isin(session_ids)) & (self[self.col_pipeline_complete] == self.status_success) ] - yield from bagel_subset[[self.col_participant_id, self.col_session]].itertuples( - index=False - ) + yield from bagel_subset[ + [self.col_participant_id, self.col_session_id] + ].itertuples(index=False) diff --git a/nipoppy_cli/nipoppy/tabular/dicom_dir_map.py b/nipoppy_cli/nipoppy/tabular/dicom_dir_map.py index 71e620b8..8bf7469b 100644 --- a/nipoppy_cli/nipoppy/tabular/dicom_dir_map.py +++ b/nipoppy_cli/nipoppy/tabular/dicom_dir_map.py @@ -11,11 +11,7 @@ from nipoppy.layout import DEFAULT_LAYOUT_INFO from nipoppy.tabular.base import BaseTabular, BaseTabularModel from nipoppy.tabular.manifest import Manifest -from nipoppy.utils import ( - BIDS_SESSION_PREFIX, - BIDS_SUBJECT_PREFIX, - FIELD_DESCRIPTION_MAP, -) +from nipoppy.utils import FIELD_DESCRIPTION_MAP, check_participant_id, check_session_id class DicomDirMapModel(BaseTabularModel): @@ -29,28 +25,20 @@ class DicomDirMapModel(BaseTabularModel): participant_id: str = Field( title="Participant ID", description=FIELD_DESCRIPTION_MAP["participant_id"] ) - session: str = Field(description=FIELD_DESCRIPTION_MAP["session"]) + session_id: str = Field(description=FIELD_DESCRIPTION_MAP["session_id"]) participant_dicom_dir: str = Field( title="Participant's raw DICOM directory", description=( "Path to the participant's raw DICOM directory, relative to the dataset's" - f"raw DICOM directory (default: {DEFAULT_LAYOUT_INFO.dpath_raw_dicom})" + f"raw DICOM directory (default: {DEFAULT_LAYOUT_INFO.dpath_raw_imaging})" ), ) @model_validator(mode="after") def validate_after(self) -> Self: """Validate participant_id and session fields.""" - if self.participant_id.startswith(BIDS_SUBJECT_PREFIX): - raise ValueError( - f'Participant ID should not start with "{BIDS_SUBJECT_PREFIX}"' - f", got {self.participant_id}" - ) - if not self.session.startswith(BIDS_SESSION_PREFIX): - raise ValueError( - f'Session should start with "{BIDS_SESSION_PREFIX}"' - f", got {self.session}" - ) + check_participant_id(self.participant_id, raise_error=True) + check_session_id(self.session_id, raise_error=True) return self @@ -63,17 +51,17 @@ class DicomDirMap(BaseTabular): # column names col_participant_id = "participant_id" - col_session = "session" + col_session_id = "session_id" col_participant_dicom_dir = "participant_dicom_dir" - index_cols = [col_participant_id, col_session] + index_cols = [col_participant_id, col_session_id] # set the model model = DicomDirMapModel _metadata = BaseTabular._metadata + [ "col_participant_id", - "col_session", + "col_session_id", "col_participant_dicom_dir", "index_cols", "model", @@ -116,15 +104,15 @@ def load_or_generate( # else depends on participant_first or no else: data_dicom_dir_map = [] - for participant, session in manifest.get_participants_sessions(): + for participant_id, session_id in manifest.get_participants_sessions(): if participant_first is not False: - participant_dicom_dir = f"{participant}/{session}" + participant_dicom_dir = f"{participant_id}/{session_id}" else: - participant_dicom_dir = f"{session}/{participant}" + participant_dicom_dir = f"{session_id}/{participant_id}" data_dicom_dir_map.append( { - cls.col_participant_id: participant, - cls.col_session: session, + cls.col_participant_id: participant_id, + cls.col_session_id: session_id, cls.col_participant_dicom_dir: participant_dicom_dir, } ) @@ -133,14 +121,14 @@ def load_or_generate( dicom_dir_map.validate() return dicom_dir_map - def get_dicom_dir(self, participant: str, session: str) -> str: + def get_dicom_dir(self, participant_id: str, session_id: str) -> str: """Return the participant's raw DICOM directory for a given session. Parameters ---------- - participant : str + participant_id : str Participant ID, without the BIDS prefix - session : str + session_id : str Session, with the BIDS prefix """ - return self.set_index(self.index_cols).loc[participant, session].item() + return self.set_index(self.index_cols).loc[participant_id, session_id].item() diff --git a/nipoppy_cli/nipoppy/tabular/doughnut.py b/nipoppy_cli/nipoppy/tabular/doughnut.py index 486d0b35..e2234e46 100644 --- a/nipoppy_cli/nipoppy/tabular/doughnut.py +++ b/nipoppy_cli/nipoppy/tabular/doughnut.py @@ -13,10 +13,9 @@ from nipoppy.tabular.dicom_dir_map import DicomDirMap from nipoppy.tabular.manifest import Manifest, ManifestModel from nipoppy.utils import ( - FIELD_DESCRIPTION_MAP, StrOrPathLike, - participant_id_to_bids_id, - participant_id_to_dicom_id, + participant_id_to_bids_participant, + session_id_to_bids_session, ) @@ -38,16 +37,11 @@ class DoughnutModel(ManifestModel): "relative to the raw data directory" ), ) - dicom_id: str = Field( - title="DICOM ID", - description="Participant identifier used in DICOM file names/paths", - ) - bids_id: str = Field(title="BIDS ID", description=FIELD_DESCRIPTION_MAP["bids_id"]) - downloaded: bool = Field(description="Whether files are available on disk") - organized: bool = Field( + in_raw_imaging: bool = Field(description="Whether files are available on disk") + in_sourcedata: bool = Field( description="Whether files have been organized in the sourcedata directory" ) - bidsified: bool = Field( + in_bids: bool = Field( title="BIDSified", description="Whether files have been converted to BIDS" ) @@ -57,26 +51,22 @@ class Doughnut(Manifest): # column names col_participant_dicom_dir = "participant_dicom_dir" - col_dicom_id = "dicom_id" - col_bids_id = "bids_id" - col_downloaded = "downloaded" - col_organized = "organized" - col_bidsified = "bidsified" + col_in_raw_imaging = "in_raw_imaging" + col_in_sourcedata = "in_sourcedata" + col_in_bids = "in_bids" - status_cols = [col_downloaded, col_organized, col_bidsified] + status_cols = [col_in_raw_imaging, col_in_sourcedata, col_in_bids] # set the model model = DoughnutModel - index_cols = [Manifest.col_participant_id, Manifest.col_session] + index_cols = [Manifest.col_participant_id, Manifest.col_session_id] _metadata = Manifest._metadata + [ "col_participant_dicom_dir", - "col_dicom_id", - "col_bids_id", - "col_downloaded", - "col_organized", - "col_bidsified", + "col_in_raw_imaging", + "col_in_sourcedata", + "col_in_bids", ] @classmethod @@ -93,61 +83,63 @@ def _check_status_value(cls, value: bool) -> bool: raise ValueError(f"Invalid status value: {value}. Must be a boolean") return value - def get_status(self, participant: str, session: str, col: str) -> bool: + def get_status(self, participant_id: str, session_id: str, col: str) -> bool: """Get one of the statuses for an existing record.""" col = self._check_status_col(col) - return self.set_index(self.index_cols).loc[(participant, session), col] + return self.set_index(self.index_cols).loc[(participant_id, session_id), col] def set_status( - self, participant: str, session: str, col: str, status: bool + self, participant_id: str, session_id: str, col: str, status: bool ) -> Self: """Set one of the statuses for an existing record.""" col = self._check_status_col(col) status = self._check_status_value(status) self.set_index(self.index_cols, inplace=True) - self.loc[(participant, session), col] = status + self.loc[(participant_id, session_id), col] = status return self.reset_index(inplace=True) def _get_participant_sessions_helper( self, status_col: str, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ): """Get subset of participants/sessions based on a status column.""" doughnut_subset: Doughnut = self.loc[self[status_col]] return doughnut_subset.get_participants_sessions( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) def get_downloaded_participants_sessions( self, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ): """Get participants and sessions with downloaded data.""" return self._get_participant_sessions_helper( - self.col_downloaded, participant=participant, session=session + self.col_in_raw_imaging, + participant_id=participant_id, + session_id=session_id, ) def get_organized_participants_sessions( self, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ): """Get participants and sessions with organized data.""" return self._get_participant_sessions_helper( - self.col_organized, participant=participant, session=session + self.col_in_sourcedata, participant_id=participant_id, session_id=session_id ) def get_bidsified_participants_sessions( self, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ): """Get participants and sessions with BIDS data.""" return self._get_participant_sessions_helper( - self.col_bidsified, participant=participant, session=session + self.col_in_bids, participant_id=participant_id, session_id=session_id ) @@ -171,7 +163,7 @@ def check_status( status = False else: dpath = Path(dpath) - dpath_participant = dpath / dname_subdirectory + dpath_participant: Path = dpath / dname_subdirectory if dpath_participant.exists(): status = next(dpath_participant.iterdir(), None) is not None else: @@ -189,17 +181,17 @@ def check_status( doughnut_records = [] for _, manifest_record in manifest_imaging_only.iterrows(): - participant = manifest_record[manifest.col_participant_id] - session = manifest_record[manifest.col_session] + participant_id = manifest_record[manifest.col_participant_id] + session_id = manifest_record[manifest.col_session_id] # get DICOM dir participant_dicom_dir = dicom_dir_map.get_dicom_dir( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) - # get DICOM and BIDS IDs - dicom_id = participant_id_to_dicom_id(participant) - bids_id = participant_id_to_bids_id(participant) + # get BIDS IDs + bids_participant = participant_id_to_bids_participant(participant_id) + bids_session = session_id_to_bids_session(session_id) if empty: status_downloaded = False @@ -211,24 +203,24 @@ def check_status( dname_subdirectory=participant_dicom_dir, ) status_organized = check_status( - dpath=dpath_organized, dname_subdirectory=Path(dicom_id, session) + dpath=dpath_organized, + dname_subdirectory=Path(bids_participant, bids_session), ) status_bidsified = check_status( - dpath=dpath_bidsified, dname_subdirectory=Path(bids_id, session) + dpath=dpath_bidsified, + dname_subdirectory=Path(bids_participant, bids_session), ) doughnut_records.append( { - Doughnut.col_participant_id: participant, - Doughnut.col_visit: manifest_record[Manifest.col_visit], - Doughnut.col_session: session, + Doughnut.col_participant_id: participant_id, + Doughnut.col_visit_id: manifest_record[Manifest.col_visit_id], + Doughnut.col_session_id: session_id, Doughnut.col_datatype: manifest_record[Manifest.col_datatype], Doughnut.col_participant_dicom_dir: participant_dicom_dir, - Doughnut.col_dicom_id: dicom_id, - Doughnut.col_bids_id: bids_id, - Doughnut.col_downloaded: status_downloaded, - Doughnut.col_organized: status_organized, - Doughnut.col_bidsified: status_bidsified, + Doughnut.col_in_raw_imaging: status_downloaded, + Doughnut.col_in_sourcedata: status_organized, + Doughnut.col_in_bids: status_bidsified, } ) diff --git a/nipoppy_cli/nipoppy/tabular/manifest.py b/nipoppy_cli/nipoppy/tabular/manifest.py index 740c8a93..68374631 100644 --- a/nipoppy_cli/nipoppy/tabular/manifest.py +++ b/nipoppy_cli/nipoppy/tabular/manifest.py @@ -9,11 +9,7 @@ from typing_extensions import Self from nipoppy.tabular.base import BaseTabular, BaseTabularModel -from nipoppy.utils import ( - FIELD_DESCRIPTION_MAP, - check_participant_id_strict, - check_session_strict, -) +from nipoppy.utils import FIELD_DESCRIPTION_MAP, check_participant_id, check_session_id class ManifestModel(BaseTabularModel): @@ -27,8 +23,8 @@ class ManifestModel(BaseTabularModel): participant_id: str = Field( title="Participant ID", description=FIELD_DESCRIPTION_MAP["participant_id"] ) - visit: str = Field(description=FIELD_DESCRIPTION_MAP["visit"]) - session: Optional[str] = Field(description=FIELD_DESCRIPTION_MAP["session"]) + visit_id: str = Field(description=FIELD_DESCRIPTION_MAP["visit_id"]) + session_id: Optional[str] = Field(description=FIELD_DESCRIPTION_MAP["session_id"]) datatype: Optional[list[str]] = Field( description=( "Imaging datatype, as recognized by BIDS (see " @@ -54,8 +50,8 @@ def _validate_before_fields(cls, data: dict): @model_validator(mode="after") def validate_after(self) -> Self: """Validate fields after instance creation.""" - check_participant_id_strict(self.participant_id) - check_session_strict(self.session) + check_participant_id(self.participant_id, raise_error=True) + check_session_id(self.session_id, raise_error=True) return self # allow extra columns @@ -67,44 +63,46 @@ class Manifest(BaseTabular): # column names col_participant_id = "participant_id" - col_visit = "visit" - col_session = "session" + col_visit_id = "visit_id" + col_session_id = "session_id" col_datatype = "datatype" - index_cols = [col_participant_id, col_visit] + index_cols = [col_participant_id, col_visit_id] # set the model model = ManifestModel _metadata = BaseTabular._metadata + [ "col_participant_id", - "col_visit", - "col_session", + "col_visit_id", + "col_session_id", "col_datatype", "index_cols", "model", ] @classmethod - def load(cls, *args, sessions=None, visits=None, validate=True, **kwargs) -> Self: + def load( + cls, *args, session_ids=None, visit_ids=None, validate=True, **kwargs + ) -> Self: """Load the manifest.""" manifest = super().load(*args, validate=validate, **kwargs) - manifest.sessions = sessions - manifest.visits = visits + manifest.session_ids = session_ids + manifest.visit_ids = visit_ids return manifest - def __init__(self, *args, sessions=None, visits=None, **kwargs) -> None: + def __init__(self, *args, session_ids=None, visit_ids=None, **kwargs) -> None: super().__init__(*args, **kwargs) - self.sessions = sessions - self.visits = visits + self.session_ids = session_ids + self.visit_ids = visit_ids def validate(self, *args, **kwargs) -> Self: """Validate the manifest.""" manifest = super().validate(*args, **kwargs) - if self.sessions is not None: - self._check_values(self.col_session, self.sessions) - if self.visits is not None: - self._check_values(self.col_visit, self.visits) + if self.session_ids is not None: + self._check_values(self.col_session_id, self.session_ids) + if self.visit_ids is not None: + self._check_values(self.col_visit_id, self.visit_ids) return manifest def _check_values(self, col, allowed_values) -> Self: @@ -117,32 +115,32 @@ def _check_values(self, col, allowed_values) -> Self: ) return self - def get_imaging_subset(self, session: Optional[str] = None): + def get_imaging_subset(self, session_id: Optional[str] = None): """Get records with imaging data.""" - manifest = self[self[self.col_session].notna()] - if session is not None: - return manifest[manifest[self.col_session] == session] + manifest = self[self[self.col_session_id].notna()] + if session_id is not None: + return manifest[manifest[self.col_session_id] == session_id] return manifest def get_participants_sessions( - self, participant: Optional[str] = None, session: Optional[str] = None + self, participant_id: Optional[str] = None, session_id: Optional[str] = None ): - """Get participants and sessions.""" - if participant is None: - participants = set(self[self.col_participant_id]) + """Get participant IDs and session IDs.""" + if participant_id is None: + participant_ids = set(self[self.col_participant_id]) else: - participants = {participant} - if session is None: - sessions = self[self.col_session] - sessions = set(sessions[sessions.notna()]) + participant_ids = {participant_id} + if session_id is None: + session_ids = self[self.col_session_id] + session_ids = set(session_ids[session_ids.notna()]) else: - sessions = {session} + session_ids = {session_id} manifest_subset = self[ - (self[self.col_participant_id].isin(participants)) - & (self[self.col_session].isin(sessions)) + (self[self.col_participant_id].isin(participant_ids)) + & (self[self.col_session_id].isin(session_ids)) ] yield from manifest_subset[ - [self.col_participant_id, self.col_session] + [self.col_participant_id, self.col_session_id] ].itertuples(index=False) diff --git a/nipoppy_cli/nipoppy/utils.py b/nipoppy_cli/nipoppy/utils.py index d4478e9d..f6cc121f 100644 --- a/nipoppy_cli/nipoppy/utils.py +++ b/nipoppy_cli/nipoppy/utils.py @@ -36,87 +36,96 @@ # descriptions for common fields in the Pydantic models FIELD_DESCRIPTION_MAP = { - "bids_id": "BIDS-compliant participant identifier (e.g., sub-01)", - "participant_id": "Participant identifier", - "session": "BIDS-compliant identifier imaging session (e.g., ses-1)", - "visit": "Visit identifier", + "participant_id": "Participant identifier, without the BIDS prefix", + "session_id": "Imaging session identifier, without the BIDS prefix", + "bids_participant": "Participant identifier with BIDS prefix (e.g., sub-01)", + "bids_session": "Imaging session identifier with BIDS prefix (e.g., ses-01)", + "visit_id": "Visit identifier", } -def participant_id_to_dicom_id(participant_id: str): - """Convert a participant ID to a BIDS-compatible DICOM ID.""" - # keep only alphanumeric characters - participant_id = str(participant_id) - dicom_id = "".join(filter(str.isalnum, participant_id)) - return dicom_id +def participant_id_to_bids_participant(participant_id: str) -> str: + """Add the BIDS prefix to a participant ID.""" + return f"{BIDS_SUBJECT_PREFIX}{participant_id}" -def dicom_id_to_bids_id(dicom_id: str): - """Add the BIDS prefix to a DICOM ID.""" - return f"{BIDS_SUBJECT_PREFIX}{dicom_id}" +def session_id_to_bids_session(session_id: Optional[str]) -> str: + """ + Add the BIDS prefix to a session ID. + If session_id is None, returns None. + """ + if session_id is None: + return session_id -def participant_id_to_bids_id(participant_id: str): - """Convert a participant ID to a BIDS-compatible participant ID.""" - bids_id = dicom_id_to_bids_id(participant_id_to_dicom_id(participant_id)) - return bids_id + return f"{BIDS_SESSION_PREFIX}{session_id}" -def check_participant(participant: Optional[str]): - """Check/process a participant string.""" - if participant is None: - return participant +def check_participant_id(participant_id: Optional[str], raise_error=False): + """Make sure a participant ID does not have the BIDS prefix. - # remove the BIDS prefix if it exists - return str(participant).removeprefix(BIDS_SUBJECT_PREFIX) + Parameters + ---------- + participant_id : Optional[str] + The participant ID to check. If None, returns None. + strict : bool, optional + Whether to raise an error if the participant ID has the prefix, by default False + Returns + ------- + str + The participant ID without the BIDS prefix -def check_participant_id_strict(participant_id: str): + Raises + ------ + ValueError """ - Make sure participant_id does not have the BIDS prefix. + if participant_id is None: + return participant_id - To use when validating user-provided files (e.g. the manifest). - """ if participant_id.startswith(BIDS_SUBJECT_PREFIX): - raise ValueError( - f'Participant ID should not start with "{BIDS_SUBJECT_PREFIX}"' - f", got {participant_id}" - ) - return participant_id + if raise_error: + raise ValueError( + f'Participant ID should not start with "{BIDS_SUBJECT_PREFIX}"' + f", got {participant_id}" + ) + else: + return participant_id.removeprefix(BIDS_SUBJECT_PREFIX) + return participant_id -def check_session(session: Optional[str]): - """Check/process a session string.""" - if session is None: - return session - # add BIDS prefix if it doesn't already exist - session = str(session) - if session.startswith(BIDS_SESSION_PREFIX): - return session - else: - return f"{BIDS_SESSION_PREFIX}{session}" +def check_session_id(session_id: Optional[str], raise_error=False): + """Make sure a session ID does not have the BIDS prefix. + Parameters + ---------- + session_id : Optional[str] + The session ID to check. If None, returns None. + strict : bool, optional + Whether to raise an error if the session ID has the prefix, by default False -def check_session_strict(session: Optional[str]): - """ - Make sure session has the BIDS prefix. + Returns + ------- + str + The session ID without the BIDS prefix - To use when validating user-provided files (e.g. the manifest). + Raises + ------ + ValueError """ - if session is not None and not session.startswith(BIDS_SESSION_PREFIX): - raise ValueError( - f'Session should start with "{BIDS_SESSION_PREFIX}"' f", got {session}" - ) - return session - + if session_id is None: + return session_id -def strip_session(session: Optional[str]): - """Strip the BIDS prefix from a session string.""" - if session is None: - return session - session = str(session) - return session.removeprefix(BIDS_SESSION_PREFIX) + if session_id.startswith(BIDS_SESSION_PREFIX): + if raise_error: + raise ValueError( + f'Session ID should not start with "{BIDS_SESSION_PREFIX}"' + f", got {session_id}" + ) + else: + return session_id.removeprefix(BIDS_SESSION_PREFIX) + return session_id def create_bids_db( @@ -167,18 +176,18 @@ def get_pipeline_tag( pipeline_name: str, pipeline_version: str, pipeline_step: Optional[str] = None, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, sep="-", ): """Generate a tag for a pipeline.""" components = [pipeline_name, pipeline_version] if pipeline_step is not None: components.append(pipeline_step) - if participant is not None: - components.append(participant) - if session is not None: - components.append(strip_session(session)) + if participant_id is not None: + components.append(participant_id) + if session_id is not None: + components.append(session_id) return sep.join(components) diff --git a/nipoppy_cli/nipoppy/workflows/base.py b/nipoppy_cli/nipoppy/workflows/base.py index 6df672fc..e5438bd2 100644 --- a/nipoppy_cli/nipoppy/workflows/base.py +++ b/nipoppy_cli/nipoppy/workflows/base.py @@ -292,13 +292,13 @@ def config(self) -> Config: def manifest(self) -> Manifest: """Load the manifest.""" fpath_manifest = Path(self.layout.fpath_manifest) - expected_sessions = self.config.SESSIONS - expected_visits = self.config.VISITS + expected_session_ids = self.config.SESSION_IDS + expected_visit_ids = self.config.VISIT_IDS try: return Manifest.load( fpath_manifest, - sessions=expected_sessions, - visits=expected_visits, + session_ids=expected_session_ids, + visit_ids=expected_visit_ids, ) except FileNotFoundError: raise FileNotFoundError(f"Manifest file not found: {fpath_manifest}") @@ -318,7 +318,7 @@ def doughnut(self) -> Doughnut: doughnut = generate_doughnut( manifest=self.manifest, dicom_dir_map=self.dicom_dir_map, - dpath_downloaded=self.layout.dpath_raw_dicom, + dpath_downloaded=self.layout.dpath_raw_imaging, dpath_organized=self.layout.dpath_sourcedata, dpath_bidsified=self.layout.dpath_bids, empty=False, diff --git a/nipoppy_cli/nipoppy/workflows/bids_conversion.py b/nipoppy_cli/nipoppy/workflows/bids_conversion.py index 2651f843..78d9eebf 100644 --- a/nipoppy_cli/nipoppy/workflows/bids_conversion.py +++ b/nipoppy_cli/nipoppy/workflows/bids_conversion.py @@ -21,8 +21,8 @@ def __init__( pipeline_name: str, pipeline_version: Optional[str] = None, pipeline_step: Optional[str] = None, - participant: str = None, - session: str = None, + participant_id: str = None, + session_id: str = None, simulate: bool = False, fpath_layout: Optional[StrOrPathLike] = None, logger: Optional[logging.Logger] = None, @@ -33,8 +33,8 @@ def __init__( pipeline_name=pipeline_name, pipeline_version=pipeline_version, pipeline_step=pipeline_step, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, simulate=simulate, fpath_layout=fpath_layout, logger=logger, @@ -57,26 +57,26 @@ def pipeline_config(self) -> PipelineConfig: ) def get_participants_sessions_to_run( - self, participant: Optional[str], session: Optional[str] + self, participant_id: Optional[str], session_id: Optional[str] ): """Return participant-session pairs to run the pipeline on.""" participants_sessions_bidsified = set( self.doughnut.get_bidsified_participants_sessions( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) ) for participant_session in self.doughnut.get_organized_participants_sessions( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ): if participant_session not in participants_sessions_bidsified: yield participant_session - def run_single(self, participant: str, session: str): + def run_single(self, participant_id: str, session_id: str): """Run BIDS conversion on a single participant/session.""" # get container command container_command = self.process_container_config( - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, bind_paths=[ self.layout.dpath_sourcedata, self.layout.dpath_bids, @@ -84,14 +84,16 @@ def run_single(self, participant: str, session: str): ) # run pipeline with Boutiques - self.launch_boutiques_run( - participant, session, container_command=container_command + invocation_and_descriptor = self.launch_boutiques_run( + participant_id, session_id, container_command=container_command ) # update status self.doughnut.set_status( - participant=participant, - session=session, - col=self.doughnut.col_bidsified, + participant_id=participant_id, + session_id=session_id, + col=self.doughnut.col_in_bids, status=True, ) + + return invocation_and_descriptor diff --git a/nipoppy_cli/nipoppy/workflows/dicom_reorg.py b/nipoppy_cli/nipoppy/workflows/dicom_reorg.py index 9f5463a3..c0b671f8 100644 --- a/nipoppy_cli/nipoppy/workflows/dicom_reorg.py +++ b/nipoppy_cli/nipoppy/workflows/dicom_reorg.py @@ -8,7 +8,11 @@ import pydicom from nipoppy.tabular.doughnut import update_doughnut -from nipoppy.utils import StrOrPathLike +from nipoppy.utils import ( + StrOrPathLike, + participant_id_to_bids_participant, + session_id_to_bids_session, +) from nipoppy.workflows.base import BaseWorkflow @@ -48,20 +52,22 @@ def __init__( def get_fpaths_to_reorg( self, - participant: str, - session: str, + participant_id: str, + session_id: str, ) -> list[Path]: """Get file paths to reorganize for a single participant and session.""" dpath_downloaded = ( - self.layout.dpath_raw_dicom - / self.dicom_dir_map.get_dicom_dir(participant=participant, session=session) + self.layout.dpath_raw_imaging + / self.dicom_dir_map.get_dicom_dir( + participant_id=participant_id, session_id=session_id + ) ) # make sure directory exists if not dpath_downloaded.exists(): raise FileNotFoundError( - f"Raw DICOM directory not found for participant {participant}" - f" session {session}: {dpath_downloaded}" + f"Raw DICOM directory not found for participant {participant_id}" + f" session {session_id}: {dpath_downloaded}" ) # crawl through directory tree and get all file paths @@ -71,7 +77,7 @@ def get_fpaths_to_reorg( return fpaths def apply_fname_mapping( - self, fname_source: str, participant: str, session: str + self, fname_source: str, participant_id: str, session_id: str ) -> str: """ Apply a mapping to the file name. @@ -82,14 +88,19 @@ def apply_fname_mapping( """ return fname_source - def run_single(self, participant: str, session: str): + def run_single(self, participant_id: str, session_id: str): """Reorganize downloaded DICOM files for a single participant and session.""" # get paths to reorganize - fpaths_to_reorg = self.get_fpaths_to_reorg(participant, session) + fpaths_to_reorg = self.get_fpaths_to_reorg(participant_id, session_id) - # do reorg - dpath_reorganized: Path = self.layout.dpath_sourcedata / participant / session + dpath_reorganized: Path = ( + self.layout.dpath_sourcedata + / participant_id_to_bids_participant(participant_id) + / session_id_to_bids_session(session_id) + ) self.mkdir(dpath_reorganized) + + # do reorg for fpath_source in fpaths_to_reorg: # check file (though only error out if DICOM cannot be read) if self.check_dicoms: @@ -104,7 +115,7 @@ def run_single(self, participant: str, session: str): ) fpath_dest = dpath_reorganized / self.apply_fname_mapping( - fpath_source.name, participant=participant, session=session + fpath_source.name, participant_id=participant_id, session_id=session_id ) # do not overwrite existing files @@ -124,9 +135,9 @@ def run_single(self, participant: str, session: str): # update doughnut entry self.doughnut.set_status( - participant=participant, - session=session, - col=self.doughnut.col_organized, + participant_id=participant_id, + session_id=session_id, + col=self.doughnut.col_in_sourcedata, status=True, ) @@ -145,7 +156,7 @@ def run_setup(self): doughnut=self.doughnut, manifest=self.manifest, dicom_dir_map=self.dicom_dir_map, - dpath_downloaded=self.layout.dpath_raw_dicom, + dpath_downloaded=self.layout.dpath_raw_imaging, dpath_organized=self.layout.dpath_sourcedata, dpath_bidsified=self.layout.dpath_bids, logger=self.logger, @@ -154,13 +165,13 @@ def run_setup(self): def run_main(self): """Reorganize all downloaded DICOM files.""" for ( - participant, - session, + participant_id, + session_id, ) in self.get_participants_sessions_to_run(): try: - self.run_single(participant, session) + self.run_single(participant_id, session_id) except Exception as exception: self.logger.error( - "Error reorganizing DICOM files" - f" for participant {participant} session {session}: {exception}" + "Error reorganizing DICOM files for participant " + f"{participant_id} session {session_id}: {exception}" ) diff --git a/nipoppy_cli/nipoppy/workflows/doughnut.py b/nipoppy_cli/nipoppy/workflows/doughnut.py index 137f9a77..3e6fd4ef 100644 --- a/nipoppy_cli/nipoppy/workflows/doughnut.py +++ b/nipoppy_cli/nipoppy/workflows/doughnut.py @@ -36,7 +36,7 @@ def __init__( def run_main(self): """Generate/update the dataset's doughnut file.""" fpath_doughnut = self.layout.fpath_doughnut - dpath_downloaded = self.layout.dpath_raw_dicom + dpath_downloaded = self.layout.dpath_raw_imaging dpath_organized = self.layout.dpath_sourcedata dpath_bidsified = self.layout.dpath_bids empty = self.empty diff --git a/nipoppy_cli/nipoppy/workflows/pipeline.py b/nipoppy_cli/nipoppy/workflows/pipeline.py index d41937f8..c92863d5 100644 --- a/nipoppy_cli/nipoppy/workflows/pipeline.py +++ b/nipoppy_cli/nipoppy/workflows/pipeline.py @@ -23,14 +23,14 @@ BIDS_SUBJECT_PREFIX, StrOrPathLike, add_pybids_ignore_patterns, - check_participant, - check_session, + check_participant_id, + check_session_id, create_bids_db, get_pipeline_tag, load_json, - participant_id_to_bids_id, + participant_id_to_bids_participant, process_template_str, - strip_session, + session_id_to_bids_session, ) from nipoppy.workflows.base import BaseWorkflow @@ -45,8 +45,8 @@ def __init__( pipeline_name: str, pipeline_version: Optional[str] = None, pipeline_step: Optional[str] = None, - participant: str = None, - session: str = None, + participant_id: str = None, + session_id: str = None, fpath_layout: Optional[StrOrPathLike] = None, logger: Optional[logging.Logger] = None, dry_run=False, @@ -61,8 +61,8 @@ def __init__( self.pipeline_name = pipeline_name self.pipeline_version = pipeline_version self.pipeline_step = pipeline_step - self.participant = check_participant(participant) - self.session = check_session(session) + self.participant_id = check_participant_id(participant_id) + self.session_id = check_session_id(session_id) @cached_property def dpaths_to_check(self) -> list[Path]: @@ -90,8 +90,8 @@ def dpath_pipeline_work(self) -> Path: return self.layout.get_dpath_pipeline_work( pipeline_name=self.pipeline_name, pipeline_version=self.pipeline_version, - participant=self.participant, - session=self.session, + participant_id=self.participant_id, + session_id=self.session_id, ) @cached_property @@ -100,8 +100,8 @@ def dpath_pipeline_bids_db(self) -> Path: return self.layout.get_dpath_bids_db( pipeline_name=self.pipeline_name, pipeline_version=self.pipeline_version, - participant=self.participant, - session=self.session, + participant_id=self.participant_id, + session_id=self.session_id, ) @cached_property @@ -217,35 +217,35 @@ def boutiques_config(self): def process_template_json( self, template_json: dict, - participant: str, - session: str, - bids_id: Optional[str] = None, - session_short: Optional[str] = None, + participant_id: str, + session_id: str, + bids_participant: Optional[str] = None, + bids_session: Optional[str] = None, objs: Optional[list] = None, return_str: bool = False, **kwargs, ): """Replace template strings in a JSON object.""" - if not (isinstance(participant, str) and isinstance(session, str)): + if not (isinstance(participant_id, str) and isinstance(session_id, str)): raise ValueError( - "participant and session must be strings" - f", got {participant} ({type(participant)})" - f" and {session} ({type(session)})" + "participant_id and session_id must be strings" + f", got {participant_id} ({type(participant_id)})" + f" and {session_id} ({type(session_id)})" ) - if bids_id is None: - bids_id = participant_id_to_bids_id(participant) - if session_short is None: - session_short = strip_session(session) + if bids_participant is None: + bids_participant = participant_id_to_bids_participant(participant_id) + if bids_session is None: + bids_session = session_id_to_bids_session(session_id) if objs is None: objs = [] objs.extend([self, self.layout]) - kwargs["participant"] = participant - kwargs["session"] = session - kwargs["bids_id"] = bids_id - kwargs["session_short"] = session_short + kwargs["participant_id"] = participant_id + kwargs["session_id"] = session_id + kwargs["bids_participant"] = bids_participant + kwargs["bids_session"] = bids_session self.logger.debug("Available replacement strings: ") max_len = max(len(k) for k in kwargs) @@ -264,21 +264,21 @@ def process_template_json( def set_up_bids_db( self, dpath_bids_db: StrOrPathLike, - participant: Optional[str] = None, - session: Optional[str] = None, + participant_id: Optional[str] = None, + session_id: Optional[str] = None, ) -> bids.BIDSLayout: """Set up the BIDS database.""" dpath_bids_db: Path = Path(dpath_bids_db) - if participant is not None: + if participant_id is not None: add_pybids_ignore_patterns( current=self.pybids_ignore_patterns, - new=f"^(?!/{BIDS_SUBJECT_PREFIX}({participant}))", + new=f"^(?!/{BIDS_SUBJECT_PREFIX}({participant_id}))", ) - if session is not None: + if session_id is not None: add_pybids_ignore_patterns( current=self.pybids_ignore_patterns, - new=f".*?/{BIDS_SESSION_PREFIX}(?!{strip_session(session)})", + new=f".*?/{BIDS_SESSION_PREFIX}(?!{session_id})", ) self.logger.info( @@ -340,16 +340,18 @@ def run_setup(self, **kwargs): def run_main(self, **kwargs): """Run the pipeline.""" - for participant, session in self.get_participants_sessions_to_run( - self.participant, self.session + for participant_id, session_id in self.get_participants_sessions_to_run( + self.participant_id, self.session_id ): - self.logger.info(f"Running on participant {participant}, session {session}") + self.logger.info( + f"Running on participant {participant_id}, session {session_id}" + ) try: - self.run_single(participant, session) + self.run_single(participant_id, session_id) except Exception as exception: self.logger.error( f"Error running {self.pipeline_name} {self.pipeline_version}" - f" on participant {participant}, session {session}" + f" on participant {participant_id}, session {session_id}" f": {exception}" ) @@ -361,7 +363,7 @@ def run_cleanup(self, **kwargs): @abstractmethod def get_participants_sessions_to_run( - self, participant: Optional[str], session: Optional[str] + self, participant_id: Optional[str], session_id: Optional[str] ): """ Return participant-session pairs to loop over with run_single(). @@ -370,7 +372,7 @@ def get_participants_sessions_to_run( """ @abstractmethod - def run_single(self, participant: Optional[str], session: Optional[str]): + def run_single(self, participant_id: Optional[str], session_id: Optional[str]): """ Run on a single participant/session. @@ -394,8 +396,8 @@ def generate_fpath_log( fname_stem = get_pipeline_tag( pipeline_name=self.pipeline_name, pipeline_version=self.pipeline_version, - participant=self.participant, - session=self.session, + participant_id=self.participant_id, + session_id=self.session_id, ) return super().generate_fpath_log( dnames_parent=dnames_parent, fname_stem=fname_stem diff --git a/nipoppy_cli/nipoppy/workflows/runner.py b/nipoppy_cli/nipoppy/workflows/runner.py index 4fdd9fa8..e207ddc7 100644 --- a/nipoppy_cli/nipoppy/workflows/runner.py +++ b/nipoppy_cli/nipoppy/workflows/runner.py @@ -23,8 +23,8 @@ def __init__( pipeline_name: str, pipeline_version: Optional[str] = None, pipeline_step: Optional[str] = None, - participant: str = None, - session: str = None, + participant_id: str = None, + session_id: str = None, simulate: bool = False, fpath_layout: Optional[StrOrPathLike] = None, logger: Optional[logging.Logger] = None, @@ -36,8 +36,8 @@ def __init__( pipeline_name=pipeline_name, pipeline_version=pipeline_version, pipeline_step=pipeline_step, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, fpath_layout=fpath_layout, logger=logger, dry_run=dry_run, @@ -54,8 +54,8 @@ def dpaths_to_check(self) -> list[Path]: def process_container_config( self, - participant: str, - session: str, + participant_id: str, + session_id: str, bind_paths: Optional[list[StrOrPathLike]] = None, ) -> str: """Update container config and generate container command.""" @@ -67,8 +67,8 @@ def process_container_config( container_config = ContainerConfig( **self.process_template_json( container_config.model_dump(), - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) ) self.logger.debug(f"Initial container config: {container_config}") @@ -77,8 +77,8 @@ def process_container_config( boutiques_config = BoutiquesConfig( **self.process_template_json( self.boutiques_config.model_dump(), - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) ) @@ -104,15 +104,19 @@ def process_container_config( return container_command def launch_boutiques_run( - self, participant: str, session: str, objs: Optional[list] = None, **kwargs + self, + participant_id: str, + session_id: str, + objs: Optional[list] = None, + **kwargs, ): """Launch a pipeline run using Boutiques.""" # process and validate the descriptor self.logger.info("Processing the JSON descriptor") descriptor_str = self.process_template_json( self.descriptor, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, objs=objs, **kwargs, return_str=True, @@ -125,8 +129,8 @@ def launch_boutiques_run( self.logger.info("Processing the JSON invocation") invocation_str = self.process_template_json( self.invocation, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, objs=objs, **kwargs, return_str=True, @@ -148,9 +152,9 @@ def launch_boutiques_run( return descriptor_str, invocation_str def get_participants_sessions_to_run( - self, participant: Optional[str], session: Optional[str] + self, participant_id: Optional[str], session_id: Optional[str] ): - """Generate a list of participants and sessions to run. + """Generate a list of participant and session IDs to run. Specifically, this list will include participants who have BIDS data but who have not previously successfully completed the pipeline (according) @@ -163,32 +167,32 @@ def get_participants_sessions_to_run( bagel.get_completed_participants_sessions( pipeline_name=self.pipeline_name, pipeline_version=self.pipeline_version, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) ) else: participants_sessions_completed = {} for participant_session in self.doughnut.get_bidsified_participants_sessions( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ): if participant_session not in participants_sessions_completed: yield participant_session - def run_single(self, participant: str, session: str): + def run_single(self, participant_id: str, session_id: str): """Run pipeline on a single participant/session.""" # set up PyBIDS database self.set_up_bids_db( dpath_bids_db=self.dpath_pipeline_bids_db, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) # get container command container_command = self.process_container_config( - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, bind_paths=[ self.layout.dpath_bids, self.dpath_pipeline_output, @@ -198,8 +202,8 @@ def run_single(self, participant: str, session: str): ) # run pipeline with Boutiques - self.launch_boutiques_run( - participant, session, container_command=container_command + return self.launch_boutiques_run( + participant_id, session_id, container_command=container_command ) def run_cleanup(self, **kwargs): diff --git a/nipoppy_cli/nipoppy/workflows/tracker.py b/nipoppy_cli/nipoppy/workflows/tracker.py index c64c2a27..d2e572f8 100644 --- a/nipoppy_cli/nipoppy/workflows/tracker.py +++ b/nipoppy_cli/nipoppy/workflows/tracker.py @@ -19,8 +19,8 @@ def __init__( dpath_root: StrOrPathLike, pipeline_name: str, pipeline_version: Optional[str] = None, - participant: str = None, - session: str = None, + participant_id: str = None, + session_id: str = None, fpath_layout: Optional[StrOrPathLike] = None, logger: Optional[logging.Logger] = None, dry_run: bool = False, @@ -30,8 +30,8 @@ def __init__( name="track", pipeline_name=pipeline_name, pipeline_version=pipeline_version, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, fpath_layout=fpath_layout, logger=logger, dry_run=dry_run, @@ -66,14 +66,14 @@ def check_status(self, relative_paths: StrOrPathLike): return Bagel.status_success def get_participants_sessions_to_run( - self, participant: Optional[str], session: Optional[str] + self, participant_id: Optional[str], session_id: Optional[str] ): """Get participant-session pairs with BIDS data to run the tracker on.""" return self.doughnut.get_bidsified_participants_sessions( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) - def run_single(self, participant: str, session: str): + def run_single(self, participant_id: str, session_id: str): """Run tracker on a single participant/session.""" # load tracker configs from file fpath_tracker_config = self.pipeline_config.TRACKER_CONFIG_FILE @@ -85,8 +85,8 @@ def run_single(self, participant: str, session: str): # replace template strings tracker_configs = self.process_template_json( load_json(fpath_tracker_config), - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) # convert to list of TrackerConfig objects and validate tracker_configs = TypeAdapter(List[TrackerConfig]).validate_python( @@ -106,8 +106,8 @@ def run_single(self, participant: str, session: str): status = self.check_status(tracker_config.PATHS) self.bagel = self.bagel.add_or_update_records( { - Bagel.col_participant_id: participant, - Bagel.col_session: session, + Bagel.col_participant_id: participant_id, + Bagel.col_session_id: session_id, Bagel.col_pipeline_name: self.pipeline_name, Bagel.col_pipeline_version: self.pipeline_version, Bagel.col_pipeline_complete: status, diff --git a/nipoppy_cli/tests/conftest.py b/nipoppy_cli/tests/conftest.py index ff3a57e9..2e1d3634 100644 --- a/nipoppy_cli/tests/conftest.py +++ b/nipoppy_cli/tests/conftest.py @@ -15,7 +15,11 @@ from nipoppy.config.main import Config from nipoppy.tabular.doughnut import Doughnut from nipoppy.tabular.manifest import Manifest -from nipoppy.utils import StrOrPathLike, strip_session +from nipoppy.utils import ( + StrOrPathLike, + participant_id_to_bids_participant, + session_id_to_bids_session, +) FPATH_CONFIG = "global_config.json" FPATH_MANIFEST = "manifest.csv" @@ -36,7 +40,7 @@ "dpath_bids_db": "proc/pybids/bids_db", "dpath_bids_ignore_patterns": "proc/pybids/ignore_patterns", "dpath_scratch": "scratch", - "dpath_raw_dicom": "scratch/raw_dicom", + "dpath_raw_imaging": "scratch/raw_imaging", "dpath_logs": "scratch/logs", "dpath_tabular": "tabular", "dpath_assessments": "tabular/assessments", @@ -72,18 +76,18 @@ def datetime_fixture( def get_config( dataset_name="my_dataset", - sessions=None, - visits=None, + session_ids=None, + visit_ids=None, bids_pipelines=None, proc_pipelines=None, container_config=None, ): """Create a valid Config object with all required parameters.""" # everything empty by default - if sessions is None: - sessions = [] - if visits is None: - visits = [] + if session_ids is None: + session_ids = [] + if visit_ids is None: + visit_ids = [] if bids_pipelines is None: bids_pipelines = [] if proc_pipelines is None: @@ -93,8 +97,8 @@ def get_config( return Config( DATASET_NAME=dataset_name, - VISITS=visits, - SESSIONS=sessions, + VISIT_IDS=visit_ids, + SESSION_IDS=session_ids, BIDS_PIPELINES=bids_pipelines, PROC_PIPELINES=proc_pipelines, CONTAINER_CONFIG=container_config, @@ -111,38 +115,39 @@ def create_empty_dataset(dpath_root: Path): def _process_participants_sessions( participants_and_sessions: Optional[dict[str, list[str]]] = None, - participants: Optional[list[str]] = None, - sessions: Optional[list[str] | dict[str, list[str]]] = None, + participant_ids: Optional[list[str]] = None, + session_ids: Optional[list[str] | dict[str, list[str]]] = None, ): """Process participant/session arguments.""" if participants_and_sessions is None: - if participants is None: - participants = ["01", "02"] - if sessions is None: - sessions = ["ses-BL", "ses-M12"] + if participant_ids is None: + participant_ids = ["01", "02"] + if session_ids is None: + session_ids = ["ses-BL", "ses-M12"] participants_and_sessions = { - participant: sessions for participant in participants + participant: session_ids for participant in participant_ids } return participants_and_sessions -def _fake_dicoms( +def _fake_dicoms( # noqa: C901 dpath: StrOrPathLike, participants_and_sessions: Optional[dict[str, list[str]]] = None, - participants: Optional[list[str]] = None, - sessions: Optional[list[str]] = None, + participant_ids: Optional[list[str]] = None, + session_ids: Optional[list[str]] = None, n_images: int = 3, min_n_files_per_image: int = 1, max_n_files_per_image: int = 5, min_n_subdir_levels: int = 1, max_n_subdir_levels: int = 2, participant_first: bool = True, + with_prefixes: bool = False, max_dname_dicom: int = 1000000, rng_seed: int = 3791, ): """Generate a fake dataset with raw DICOM files.""" participants_and_sessions = _process_participants_sessions( - participants_and_sessions, participants, sessions + participants_and_sessions, participant_ids, session_ids ) if n_images < 1: @@ -158,15 +163,19 @@ def _fake_dicoms( rng = np.random.default_rng(rng_seed) - dpath = Path(dpath) + dpath: Path = Path(dpath) dpath.mkdir(parents=True, exist_ok=True) - for participant, participant_sessions in participants_and_sessions.items(): - for session in participant_sessions: + for participant_id, participant_session_ids in participants_and_sessions.items(): + if with_prefixes: + participant_id = participant_id_to_bids_participant(participant_id) + for session_id in participant_session_ids: + if with_prefixes: + session_id = session_id_to_bids_session(session_id) if participant_first: - dpath_dicom_parent = dpath / participant / session + dpath_dicom_parent = dpath / participant_id / session_id else: - dpath_dicom_parent = dpath / session / participant + dpath_dicom_parent = dpath / session_id / participant_id for i_image in range(n_images): n_subdir_levels = rng.integers( @@ -193,8 +202,8 @@ def _fake_dicoms( def fake_dicoms_downloaded( dpath: StrOrPathLike, participants_and_sessions: Optional[dict[str, list[str]]] = None, - participants: Optional[list[str]] = None, - sessions: Optional[list[str]] = None, + participant_ids: Optional[list[str]] = None, + session_ids: Optional[list[str]] = None, n_images: int = 3, min_n_files_per_image: int = 1, max_n_files_per_image: int = 5, @@ -208,8 +217,8 @@ def fake_dicoms_downloaded( _fake_dicoms( dpath=dpath, participants_and_sessions=participants_and_sessions, - participants=participants, - sessions=sessions, + participant_ids=participant_ids, + session_ids=session_ids, n_images=n_images, min_n_files_per_image=min_n_files_per_image, max_n_files_per_image=max_n_files_per_image, @@ -224,8 +233,8 @@ def fake_dicoms_downloaded( def fake_dicoms_organized( dpath: StrOrPathLike, participants_and_sessions: Optional[dict[str, list[str]]] = None, - participants: Optional[list[str]] = None, - sessions: Optional[list[str]] = None, + participant_ids: Optional[list[str]] = None, + session_ids: Optional[list[str]] = None, n_images: int = 3, min_n_files_per_image: int = 1, max_n_files_per_image: int = 5, @@ -237,14 +246,15 @@ def fake_dicoms_organized( _fake_dicoms( dpath=dpath, participants_and_sessions=participants_and_sessions, - participants=participants, - sessions=sessions, + participant_ids=participant_ids, + session_ids=session_ids, n_images=n_images, min_n_files_per_image=min_n_files_per_image, max_n_files_per_image=max_n_files_per_image, min_n_subdir_levels=0, max_n_subdir_levels=0, participant_first=participant_first, + with_prefixes=True, max_dname_dicom=max_dname_dicom, rng_seed=rng_seed, ) @@ -262,13 +272,13 @@ def prepare_dataset( """Create dummy imaging files for testing the DICOM-to-BIDS conversion process.""" # create the manifest data_manifest = [] - for participant in participants_and_sessions_manifest: - for session in participants_and_sessions_manifest[participant]: + for participant_id in participants_and_sessions_manifest: + for session_id in participants_and_sessions_manifest[participant_id]: data_manifest.append( { - Manifest.col_participant_id: participant, - Manifest.col_session: session, - Manifest.col_visit: session, + Manifest.col_participant_id: participant_id, + Manifest.col_session_id: session_id, + Manifest.col_visit_id: session_id, Manifest.col_datatype: [], } ) @@ -293,11 +303,11 @@ def prepare_dataset( # create fake BIDS dataset if participants_and_sessions_bidsified is not None and dpath_bidsified is not None: - for participant, sessions in participants_and_sessions_bidsified.items(): + for participant_id, session_ids in participants_and_sessions_bidsified.items(): create_fake_bids_dataset( Path(dpath_bidsified), - subjects=participant, - sessions=[strip_session(session) for session in sessions], + subjects=participant_id, + sessions=session_ids, datatypes=["anat"], ) @@ -315,22 +325,22 @@ def check_doughnut( """Check that a doughnut has the corrected statuses.""" if empty: for col in [ - doughnut.col_downloaded, - doughnut.col_organized, - doughnut.col_bidsified, + doughnut.col_in_raw_imaging, + doughnut.col_in_sourcedata, + doughnut.col_in_bids, ]: assert (~doughnut[col]).all() else: - for participant in participants_and_sessions_manifest: - for session in participants_and_sessions_manifest[participant]: + for participant_id in participants_and_sessions_manifest: + for session_id in participants_and_sessions_manifest[participant_id]: for col, participants_and_sessions_true in { - doughnut.col_downloaded: participants_and_sessions_downloaded, - doughnut.col_organized: participants_and_sessions_organized, - doughnut.col_bidsified: participants_and_sessions_bidsified, + doughnut.col_in_raw_imaging: participants_and_sessions_downloaded, + doughnut.col_in_sourcedata: participants_and_sessions_organized, + doughnut.col_in_bids: participants_and_sessions_bidsified, }.items(): status: pd.Series = doughnut.loc[ - (doughnut[doughnut.col_participant_id] == participant) - & (doughnut[doughnut.col_session] == session), + (doughnut[doughnut.col_participant_id] == participant_id) + & (doughnut[doughnut.col_session_id] == session_id), col, ] @@ -338,6 +348,6 @@ def check_doughnut( status = status.iloc[0] assert status == ( - participant in participants_and_sessions_true - and session in participants_and_sessions_true[participant] + participant_id in participants_and_sessions_true + and session_id in participants_and_sessions_true[participant_id] ) diff --git a/nipoppy_cli/tests/data/config1.json b/nipoppy_cli/tests/data/config1.json index 7ecc0801..1237087e 100644 --- a/nipoppy_cli/tests/data/config1.json +++ b/nipoppy_cli/tests/data/config1.json @@ -6,11 +6,11 @@ "--cleanenv" ] }, - "SESSIONS": [ - "ses-A", - "ses-B" + "SESSION_IDS": [ + "A", + "B" ], - "VISITS": [ + "VISIT_IDS": [ "V01", "V02" ], diff --git a/nipoppy_cli/tests/data/config2.json b/nipoppy_cli/tests/data/config2.json index 2e6e75dc..869b4d9b 100644 --- a/nipoppy_cli/tests/data/config2.json +++ b/nipoppy_cli/tests/data/config2.json @@ -1,6 +1,6 @@ { "DATASET_NAME": "my_dataset", - "VISITS": [ + "VISIT_IDS": [ "V01", "V02" ], diff --git a/nipoppy_cli/tests/data/config3.json b/nipoppy_cli/tests/data/config3.json index 619098e7..c2063130 100644 --- a/nipoppy_cli/tests/data/config3.json +++ b/nipoppy_cli/tests/data/config3.json @@ -1,6 +1,6 @@ { "DATASET_NAME": "my_dataset", - "VISITS": [ + "VISIT_IDS": [ "V01", "V02" ], diff --git a/nipoppy_cli/tests/data/config_invalid1.json b/nipoppy_cli/tests/data/config_invalid1.json index 3803e282..2809e700 100644 --- a/nipoppy_cli/tests/data/config_invalid1.json +++ b/nipoppy_cli/tests/data/config_invalid1.json @@ -1,7 +1,7 @@ { "DATASET_NAME": "my_dataset", - "SESSIONS": [ - "ses-A", - "ses-B" + "SESSION_IDS": [ + "A", + "B" ] } diff --git a/nipoppy_cli/tests/data/config_invalid2.json b/nipoppy_cli/tests/data/config_invalid2.json index 8c57850e..0acfc6cd 100644 --- a/nipoppy_cli/tests/data/config_invalid2.json +++ b/nipoppy_cli/tests/data/config_invalid2.json @@ -1,10 +1,10 @@ { "DATASET_NAME": "my_dataset", - "VISITS": [ + "VISIT_IDS": [ "V01", "V02" ], - "SESSIONS": [ + "SESSION_IDS": [ "V01" ], "PROC_PIPELINES": { diff --git a/nipoppy_cli/tests/data/dicom_dir_map1.csv b/nipoppy_cli/tests/data/dicom_dir_map1.csv index 883f3347..50593cb7 100644 --- a/nipoppy_cli/tests/data/dicom_dir_map1.csv +++ b/nipoppy_cli/tests/data/dicom_dir_map1.csv @@ -1,4 +1,4 @@ -participant_id,session,participant_dicom_dir -01,ses-1,path/for/sub-01/ses-1 -01,ses-2,path/for/sub-01/ses-2 -02,ses-1,path/for/sub-02/ses-1 +participant_id,session_id,participant_dicom_dir +01,1,path/for/sub-01/ses-1 +01,2,path/for/sub-01/ses-2 +02,1,path/for/sub-02/ses-1 diff --git a/nipoppy_cli/tests/data/dicom_dir_map2.csv b/nipoppy_cli/tests/data/dicom_dir_map2.csv index 93d0b4d5..c0e3defa 100644 --- a/nipoppy_cli/tests/data/dicom_dir_map2.csv +++ b/nipoppy_cli/tests/data/dicom_dir_map2.csv @@ -1,4 +1,4 @@ -participant_id,session,participant_dicom_dir -01,ses-BL,all_BL_files_are_together -01,ses-M12,all_M12_files_are_together -02,ses-BL,all_BL_files_are_together +participant_id,session_id,participant_dicom_dir +01,BL,all_BL_files_are_together +01,M12,all_M12_files_are_together +02,BL,all_BL_files_are_together diff --git a/nipoppy_cli/tests/data/dicom_dir_map_invalid1.csv b/nipoppy_cli/tests/data/dicom_dir_map_invalid1.csv index a1021200..7ac7129c 100644 --- a/nipoppy_cli/tests/data/dicom_dir_map_invalid1.csv +++ b/nipoppy_cli/tests/data/dicom_dir_map_invalid1.csv @@ -1,4 +1,4 @@ -participant_id,session,dicom_dir -01,ses-1,path/for/sub-01/ses-1 -01,ses-2,path/for/sub-01/ses-2 -02,ses-1,path/for/sub-02/ses-1 +participant_id,session_id,dicom_dir +01,1,path/for/sub-01/ses1 +01,2,path/for/sub-01/ses2 +02,1,path/for/sub-02/ses1 diff --git a/nipoppy_cli/tests/data/dicom_dir_map_invalid3.csv b/nipoppy_cli/tests/data/dicom_dir_map_invalid3.csv index d37218df..33948be0 100644 --- a/nipoppy_cli/tests/data/dicom_dir_map_invalid3.csv +++ b/nipoppy_cli/tests/data/dicom_dir_map_invalid3.csv @@ -1,4 +1,4 @@ -participant_id,session,participant_dicom_dir -01,ses-1,path/for/sub-01/ses-1 -01,ses-2,path/for/sub-01/ses-2 -sub-02,ses-1,path/for/sub-02/ses-1 +participant_id,session_id,participant_dicom_dir +01,1,path/for/sub-01/ses-1 +01,2,path/for/sub-01/ses-2 +sub-02,1,path/for/sub-02/ses-1 diff --git a/nipoppy_cli/tests/data/dicom_dir_map_invalid4.csv b/nipoppy_cli/tests/data/dicom_dir_map_invalid4.csv index 3d1124a2..1d1b284f 100644 --- a/nipoppy_cli/tests/data/dicom_dir_map_invalid4.csv +++ b/nipoppy_cli/tests/data/dicom_dir_map_invalid4.csv @@ -1,4 +1,4 @@ -participant_id,session,participant_dicom_dir -01,ses-1,path/for/sub-01/ses-1 -01,ses-2,path/for/sub-01/ses-2 -02,1,path/for/sub-02/ses-1 +participant_id,session_id,participant_dicom_dir +01,1,path/for/sub-01/ses-1 +01,2,path/for/sub-01/ses-2 +02,ses-1,path/for/sub-02/ses-1 diff --git a/nipoppy_cli/tests/data/doughnut1.csv b/nipoppy_cli/tests/data/doughnut1.csv index 4784d316..0ea384db 100644 --- a/nipoppy_cli/tests/data/doughnut1.csv +++ b/nipoppy_cli/tests/data/doughnut1.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype,participant_dicom_dir,dicom_id,bids_id,downloaded,organized,bidsified -01,BL,ses-BL,"['anat']",01,01,sub-01,True,True,False -01,M12,ses-M12,"['anat']",01,01,sub-01,True,False,False -02,BL,ses-BL,"['anat']",01,01,sub-02,False,False,False -02,M12,ses-M12,"['anat']",01,01,sub-02,False,False,False +participant_id,visit_id,session_id,datatype,participant_dicom_dir,in_raw_imaging,in_sourcedata,in_bids +01,BL,BL,"['anat']",01,True,True,False +01,M12,M12,"['anat']",01,True,False,False +02,BL,BL,"['anat']",01,False,False,False +02,M12,M12,"['anat']",01,False,False,False diff --git a/nipoppy_cli/tests/data/doughnut2.csv b/nipoppy_cli/tests/data/doughnut2.csv index 5b165da5..85546a14 100644 --- a/nipoppy_cli/tests/data/doughnut2.csv +++ b/nipoppy_cli/tests/data/doughnut2.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype,participant_dicom_dir,dicom_id,bids_id,downloaded,organized,bidsified -01,BL,ses-BL,"['anat']",01,01,sub-01,False,False,True -01,M12,ses-M12,"['anat']",01,01,sub-01,False,False,True -02,BL,ses-BL,"['anat']",01,01,sub-02,False,False,True -02,M12,ses-M12,"['anat']",01,01,sub-02,False,False,True +participant_id,visit_id,session_id,datatype,participant_dicom_dir,in_raw_imaging,in_sourcedata,in_bids +01,BL,BL,"['anat']",01,False,False,True +01,M12,M12,"['anat']",01,False,False,True +02,BL,BL,"['anat']",01,False,False,True +02,M12,M12,"['anat']",01,False,False,True diff --git a/nipoppy_cli/tests/data/doughnut_invalid1.csv b/nipoppy_cli/tests/data/doughnut_invalid1.csv index 3af46b97..c65d3963 100644 --- a/nipoppy_cli/tests/data/doughnut_invalid1.csv +++ b/nipoppy_cli/tests/data/doughnut_invalid1.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype,participant_dicom_dir,dicom_id,bids_id -01,BL,ses-BL,"['anat']",01,01,sub-01 -01,M12,ses-M12,"['anat']",01,01,sub-01 -02,BL,ses-BL,"['anat']",01,01,sub-02 -02,M12,ses-M12,"['anat']",01,01,sub-02 +participant_id,visit_id,session_id,datatype,participant_dicom_dir,dicom_id,bids_id +01,BL,BL,"['anat']",01,01,sub-01 +01,M12,M12,"['anat']",01,01,sub-01 +02,BL,BL,"['anat']",01,01,sub-02 +02,M12,M12,"['anat']",01,01,sub-02 diff --git a/nipoppy_cli/tests/data/doughnut_invalid2.csv b/nipoppy_cli/tests/data/doughnut_invalid2.csv index a0d588f8..a7ea24b8 100644 --- a/nipoppy_cli/tests/data/doughnut_invalid2.csv +++ b/nipoppy_cli/tests/data/doughnut_invalid2.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype,dicom_id,bids_id,downloaded,organized,bidsified -01,BL,ses-BL,"['anat']",01,sub-01,False,False,True -01,M12,ses-M12,"['anat']",01,sub-01,False,False,True -02,BL,ses-BL,"['anat']",01,sub-02,False,False,True -02,M12,ses-M12,"['anat']",01,sub-02,False,False,True +participant_id,visit_id,session_id,datatype,dicom_id,bids_id,in_raw_imaging,in_sourcedata,in_bids +01,BL,BL,"['anat']",01,sub-01,False,False,True +01,M12,M12,"['anat']",01,sub-01,False,False,True +02,BL,BL,"['anat']",01,sub-02,False,False,True +02,M12,M12,"['anat']",01,sub-02,False,False,True diff --git a/nipoppy_cli/tests/data/layout1.json b/nipoppy_cli/tests/data/layout1.json index a5ce6fab..92a5633e 100644 --- a/nipoppy_cli/tests/data/layout1.json +++ b/nipoppy_cli/tests/data/layout1.json @@ -42,8 +42,8 @@ "dpath_scratch": { "path": "scratch" }, - "dpath_raw_dicom": { - "path": "scratch/raw_dicom" + "dpath_raw_imaging": { + "path": "scratch/raw_imaging" }, "dpath_logs": { "path": "scratch/logs" diff --git a/nipoppy_cli/tests/data/layout2.json b/nipoppy_cli/tests/data/layout2.json index 76220ebc..2d2d9f8d 100644 --- a/nipoppy_cli/tests/data/layout2.json +++ b/nipoppy_cli/tests/data/layout2.json @@ -41,8 +41,8 @@ "dpath_scratch": { "path": "scratch" }, - "dpath_raw_dicom": { - "path": "scratch/raw_dicom" + "dpath_raw_imaging": { + "path": "scratch/raw_imaging" }, "dpath_logs": { "path": "scratch/logs" diff --git a/nipoppy_cli/tests/data/layout_invalid1.json b/nipoppy_cli/tests/data/layout_invalid1.json index 8fc81128..3eabbdb2 100644 --- a/nipoppy_cli/tests/data/layout_invalid1.json +++ b/nipoppy_cli/tests/data/layout_invalid1.json @@ -42,8 +42,8 @@ "dpath_scratch": { "path": "scratch" }, - "dpath_raw_dicom": { - "path": "scratch/raw_dicom" + "dpath_raw_imaging": { + "path": "scratch/raw_imaging" }, "dpath_logs": { "path": "scratch/logs" diff --git a/nipoppy_cli/tests/data/layout_invalid2.json b/nipoppy_cli/tests/data/layout_invalid2.json index 43525da2..d94ca9d0 100644 --- a/nipoppy_cli/tests/data/layout_invalid2.json +++ b/nipoppy_cli/tests/data/layout_invalid2.json @@ -38,8 +38,8 @@ "dpath_scratch": { "path": "scratch" }, - "dpath_raw_dicom": { - "path": "scratch/raw_dicom" + "dpath_raw_imaging": { + "path": "scratch/raw_imaging" }, "dpath_logs": { "path": "scratch/logs" diff --git a/nipoppy_cli/tests/data/manifest1.csv b/nipoppy_cli/tests/data/manifest1.csv index e4b17f6d..7b64548c 100644 --- a/nipoppy_cli/tests/data/manifest1.csv +++ b/nipoppy_cli/tests/data/manifest1.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype -01,BL,ses-BL,"['anat']" -01,M12,ses-M12,"['anat']" -02,BL,ses-BL,"['anat','dwi']" -02,M12,ses-M12,"['anat','dwi']" +participant_id,visit_id,session_id,datatype +01,BL,BL,"['anat']" +01,M12,M12,"['anat']" +02,BL,BL,"['anat','dwi']" +02,M12,M12,"['anat','dwi']" diff --git a/nipoppy_cli/tests/data/manifest2.csv b/nipoppy_cli/tests/data/manifest2.csv index 3a5d1ffa..5ebbfa1c 100644 --- a/nipoppy_cli/tests/data/manifest2.csv +++ b/nipoppy_cli/tests/data/manifest2.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype -01,MRI1,ses-1,["anat"] +participant_id,visit_id,session_id,datatype +01,MRI1,1,["anat"] 01,MOCA1,, -02,MRI1,ses-1,["anat"] +02,MRI1,1,["anat"] 02,MOCA1,,[] diff --git a/nipoppy_cli/tests/data/manifest3.csv b/nipoppy_cli/tests/data/manifest3.csv index 82df99b5..34e2ad00 100644 --- a/nipoppy_cli/tests/data/manifest3.csv +++ b/nipoppy_cli/tests/data/manifest3.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype,extra_col -01,MRI1,ses-1,["anat"],x +participant_id,visit_id,session_id,datatype,extra_col +01,MRI1,1,["anat"],x 01,MOCA1,,, -02,MRI1,ses-1,["anat"],y +02,MRI1,1,["anat"],y 02,MOCA1,,[], diff --git a/nipoppy_cli/tests/data/manifest_invalid1.csv b/nipoppy_cli/tests/data/manifest_invalid1.csv index 4b89eaf7..2aa0e170 100644 --- a/nipoppy_cli/tests/data/manifest_invalid1.csv +++ b/nipoppy_cli/tests/data/manifest_invalid1.csv @@ -1,5 +1,5 @@ -participant_id,visit,session,datatype -01,MRI1,ses-MRI1,anat +participant_id,visit_id,session_id,datatype +01,MRI1,MRI1,anat 01,MOCA1,, -02,MRI1,ses-MRI1, +02,MRI1,MRI1, 02,MOCA1,, diff --git a/nipoppy_cli/tests/data/manifest_invalid2.csv b/nipoppy_cli/tests/data/manifest_invalid2.csv index 7b77f2c7..dcdd16f8 100644 --- a/nipoppy_cli/tests/data/manifest_invalid2.csv +++ b/nipoppy_cli/tests/data/manifest_invalid2.csv @@ -1,5 +1,5 @@ -participant_id,visit,session -01,MRI1,ses-MRI1 -01,MOCA1,ses-MOCA1 -02,MRI1,ses-MRI1 -02,MOCA1,ses-MOCA1 +participant_id,visit_id,session_id +01,MRI1,MRI1 +01,MOCA1,MOCA1 +02,MRI1,MRI1 +02,MOCA1,MOCA1 diff --git a/nipoppy_cli/tests/data/manifest_invalid3.csv b/nipoppy_cli/tests/data/manifest_invalid3.csv index ff0e2919..9f85997c 100644 --- a/nipoppy_cli/tests/data/manifest_invalid3.csv +++ b/nipoppy_cli/tests/data/manifest_invalid3.csv @@ -1,6 +1,6 @@ -participant_id,visit,session,datatype -01,BL,ses-BL,"['anat']" -01,M12,ses-M12,"['anat']" -02,BL,ses-BL,"['anat','dwi']" -02,M12,ses-M12,"['anat','dwi']" -sub-01,BL,ses-BL,"['anat']" +participant_id,visit_id,session_id,datatype +01,BL,BL,"['anat']" +01,M12,M12,"['anat']" +02,BL,BL,"['anat','dwi']" +02,M12,M12,"['anat','dwi']" +sub-01,BL,BL,"['anat']" diff --git a/nipoppy_cli/tests/data/manifest_invalid4.csv b/nipoppy_cli/tests/data/manifest_invalid4.csv index 1107c083..4b986ae1 100644 --- a/nipoppy_cli/tests/data/manifest_invalid4.csv +++ b/nipoppy_cli/tests/data/manifest_invalid4.csv @@ -1,6 +1,6 @@ -participant_id,visit,session,datatype -01,BL,ses-BL,"['anat']" -01,M12,ses-M12,"['anat']" -02,BL,ses-BL,"['anat','dwi']" -02,M12,ses-M12,"['anat','dwi']" -03,BL,BL,"['anat']" +participant_id,visit_id,session_id,datatype +01,BL,BL,"['anat']" +01,M12,M12,"['anat']" +02,BL,BL,"['anat','dwi']" +02,M12,M12,"['anat','dwi']" +03,BL,ses-BL,"['anat']" diff --git a/nipoppy_cli/tests/test_config_main.py b/nipoppy_cli/tests/test_config_main.py index 2ae1321b..64c53fba 100644 --- a/nipoppy_cli/tests/test_config_main.py +++ b/nipoppy_cli/tests/test_config_main.py @@ -14,9 +14,9 @@ from .conftest import DPATH_TEST_DATA -REQUIRED_FIELDS_CONFIG = ["DATASET_NAME", "VISITS", "PROC_PIPELINES"] +REQUIRED_FIELDS_CONFIG = ["DATASET_NAME", "VISIT_IDS", "PROC_PIPELINES"] FIELDS_CONFIG = REQUIRED_FIELDS_CONFIG + [ - "SESSIONS", + "SESSION_IDS", "SUBSTITUTIONS", "BIDS_PIPELINES", "CUSTOM", @@ -30,8 +30,8 @@ def valid_config_data(): return { "DATASET_NAME": "my_dataset", - "VISITS": ["1"], - "SESSIONS": ["ses-1"], + "VISIT_IDS": ["1"], + "SESSION_IDS": ["1"], "BIDS_PIPELINES": [ { "NAME": "bids_converter", @@ -104,21 +104,21 @@ def test_check_no_duplicate_pipeline( @pytest.mark.parametrize( - "visits,expected_sessions", + "visit_ids,expected_session_ids", [ - (["V01", "V02"], ["ses-V01", "ses-V02"]), - (["ses-1", "2"], ["ses-1", "ses-2"]), + (["V01", "V02"], ["V01", "V02"]), + (["1", "2"], ["1", "2"]), ], ) -def test_sessions_inferred(visits, expected_sessions): +def test_sessions_inferred(visit_ids, expected_session_ids): data = { "DATASET_NAME": "my_dataset", - "VISITS": visits, + "VISIT_IDS": visit_ids, "BIDS_PIPELINES": [], "PROC_PIPELINES": [], } config = Config(**data) - assert config.SESSIONS == expected_sessions + assert config.SESSION_IDS == expected_session_ids @pytest.mark.parametrize( diff --git a/nipoppy_cli/tests/test_default_config.py b/nipoppy_cli/tests/test_default_config.py index 8e74ee77..a709172a 100644 --- a/nipoppy_cli/tests/test_default_config.py +++ b/nipoppy_cli/tests/test_default_config.py @@ -8,7 +8,12 @@ from nipoppy.config.main import Config from nipoppy.layout import DatasetLayout -from nipoppy.utils import DPATH_DESCRIPTORS, DPATH_INVOCATIONS, FPATH_SAMPLE_CONFIG_FULL +from nipoppy.utils import ( + DPATH_DESCRIPTORS, + DPATH_INVOCATIONS, + FPATH_SAMPLE_CONFIG_FULL, + TEMPLATE_REPLACE_PATTERN, +) from nipoppy.workflows import BidsConversionRunner, PipelineRunner from .conftest import create_empty_dataset, prepare_dataset @@ -19,8 +24,8 @@ def single_subject_dataset( tmp_path: Path, mocker: pytest_mock.MockerFixture ) -> DatasetLayout: dataset_root = tmp_path / "my_dataset" - participant = "01" - session = "ses-01" + participant_id = "01" + session_id = "01" container_command = "apptainer" substitutions = { "[[NIPOPPY_DPATH_DESCRIPTORS]]": str(DPATH_DESCRIPTORS), @@ -32,7 +37,7 @@ def single_subject_dataset( "[[TEMPLATEFLOW_HOME]]": str(tmp_path / "templateflow"), } - participants_and_sessions = {participant: [session]} + participants_and_sessions = {participant_id: [session_id]} layout = DatasetLayout(dataset_root) create_empty_dataset(dataset_root) @@ -57,7 +62,7 @@ def single_subject_dataset( return_value=container_command, ) - return layout, participant, session + return layout, participant_id, session_id def get_fpaths_descriptors() -> list[str]: @@ -77,8 +82,12 @@ def test_boutiques_descriptors(fpath_descriptor): ("mriqc", "23.1.0"), ], ) -def test_pipeline_runner(pipeline_name, pipeline_version, single_subject_dataset): - layout, participant, session = single_subject_dataset +def test_pipeline_runner( + pipeline_name, + pipeline_version, + single_subject_dataset, +): + layout, participant_id, session_id = single_subject_dataset layout: DatasetLayout runner = PipelineRunner( dpath_root=layout.dpath_root, @@ -89,7 +98,12 @@ def test_pipeline_runner(pipeline_name, pipeline_version, single_subject_dataset runner.pipeline_config.get_fpath_container().touch() - runner.run_single(participant=participant, session=session) + invocation_str, descriptor_str = runner.run_single( + participant_id=participant_id, session_id=session_id + ) + + assert TEMPLATE_REPLACE_PATTERN.search(invocation_str) is None + assert TEMPLATE_REPLACE_PATTERN.search(descriptor_str) is None @pytest.mark.parametrize( @@ -104,7 +118,7 @@ def test_pipeline_runner(pipeline_name, pipeline_version, single_subject_dataset def test_bids_conversion_runner( pipeline_name, pipeline_version, pipeline_step, single_subject_dataset ): - layout, participant, session = single_subject_dataset + layout, participant_id, session_id = single_subject_dataset layout: DatasetLayout runner = BidsConversionRunner( dpath_root=layout.dpath_root, @@ -116,5 +130,9 @@ def test_bids_conversion_runner( runner.pipeline_config.get_fpath_container().touch() - print(runner.invocation) - runner.run_single(participant=participant, session=session) + invocation_str, descriptor_str = runner.run_single( + participant_id=participant_id, session_id=session_id + ) + + assert TEMPLATE_REPLACE_PATTERN.search(invocation_str) is None + assert TEMPLATE_REPLACE_PATTERN.search(descriptor_str) is None diff --git a/nipoppy_cli/tests/test_layout.py b/nipoppy_cli/tests/test_layout.py index 24fc6822..e58463ca 100644 --- a/nipoppy_cli/tests/test_layout.py +++ b/nipoppy_cli/tests/test_layout.py @@ -206,7 +206,7 @@ def test_get_dpath_pipeline( @pytest.mark.parametrize( - "pipeline_name,pipeline_version,participant,session,expected", + "pipeline_name,pipeline_version,participant_id,session_id,expected", [ ( "my_pipeline", @@ -222,13 +222,6 @@ def test_get_dpath_pipeline( None, "derivatives/pipeline/v2/work/pipeline-v2-3000", ), - ( - "pipeline", - "v2", - None, - "ses-BL", - "derivatives/pipeline/v2/work/pipeline-v2-BL", - ), ( "pipeline", "v2", @@ -239,15 +232,20 @@ def test_get_dpath_pipeline( ], ) def test_get_dpath_pipeline_work( - dpath_root: Path, pipeline_name, pipeline_version, participant, session, expected + dpath_root: Path, + pipeline_name, + pipeline_version, + participant_id, + session_id, + expected, ): layout = DatasetLayout(dpath_root=dpath_root) assert ( layout.get_dpath_pipeline_work( pipeline_name=pipeline_name, pipeline_version=pipeline_version, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) == dpath_root / expected ) @@ -273,22 +271,27 @@ def test_get_dpath_pipeline_output( @pytest.mark.parametrize( - "pipeline_name,pipeline_version,participant,session,expected", + "pipeline_name,pipeline_version,participant_id,session_id,expected", [ ("my_pipeline", "v1", None, None, "proc/pybids/bids_db/my_pipeline-v1"), - ("pipeline", "v2", "01", "ses-1", "proc/pybids/bids_db/pipeline-v2-01-1"), + ("pipeline", "v2", "01", "1", "proc/pybids/bids_db/pipeline-v2-01-1"), ], ) def test_get_dpath_bids_db( - dpath_root: Path, pipeline_name, pipeline_version, participant, session, expected + dpath_root: Path, + pipeline_name, + pipeline_version, + participant_id, + session_id, + expected, ): layout = DatasetLayout(dpath_root=dpath_root) assert ( layout.get_dpath_bids_db( pipeline_name=pipeline_name, pipeline_version=pipeline_version, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) == dpath_root / expected ) diff --git a/nipoppy_cli/tests/test_parser.py b/nipoppy_cli/tests/test_parser.py index 3ad0b2e4..d4d999a9 100644 --- a/nipoppy_cli/tests/test_parser.py +++ b/nipoppy_cli/tests/test_parser.py @@ -58,9 +58,9 @@ def test_add_arg_pipeline_step(): "args", [ [], - ["--participant", "1000"], - ["--session", "1"], - ["--participant", "sub-123", "--session", "ses-1"], + ["--participant-id", "1000"], + ["--session-id", "1"], + ["--participant-id", "sub-123", "--session-id", "ses-1"], ], ) def test_add_args_participant_and_session(args): @@ -179,7 +179,7 @@ def test_add_subparser_bids_conversion(args): "my_dataset", "--pipeline", "pipeline2", - "--participant", + "--participant-id", "1000", ], [ @@ -187,9 +187,9 @@ def test_add_subparser_bids_conversion(args): "my_dataset", "--pipeline", "pipeline2", - "--participant", + "--participant-id", "1000", - "--session", + "--session-id", "BL", ], ], diff --git a/nipoppy_cli/tests/test_tabular_bagel.py b/nipoppy_cli/tests/test_tabular_bagel.py index 5063f303..a82437ac 100644 --- a/nipoppy_cli/tests/test_tabular_bagel.py +++ b/nipoppy_cli/tests/test_tabular_bagel.py @@ -11,15 +11,15 @@ [ { Bagel.col_participant_id: "1", - Bagel.col_bids_id: "sub-1", - Bagel.col_session: "ses-01", + Bagel.col_bids_participant: "sub-1", + Bagel.col_session_id: "01", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, }, { Bagel.col_participant_id: "2", - Bagel.col_session: "ses-02", + Bagel.col_session_id: "02", Bagel.col_pipeline_name: "my_other_pipeline", Bagel.col_pipeline_version: "2.0", Bagel.col_pipeline_complete: Bagel.status_fail, @@ -36,12 +36,12 @@ def test_model(data): def test_model_bids_id(participant_id, expected_bids_id): model = BagelModel( participant_id=participant_id, - session="ses-01", + session_id="01", pipeline_name="my_pipeline", pipeline_version="1.0", pipeline_complete=Bagel.status_success, ) - assert model.bids_id == expected_bids_id + assert model.bids_participant == expected_bids_id @pytest.mark.parametrize( @@ -56,7 +56,7 @@ def test_model_bids_id(participant_id, expected_bids_id): def test_model_status(status): model = BagelModel( participant_id="1", - session="ses-01", + session_id="01", pipeline_name="my_pipeline", pipeline_version="1.0", pipeline_complete=status, @@ -68,7 +68,7 @@ def test_model_status_invalid(): with pytest.raises(ValidationError): BagelModel( participant_id="1", - session="ses-01", + session_id="01", pipeline_name="my_pipeline", pipeline_version="1.0", pipeline_complete="BAD_STATUS", @@ -83,7 +83,7 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-1", + Bagel.col_session_id: "1", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, @@ -92,7 +92,7 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-1", + Bagel.col_session_id: "1", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, @@ -103,7 +103,7 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-1", + Bagel.col_session_id: "1", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_fail, @@ -112,7 +112,7 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-1", + Bagel.col_session_id: "1", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, @@ -121,7 +121,7 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-1", + Bagel.col_session_id: "1", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, @@ -132,14 +132,14 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-1", + Bagel.col_session_id: "1", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_fail, }, { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-2", + Bagel.col_session_id: "2", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_unavailable, @@ -148,14 +148,14 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-2", + Bagel.col_session_id: "2", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, }, { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-3", + Bagel.col_session_id: "3", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, @@ -164,21 +164,21 @@ def test_model_status_invalid(): [ { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-1", + Bagel.col_session_id: "1", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_fail, }, { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-2", + Bagel.col_session_id: "2", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, }, { Bagel.col_participant_id: "01", - Bagel.col_session: "ses-3", + Bagel.col_session_id: "3", Bagel.col_pipeline_name: "my_pipeline", Bagel.col_pipeline_version: "1.0", Bagel.col_pipeline_complete: Bagel.status_success, @@ -195,61 +195,61 @@ def test_add_or_update_records(data_orig, data_new, data_expected): @pytest.mark.parametrize( - "data,pipeline_name,pipeline_version,participant,session,expected", + "data,pipeline_name,pipeline_version,participant_id,session_id,expected", [ ( [ - ["01", "ses-1", "pipeline1", "1.0", Bagel.status_success], - ["02", "ses-1", "pipeline1", "1.0", Bagel.status_fail], - ["03", "ses-1", "pipeline1", "1.0", Bagel.status_incomplete], - ["04", "ses-1", "pipeline1", "1.0", Bagel.status_unavailable], + ["01", "1", "pipeline1", "1.0", Bagel.status_success], + ["02", "1", "pipeline1", "1.0", Bagel.status_fail], + ["03", "1", "pipeline1", "1.0", Bagel.status_incomplete], + ["04", "1", "pipeline1", "1.0", Bagel.status_unavailable], ], "pipeline1", "1.0", None, None, - [("01", "ses-1")], + [("01", "1")], ), ( [ - ["S01", "ses-BL", "pipeline1", "1.0", Bagel.status_success], - ["S01", "ses-BL", "pipeline1", "2.0", Bagel.status_success], - ["S01", "ses-BL", "pipeline2", "1.0", Bagel.status_success], - ["S01", "ses-BL", "pipeline2", "2.0", Bagel.status_success], + ["S01", "BL", "pipeline1", "1.0", Bagel.status_success], + ["S01", "BL", "pipeline1", "2.0", Bagel.status_success], + ["S01", "BL", "pipeline2", "1.0", Bagel.status_success], + ["S01", "BL", "pipeline2", "2.0", Bagel.status_success], ], "pipeline2", "2.0", None, None, - [("S01", "ses-BL")], + [("S01", "BL")], ), ( [ - ["S01", "ses-BL", "pipeline1", "1.0", Bagel.status_success], - ["S01", "ses-BL", "pipeline1", "2.0", Bagel.status_success], - ["S01", "ses-BL", "pipeline2", "1.0", Bagel.status_success], - ["S01", "ses-M12", "pipeline2", "2.0", Bagel.status_success], - ["S02", "ses-BL", "pipeline1", "1.0", Bagel.status_success], - ["S02", "ses-BL", "pipeline1", "2.0", Bagel.status_success], - ["S02", "ses-BL", "pipeline2", "2.0", Bagel.status_success], - ["S02", "ses-M12", "pipeline2", "2.0", Bagel.status_success], + ["S01", "BL", "pipeline1", "1.0", Bagel.status_success], + ["S01", "BL", "pipeline1", "2.0", Bagel.status_success], + ["S01", "BL", "pipeline2", "1.0", Bagel.status_success], + ["S01", "M12", "pipeline2", "2.0", Bagel.status_success], + ["S02", "BL", "pipeline1", "1.0", Bagel.status_success], + ["S02", "BL", "pipeline1", "2.0", Bagel.status_success], + ["S02", "BL", "pipeline2", "2.0", Bagel.status_success], + ["S02", "M12", "pipeline2", "2.0", Bagel.status_success], ], "pipeline2", "2.0", "S02", - "ses-M12", - [("S02", "ses-M12")], + "M12", + [("S02", "M12")], ), ], ) def test_get_completed_participants_sessions( - data, pipeline_name, pipeline_version, participant, session, expected + data, pipeline_name, pipeline_version, participant_id, session_id, expected ): bagel = Bagel( data, columns=[ Bagel.col_participant_id, - Bagel.col_session, + Bagel.col_session_id, Bagel.col_pipeline_name, Bagel.col_pipeline_version, Bagel.col_pipeline_complete, @@ -261,7 +261,7 @@ def test_get_completed_participants_sessions( for x in bagel.get_completed_participants_sessions( pipeline_name=pipeline_name, pipeline_version=pipeline_version, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) ] == expected diff --git a/nipoppy_cli/tests/test_tabular_dicom_dir_map.py b/nipoppy_cli/tests/test_tabular_dicom_dir_map.py index 40d5430d..b6f0fd93 100644 --- a/nipoppy_cli/tests/test_tabular_dicom_dir_map.py +++ b/nipoppy_cli/tests/test_tabular_dicom_dir_map.py @@ -49,8 +49,8 @@ def test_load_or_generate_load(fpath_dicom_dir_map): @pytest.mark.parametrize( "participant_ids,sessions,participant_first,expected", [ - (["01", "02"], ["ses-1", "ses-2"], True, ["01/ses-1", "02/ses-2"]), - (["P01", "P02"], ["ses-BL", "ses-BL"], False, ["ses-BL/P01", "ses-BL/P02"]), + (["01", "02"], ["1", "2"], True, ["01/1", "02/2"]), + (["P01", "P02"], ["BL", "BL"], False, ["BL/P01", "BL/P02"]), ], ) def test_load_or_generate_generate( @@ -59,8 +59,8 @@ def test_load_or_generate_generate( manifest = Manifest( data={ Manifest.col_participant_id: participant_ids, - Manifest.col_visit: sessions, - Manifest.col_session: sessions, + Manifest.col_visit_id: sessions, + Manifest.col_session_id: sessions, Manifest.col_datatype: [[] for _ in participant_ids], } ) diff --git a/nipoppy_cli/tests/test_tabular_doughnut.py b/nipoppy_cli/tests/test_tabular_doughnut.py index 18980b59..9538dc54 100644 --- a/nipoppy_cli/tests/test_tabular_doughnut.py +++ b/nipoppy_cli/tests/test_tabular_doughnut.py @@ -1,4 +1,4 @@ -"""Tests for the manifest.""" +"""Tests for the doughnut.""" from contextlib import nullcontext from pathlib import Path @@ -16,15 +16,13 @@ def data(): return { Doughnut.col_participant_id: ["01", "01", "02", "02"], - Doughnut.col_visit: ["BL", "M12", "BL", "M12"], - Doughnut.col_session: ["ses-BL", "ses-M12", "ses-BL", "ses-M12"], + Doughnut.col_visit_id: ["BL", "M12", "BL", "M12"], + Doughnut.col_session_id: ["BL", "M12", "BL", "M12"], Doughnut.col_datatype: ["anat", "anat", "anat", "anat"], Doughnut.col_participant_dicom_dir: ["01", "01", "02", "02"], - Doughnut.col_dicom_id: ["01", "01", "02", "02"], - Doughnut.col_bids_id: ["01", "01", "02", "02"], - Doughnut.col_downloaded: [True, True, True, False], - Doughnut.col_organized: [True, False, True, False], - Doughnut.col_bidsified: [True, False, False, False], + Doughnut.col_in_raw_imaging: [True, True, True, False], + Doughnut.col_in_sourcedata: [True, False, True, False], + Doughnut.col_in_bids: [True, False, False, False], } @@ -56,7 +54,8 @@ def test_validate(fpath, is_valid): @pytest.mark.parametrize( - "col", [Doughnut.col_downloaded, Doughnut.col_organized, Doughnut.col_bidsified] + "col", + [Doughnut.col_in_raw_imaging, Doughnut.col_in_sourcedata, Doughnut.col_in_bids], ) def test_check_status_col(col): assert Doughnut._check_status_col(col) == col @@ -78,55 +77,62 @@ def test_check_status_value_invalid(): @pytest.mark.parametrize( - "participant,session,col,expected_status", + "participant_id,session_id,col,expected_status", [ - ("01", "ses-BL", Doughnut.col_downloaded, True), - ("01", "ses-BL", Doughnut.col_organized, True), - ("01", "ses-BL", Doughnut.col_bidsified, True), - ("02", "ses-M12", Doughnut.col_downloaded, False), - ("02", "ses-M12", Doughnut.col_organized, False), - ("02", "ses-M12", Doughnut.col_bidsified, False), + ("01", "BL", Doughnut.col_in_raw_imaging, True), + ("01", "BL", Doughnut.col_in_sourcedata, True), + ("01", "BL", Doughnut.col_in_bids, True), + ("02", "M12", Doughnut.col_in_raw_imaging, False), + ("02", "M12", Doughnut.col_in_sourcedata, False), + ("02", "M12", Doughnut.col_in_bids, False), ], ) -def test_get_status(data, participant, session, col, expected_status): - assert Doughnut(data).get_status(participant, session, col) == expected_status +def test_get_status(data, participant_id, session_id, col, expected_status): + assert Doughnut(data).get_status(participant_id, session_id, col) == expected_status @pytest.mark.parametrize( - "participant,session,col,status", + "participant_id,session_id,col,status", [ - ("01", "ses-BL", Doughnut.col_downloaded, False), - ("01", "ses-BL", Doughnut.col_organized, False), - ("01", "ses-BL", Doughnut.col_bidsified, False), - ("02", "ses-M12", Doughnut.col_downloaded, True), - ("02", "ses-M12", Doughnut.col_organized, True), - ("02", "ses-M12", Doughnut.col_bidsified, True), + ("01", "BL", Doughnut.col_in_raw_imaging, False), + ("01", "BL", Doughnut.col_in_sourcedata, False), + ("01", "BL", Doughnut.col_in_bids, False), + ("02", "M12", Doughnut.col_in_raw_imaging, True), + ("02", "M12", Doughnut.col_in_sourcedata, True), + ("02", "M12", Doughnut.col_in_bids, True), ], ) -def test_set_status(data, participant, session, col, status): +def test_set_status(data, participant_id, session_id, col, status): doughnut = Doughnut(data) - doughnut.set_status(participant, session, col, status) - assert doughnut.get_status(participant, session, col) == status + doughnut.set_status( + participant_id=participant_id, session_id=session_id, col=col, status=status + ) + assert ( + doughnut.get_status( + participant_id=participant_id, session_id=session_id, col=col + ) + == status + ) @pytest.mark.parametrize( - "status_col,participant,session,expected_count", + "status_col,participant_id,session_id,expected_count", [ - ("downloaded", None, None, 3), - ("organized", None, None, 2), - ("bidsified", None, None, 1), - ("downloaded", "01", None, 2), - ("organized", None, "ses-M12", 0), - ("bidsified", "01", "ses-BL", 1), + (Doughnut.col_in_raw_imaging, None, None, 3), + (Doughnut.col_in_sourcedata, None, None, 2), + (Doughnut.col_in_bids, None, None, 1), + (Doughnut.col_in_raw_imaging, "01", None, 2), + (Doughnut.col_in_sourcedata, None, "M12", 0), + (Doughnut.col_in_bids, "01", "BL", 1), ], ) def test_get_participant_sessions_helper( - data, status_col, participant, session, expected_count + data, status_col, participant_id, session_id, expected_count ): doughnut = Doughnut(data) count = 0 for _ in doughnut._get_participant_sessions_helper( - status_col=status_col, participant=participant, session=session + status_col=status_col, participant_id=participant_id, session_id=session_id ): count += 1 assert count == expected_count @@ -145,25 +151,25 @@ def test_get_participant_sessions_helper( ), [ ( - {"01": ["ses-BL", "ses-M12"], "02": ["ses-BL", "ses-M12"]}, + {"01": ["BL", "M12"], "02": ["BL", "M12"]}, { - "01": ["ses-BL", "ses-M12"], - "02": ["ses-BL", "ses-M12"], - "03": ["ses-BL", "ses-M12"], + "01": ["BL", "M12"], + "02": ["BL", "M12"], + "03": ["BL", "M12"], }, - {"01": ["ses-BL", "ses-M12"], "02": ["ses-BL"]}, - {"01": ["ses-BL"], "02": ["ses-BL"], "03": ["ses-BL"]}, - {"01": ["ses-BL", "ses-M12"], "03": ["ses-M12"]}, + {"01": ["BL", "M12"], "02": ["BL"]}, + {"01": ["BL"], "02": ["BL"], "03": ["BL"]}, + {"01": ["BL", "M12"], "03": ["M12"]}, "downloaded", "organized", "bidsified", ), ( - {"PD01": ["ses-BL"], "PD02": ["ses-BL"]}, - {"PD01": ["ses-BL", "ses-M12"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL", "ses-M12"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL"], "PD02": ["ses-BL"]}, + {"PD01": ["BL"], "PD02": ["BL"]}, + {"PD01": ["BL", "M12"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL", "M12"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL"], "PD02": ["BL"]}, Path("scratch", "raw_dicom"), Path("dicom"), Path("bids"), @@ -256,8 +262,8 @@ def test_generate_and_update( def test_generate_missing_paths(tmp_path: Path): participants_and_sessions = { - "01": ["ses-BL", "ses-M12"], - "02": ["ses-BL", "ses-M12"], + "01": ["BL", "M12"], + "02": ["BL", "M12"], } dpath_root = tmp_path / "my_dataset" @@ -286,6 +292,6 @@ def test_generate_missing_paths(tmp_path: Path): empty=False, ) - assert doughnut[Doughnut.col_downloaded].all() - assert (~doughnut[Doughnut.col_organized]).all() - assert doughnut[Doughnut.col_bidsified].all() + assert doughnut[Doughnut.col_in_raw_imaging].all() + assert (~doughnut[Doughnut.col_in_sourcedata]).all() + assert doughnut[Doughnut.col_in_bids].all() diff --git a/nipoppy_cli/tests/test_tabular_manifest.py b/nipoppy_cli/tests/test_tabular_manifest.py index 14a5d488..dafe4c0d 100644 --- a/nipoppy_cli/tests/test_tabular_manifest.py +++ b/nipoppy_cli/tests/test_tabular_manifest.py @@ -49,19 +49,19 @@ def test_validate(fpath, is_valid): @pytest.mark.parametrize( - "sessions,visits,is_valid", + "session_ids,visit_ids,is_valid", [ (None, None, True), - (["ses-BL", "ses-M12"], ["BL", "M12"], True), - (["ses-BL"], ["BL", "M12"], False), - (["ses-BL", "ses-M12"], ["M12"], False), + (["BL", "M12"], ["BL", "M12"], True), + (["BL"], ["BL", "M12"], False), + (["BL", "M12"], ["M12"], False), ], ) -def test_validate_sessions_visits(sessions, visits, is_valid): +def test_validate_sessions_visits(session_ids, visit_ids, is_valid): manifest = Manifest.load( DPATH_TEST_DATA / "manifest1.csv", - sessions=sessions, - visits=visits, + session_ids=session_ids, + visit_ids=visit_ids, validate=False, ) with pytest.raises(ValueError) if not is_valid else nullcontext(): @@ -69,59 +69,56 @@ def test_validate_sessions_visits(sessions, visits, is_valid): @pytest.mark.parametrize( - "data,session,expected_count", + "data,session_id,expected_count", [ ( - { - "participant_id": ["01"], - "visit": ["BL"], - "session": [None], - "datatype": [[]], - }, - "ses-BL", + (["01"], ["BL"], None, [[]]), + "BL", 0, ), ( - { - "participant_id": ["01"], - "visit": ["BL"], - "session": ["ses-BL"], - "datatype": [["anat"]], - }, - "ses-BL", + (["01"], ["BL"], "BL", [["anat"]]), + "BL", 1, ), ( - { - "participant_id": ["01", "02"], - "visit": ["BL", "M12"], - "session": ["ses-BL", "ses-M12"], - "datatype": [["anat"], ["anat", "dwi"]], - }, - "ses-BL", + ( + ["01", "02"], + ["BL", "M12"], + ["BL", "M12"], + [["anat"], ["anat", "dwi"]], + ), + "BL", 1, ), ( - { - "participant_id": ["01", "02"], - "visit": ["BL", "M12"], - "session": ["ses-BL", "ses-M12"], - "datatype": [["anat"], ["anat", "dwi"]], - }, + ( + ["01", "02"], + ["BL", "M12"], + ["BL", "M12"], + [["anat"], ["anat", "dwi"]], + ), None, 2, ), ], ) -def test_get_imaging_subset(data, session, expected_count): - manifest = Manifest(data) - manifest_with_imaging_only = manifest.get_imaging_subset(session=session) +def test_get_imaging_subset(data, session_id, expected_count): + manifest = Manifest( + { + Manifest.col_participant_id: data[0], + Manifest.col_visit_id: data[1], + Manifest.col_session_id: data[2], + Manifest.col_datatype: data[3], + } + ) + manifest_with_imaging_only = manifest.get_imaging_subset(session_id=session_id) assert isinstance(manifest_with_imaging_only, Manifest) assert len(manifest_with_imaging_only) == expected_count @pytest.mark.parametrize( - "participant,session,expected_count", + "participant_id,session_id,expected_count", [ (None, None, 6), ("01", None, 3), @@ -135,12 +132,12 @@ def test_get_imaging_subset(data, session, expected_count): ("03", "ses-BL", 1), ], ) -def get_participants_sessions(participant, session, expected_count): +def get_participants_sessions(participant_id, session_id, expected_count): data = ( { "participant_id": ["01", "01", "01", "02", "02", "03", "04"], "visit": ["BL", "M12", "M24", "BL", "M12", "BL", "SC"], - "session": [ + "session_id": [ "ses-BL", "ses-M12", "ses-M24", @@ -163,7 +160,7 @@ def get_participants_sessions(participant, session, expected_count): manifest = Manifest(data) count = 0 for _ in manifest.get_participants_sessions( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ): count += 1 assert count == expected_count diff --git a/nipoppy_cli/tests/test_utils.py b/nipoppy_cli/tests/test_utils.py index d6e1b692..43646b8d 100644 --- a/nipoppy_cli/tests/test_utils.py +++ b/nipoppy_cli/tests/test_utils.py @@ -16,19 +16,15 @@ add_path_timestamp, add_pybids_ignore_patterns, apply_substitutions_to_json, - check_participant, - check_participant_id_strict, - check_session, - check_session_strict, - dicom_id_to_bids_id, + check_participant_id, + check_session_id, get_pipeline_tag, load_json, - participant_id_to_bids_id, - participant_id_to_dicom_id, + participant_id_to_bids_participant, process_template_str, save_df_with_backup, save_json, - strip_session, + session_id_to_bids_session, ) from .conftest import datetime_fixture # noqa F401 @@ -36,70 +32,58 @@ @pytest.mark.parametrize( - "participant_id,expected", - [("123", "123"), ("P_123", "P123"), ("sub!@#-", "sub")], + "participant_id,expected", [("123", "sub-123"), ("sub01", "sub-sub01")] ) -def test_participant_id_to_dicom_id(participant_id, expected): - assert participant_id_to_dicom_id(participant_id) == expected +def test_participant_id_to_bids_participant(participant_id, expected): + assert participant_id_to_bids_participant(participant_id) == expected @pytest.mark.parametrize( - "dicom_id,expected", - [("123", "sub-123"), ("P123", "sub-P123"), ("sub", "sub-sub")], -) -def test_dicom_id_to_bids_id(dicom_id, expected): - assert dicom_id_to_bids_id(dicom_id) == expected - - -@pytest.mark.parametrize( - "participant_id,expected", - [("123", "sub-123"), ("P_123", "sub-P123"), ("sub!@#-", "sub-sub")], + "session,expected", + [("BL", "ses-BL"), ("M12", "ses-M12"), (None, None)], ) -def test_participant_id_to_bids_id(participant_id, expected): - assert participant_id_to_bids_id(participant_id) == expected +def test_session_id_to_bids_session(session, expected): + assert session_id_to_bids_session(session) == expected @pytest.mark.parametrize( - "participant,expected", - [("sub-01", "01"), ("01", "01"), (None, None)], + "participant_id,raise_error,is_valid,expected", + [ + ("sub-01", False, True, "01"), + ("01", False, True, "01"), + (None, False, True, None), + ("sub-01", True, False, None), + ("01", True, True, "01"), + (None, True, True, None), + ], ) -def test_check_participant(participant, expected): - assert check_participant(participant) == expected - - -@pytest.mark.parametrize("participant_id,is_valid", [("01", True), ("sub-01", False)]) -def test_check_participant_id_strict(participant_id, is_valid): +def test_check_participant_id(participant_id, raise_error, is_valid, expected): with ( pytest.raises(ValueError, match="Participant ID should not start with") if not is_valid else nullcontext() ): - assert check_participant_id_strict(participant_id) == participant_id + assert check_participant_id(participant_id, raise_error=raise_error) == expected @pytest.mark.parametrize( - "session,expected", - [("ses-BL", "ses-BL"), ("BL", "ses-BL"), ("M12", "ses-M12"), (None, None)], + "session_id,raise_error,is_valid,expected", + [ + ("ses-BL", False, True, "BL"), + ("M12", False, True, "M12"), + (None, False, True, None), + ("ses-1", True, False, None), + ("1", True, True, "1"), + (None, True, True, None), + ], ) -def test_check_session(session, expected): - assert check_session(session) == expected - - -@pytest.mark.parametrize("session,is_valid", [("ses-1", True), ("1", False)]) -def test_check_session_strict(session, is_valid): +def test_check_session_id(session_id, raise_error, is_valid, expected): with ( - pytest.raises(ValueError, match="Session should start with") + pytest.raises(ValueError, match="Session ID should not start with") if not is_valid else nullcontext() ): - assert check_session_strict(session) == session - - -@pytest.mark.parametrize( - "session,expected", [("ses-BL", "BL"), ("BL", "BL"), ("ses-01", "01"), (None, None)] -) -def test_strip_session(session, expected): - assert strip_session(session) == expected + assert check_session_id(session_id, raise_error=raise_error) == expected @pytest.mark.parametrize( @@ -165,22 +149,22 @@ def test_add_pybids_ignore_patterns(orig_patterns, new_patterns, expected): @pytest.mark.parametrize( - "name,version,step,participant,session,expected", + "name,version,step,participant_id,session_id,expected", [ ("my_pipeline", "1.0", None, None, None, "my_pipeline-1.0"), ("pipeline", "2.0", None, "3000", None, "pipeline-2.0-3000"), - ("pipeline", "2.0", None, None, "ses-BL", "pipeline-2.0-BL"), + ("pipeline", "2.0", None, None, "BL", "pipeline-2.0-BL"), ("pipeline", "2.0", "step1", "3000", "BL", "pipeline-2.0-step1-3000-BL"), ], ) -def test_get_pipeline_tag(name, version, participant, step, session, expected): +def test_get_pipeline_tag(name, version, participant_id, step, session_id, expected): assert ( get_pipeline_tag( pipeline_name=name, pipeline_version=version, pipeline_step=step, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) == expected ) diff --git a/nipoppy_cli/tests/test_workflow_base.py b/nipoppy_cli/tests/test_workflow_base.py index ddff7051..c9fdfb2f 100644 --- a/nipoppy_cli/tests/test_workflow_base.py +++ b/nipoppy_cli/tests/test_workflow_base.py @@ -152,7 +152,7 @@ def test_manifest(workflow: BaseWorkflow): workflow.layout.fpath_manifest.parent.mkdir(parents=True, exist_ok=True) shutil.copy(FPATH_SAMPLE_MANIFEST, workflow.layout.fpath_manifest) config = get_config( - visits=["BL", "M12"], + visit_ids=["BL", "M12"], ) workflow.layout.fpath_config.parent.mkdir(parents=True, exist_ok=True) config.save(workflow.layout.fpath_config) diff --git a/nipoppy_cli/tests/test_workflow_bids_conversion.py b/nipoppy_cli/tests/test_workflow_bids_conversion.py index ec0b0db2..0501ef0d 100644 --- a/nipoppy_cli/tests/test_workflow_bids_conversion.py +++ b/nipoppy_cli/tests/test_workflow_bids_conversion.py @@ -43,62 +43,32 @@ def test_setup(config: Config, tmp_path: Path): @pytest.mark.parametrize( - "doughnut_data,participant,session,expected", + "doughnut_data,participant_id,session_id,expected", [ ( [ - { - Doughnut.col_participant_id: "S01", - Doughnut.col_session: "ses-1", - Doughnut.col_organized: True, - Doughnut.col_bidsified: False, - }, - { - Doughnut.col_participant_id: "S01", - Doughnut.col_session: "ses-2", - Doughnut.col_organized: True, - Doughnut.col_bidsified: True, - }, - { - Doughnut.col_participant_id: "S02", - Doughnut.col_session: "ses-3", - Doughnut.col_organized: False, - Doughnut.col_bidsified: False, - }, + ["S01", "1", True, False], + ["S01", "2", True, True], + ["S02", "3", False, False], ], None, None, - [("S01", "ses-1")], + [("S01", "1")], ), ( [ - { - Doughnut.col_participant_id: "P01", - Doughnut.col_session: "ses-A", - Doughnut.col_organized: True, - Doughnut.col_bidsified: False, - }, - { - Doughnut.col_participant_id: "P01", - Doughnut.col_session: "ses-B", - Doughnut.col_organized: True, - Doughnut.col_bidsified: False, - }, - { - Doughnut.col_participant_id: "P02", - Doughnut.col_session: "ses-B", - Doughnut.col_organized: True, - Doughnut.col_bidsified: False, - }, + ["P01", "A", True, False], + ["P01", "B", True, False], + ["P02", "B", True, False], ], "P01", - "ses-B", - [("P01", "ses-B")], + "B", + [("P01", "B")], ), ], ) def test_get_participants_sessions_to_run( - doughnut_data, participant, session, expected, tmp_path: Path + doughnut_data, participant_id, session_id, expected, tmp_path: Path ): workflow = BidsConversionRunner( dpath_root=tmp_path / "my_dataset", @@ -109,13 +79,14 @@ def test_get_participants_sessions_to_run( workflow.doughnut = Doughnut().add_or_update_records( records=[ { - **data, - Doughnut.col_visit: data[Doughnut.col_session], + Doughnut.col_participant_id: data[0], + Doughnut.col_session_id: data[1], + Doughnut.col_in_sourcedata: data[2], + Doughnut.col_in_bids: data[3], + Doughnut.col_visit_id: data[1], Doughnut.col_datatype: None, Doughnut.col_participant_dicom_dir: "", - Doughnut.col_dicom_id: "", - Doughnut.col_bids_id: "", - Doughnut.col_downloaded: False, + Doughnut.col_in_raw_imaging: False, } for data in doughnut_data ] @@ -123,6 +94,6 @@ def test_get_participants_sessions_to_run( assert [ tuple(x) for x in workflow.get_participants_sessions_to_run( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) ] == expected diff --git a/nipoppy_cli/tests/test_workflow_dicom_reorg.py b/nipoppy_cli/tests/test_workflow_dicom_reorg.py index e83daf7a..084a92c1 100644 --- a/nipoppy_cli/tests/test_workflow_dicom_reorg.py +++ b/nipoppy_cli/tests/test_workflow_dicom_reorg.py @@ -8,6 +8,7 @@ from nipoppy.tabular.dicom_dir_map import DicomDirMap from nipoppy.tabular.manifest import Manifest +from nipoppy.utils import participant_id_to_bids_participant, session_id_to_bids_session from nipoppy.workflows.dicom_reorg import DicomReorgWorkflow, is_derived_dicom from .conftest import DPATH_TEST_DATA, create_empty_dataset, get_config, prepare_dataset @@ -25,24 +26,24 @@ def test_is_derived_dicom(fpath, expected_result): @pytest.mark.parametrize( - "participant,session,fpaths,participant_first", + "participant_id,session_id,fpaths,participant_first", [ - ("01", "ses-1", ["01/ses-1/file1.dcm", "01/ses-1/file2.dcm"], True), + ("01", "1", ["01/1/file1.dcm", "01/1/file2.dcm"], True), ( "02", - "ses-2", - ["ses-2/02/001.dcm", "ses-2/02/002.dcm", "ses-2/02/003.dcm"], + "2", + ["2/02/001.dcm", "2/02/002.dcm", "2/02/003.dcm"], False, ), ], ) def test_get_fpaths_to_reorg( - participant, session, fpaths, participant_first, tmp_path: Path + participant_id, session_id, fpaths, participant_first, tmp_path: Path ): dpath_root = tmp_path / "my_dataset" manifest = prepare_dataset( - participants_and_sessions_manifest={participant: [session]} + participants_and_sessions_manifest={participant_id: [session_id]} ) workflow = DicomReorgWorkflow(dpath_root=dpath_root) @@ -50,43 +51,45 @@ def test_get_fpaths_to_reorg( manifest=manifest, fpath_dicom_dir_map=None, participant_first=participant_first ) for fpath in fpaths: - fpath_full: Path = workflow.layout.dpath_raw_dicom / fpath + fpath_full: Path = workflow.layout.dpath_raw_imaging / fpath fpath_full.parent.mkdir(parents=True, exist_ok=True) fpath_full.touch() assert len( workflow.get_fpaths_to_reorg( - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) ) == len(fpaths) def test_get_fpaths_to_reorg_error_not_found(tmp_path: Path): dpath_root = tmp_path / "my_dataset" - participant = "XXX" - session = "ses-X" + participant_id = "XXX" + session_id = "X" workflow = DicomReorgWorkflow(dpath_root=dpath_root) manifest = prepare_dataset( - participants_and_sessions_manifest={participant: [session]} + participants_and_sessions_manifest={participant_id: [session_id]} ) workflow.dicom_dir_map = DicomDirMap.load_or_generate( manifest=manifest, fpath_dicom_dir_map=None, participant_first=True ) with pytest.raises(FileNotFoundError, match="Raw DICOM directory not found"): - workflow.get_fpaths_to_reorg("XXX", "ses-X") + workflow.get_fpaths_to_reorg("XXX", "X") @pytest.mark.parametrize( "mapping_func,expected", [ - (lambda fname, participant, session: fname, "123456.dcm"), - (lambda fname, participant, session: "dicoms.tar.gz", "dicoms.tar.gz"), + (lambda fname, participant_id, session_id: fname, "123456.dcm"), + (lambda fname, participant_id, session_id: "dicoms.tar.gz", "dicoms.tar.gz"), ( - lambda fname, participant, session: f"{participant}-{session}.tar.gz", - "01-ses-1.tar.gz", + lambda fname, participant_id, session_id: ( + f"{participant_id}-{session_id}.tar.gz" + ), + "01-1.tar.gz", ), ], ) @@ -96,9 +99,9 @@ def test_apply_fname_mapping(mapping_func, expected, tmp_path: Path): workflow.apply_fname_mapping = mapping_func fname = "123456.dcm" - participant = "01" - session = "ses-1" - assert workflow.apply_fname_mapping(fname, participant, session) == expected + participant_id = "01" + session_id = "1" + assert workflow.apply_fname_mapping(fname, participant_id, session_id) == expected @pytest.mark.parametrize( @@ -115,21 +118,21 @@ def test_apply_fname_mapping_default(fname_source, expected, tmp_path: Path): assert ( workflow.apply_fname_mapping( - fname_source=fname_source, participant="", session="" + fname_source=fname_source, participant_id="", session_id="" ) == expected ) def test_run_single_error_file_exists(tmp_path: Path): - participant = "01" - session = "ses-1" + participant_id = "01" + session_id = "1" dataset_name = "my_dataset" workflow = DicomReorgWorkflow(dpath_root=tmp_path / dataset_name) manifest = prepare_dataset( - participants_and_sessions_manifest={participant: [session]} + participants_and_sessions_manifest={participant_id: [session_id]} ) workflow.dicom_dir_map = DicomDirMap.load_or_generate( manifest=manifest, fpath_dicom_dir_map=None, participant_first=True @@ -138,36 +141,41 @@ def test_run_single_error_file_exists(tmp_path: Path): # create the same file in both the downloaded and organized directories fname = "test.dcm" for fpath in [ - workflow.layout.dpath_raw_dicom / participant / session / fname, - workflow.layout.dpath_sourcedata / participant / session / fname, + workflow.layout.dpath_raw_imaging / participant_id / session_id / fname, + workflow.layout.dpath_sourcedata + / participant_id_to_bids_participant(participant_id) + / session_id_to_bids_session(session_id) + / fname, ]: fpath.parent.mkdir(parents=True, exist_ok=True) fpath.touch() with pytest.raises(FileExistsError, match="Cannot move file"): - workflow.run_single(participant, session) + workflow.run_single(participant_id, session_id) def test_run_single_invalid_dicom(tmp_path: Path, caplog: pytest.LogCaptureFixture): - participant = "01" - session = "ses-1" + participant_id = "01" + session_id = "1" dataset_name = "my_dataset" workflow = DicomReorgWorkflow(dpath_root=tmp_path / dataset_name, check_dicoms=True) manifest = prepare_dataset( - participants_and_sessions_manifest={participant: [session]} + participants_and_sessions_manifest={participant_id: [session_id]} ) workflow.dicom_dir_map = DicomDirMap.load_or_generate( manifest=manifest, fpath_dicom_dir_map=None, participant_first=True ) # use derived DICOM file - fpath_dicom = workflow.layout.dpath_raw_dicom / participant / session / "test.dcm" + fpath_dicom = ( + workflow.layout.dpath_raw_imaging / participant_id / session_id / "test.dcm" + ) fpath_dicom.parent.mkdir(parents=True, exist_ok=True) shutil.copyfile(DPATH_TEST_DATA / "dicom-derived.dcm", fpath_dicom) try: - workflow.run_single(participant, session) + workflow.run_single(participant_id, session_id) except Exception: pass @@ -181,13 +189,13 @@ def test_run_single_invalid_dicom(tmp_path: Path, caplog: pytest.LogCaptureFixtu def test_run_single_error_dicom_read(tmp_path: Path): - participant = "01" - session = "ses-1" + participant_id = "01" + session_id = "1" dataset_name = "my_dataset" workflow = DicomReorgWorkflow(dpath_root=tmp_path / dataset_name, check_dicoms=True) manifest = prepare_dataset( - participants_and_sessions_manifest={participant: [session]} + participants_and_sessions_manifest={participant_id: [session_id]} ) workflow.dicom_dir_map = DicomDirMap.load_or_generate( manifest=manifest, fpath_dicom_dir_map=None, participant_first=True @@ -195,12 +203,12 @@ def test_run_single_error_dicom_read(tmp_path: Path): # create an invalid DICOM file fname = "test.dcm" - fpath = workflow.layout.dpath_raw_dicom / participant / session / fname + fpath = workflow.layout.dpath_raw_imaging / participant_id / session_id / fname fpath.parent.mkdir(parents=True, exist_ok=True) fpath.touch() with pytest.raises(RuntimeError, match="Error checking DICOM file"): - workflow.run_single(participant, session) + workflow.run_single(participant_id, session_id) def test_copy_files_default(tmp_path: Path): @@ -224,38 +232,38 @@ def test_check_dicoms_default(tmp_path: Path): [ ( { - "S01": ["ses-1", "ses-2", "ses-3"], - "S02": ["ses-1", "ses-2"], - "S03": ["ses-3"], + "S01": ["1", "2", "3"], + "S02": ["1", "2"], + "S03": ["3"], }, { - "S03": ["ses-3"], + "S03": ["3"], }, [ - ("S01", "ses-1"), - ("S01", "ses-2"), - ("S01", "ses-3"), - ("S02", "ses-1"), - ("S02", "ses-2"), + ("S01", "1"), + ("S01", "2"), + ("S01", "3"), + ("S02", "1"), + ("S02", "2"), ], ), ( { - "S01": ["ses-1", "ses-2", "ses-3"], - "S02": ["ses-1", "ses-2", "ses-3"], - "S03": ["ses-1", "ses-2", "ses-3"], + "S01": ["1", "2", "3"], + "S02": ["1", "2", "3"], + "S03": ["1", "2", "3"], }, { - "S01": ["ses-1", "ses-3"], + "S01": ["1", "3"], }, [ - ("S01", "ses-2"), - ("S02", "ses-1"), - ("S02", "ses-2"), - ("S02", "ses-3"), - ("S03", "ses-1"), - ("S03", "ses-2"), - ("S03", "ses-3"), + ("S01", "2"), + ("S02", "1"), + ("S02", "2"), + ("S02", "3"), + ("S03", "1"), + ("S03", "2"), + ("S03", "3"), ], ), ], @@ -267,9 +275,9 @@ def test_get_participants_sessions_to_run( tmp_path: Path, ): participants_and_sessions_manifest = { - "S01": ["ses-1", "ses-2", "ses-3"], - "S02": ["ses-1", "ses-2", "ses-3"], - "S03": ["ses-1", "ses-2", "ses-3"], + "S01": ["1", "2", "3"], + "S02": ["1", "2", "3"], + "S03": ["1", "2", "3"], } dataset_name = "my_dataset" workflow = DicomReorgWorkflow(dpath_root=tmp_path / dataset_name) @@ -279,13 +287,13 @@ def test_get_participants_sessions_to_run( participants_and_sessions_manifest=participants_and_sessions_manifest, participants_and_sessions_downloaded=participants_and_sessions_downloaded, participants_and_sessions_organized=participants_and_sessions_organized, - dpath_downloaded=workflow.layout.dpath_raw_dicom, + dpath_downloaded=workflow.layout.dpath_raw_imaging, dpath_organized=workflow.layout.dpath_sourcedata, ) config = get_config( dataset_name=dataset_name, - visits=list(manifest[Manifest.col_visit].unique()), + visit_ids=list(manifest[Manifest.col_visit_id].unique()), ) manifest.save_with_backup(workflow.layout.fpath_manifest) @@ -296,8 +304,8 @@ def test_get_participants_sessions_to_run( def test_run_setup(tmp_path: Path): dataset_name = "my_dataset" - participants_and_sessions1 = {"01": ["ses-1"]} - participants_and_sessions2 = {"01": ["ses-1", "ses-2"], "02": ["ses-1"]} + participants_and_sessions1 = {"01": ["1"]} + participants_and_sessions2 = {"01": ["1", "2"], "02": ["1"]} workflow = DicomReorgWorkflow(dpath_root=tmp_path / dataset_name) create_empty_dataset(workflow.layout.dpath_root) @@ -309,7 +317,7 @@ def test_run_setup(tmp_path: Path): config = get_config( dataset_name=dataset_name, - visits=list(manifest1[Manifest.col_visit].unique()), + visit_ids=list(manifest1[Manifest.col_visit_id].unique()), ) config.save(workflow.layout.fpath_config) @@ -335,26 +343,26 @@ def test_run_setup(tmp_path: Path): [ ( { - "S01": ["ses-1", "ses-2", "ses-3"], - "S02": ["ses-1", "ses-2", "ses-3"], - "S03": ["ses-1", "ses-2", "ses-3"], + "S01": ["1", "2", "3"], + "S02": ["1", "2", "3"], + "S03": ["1", "2", "3"], }, { - "S01": ["ses-1", "ses-2", "ses-3"], - "S02": ["ses-1", "ses-2"], - "S03": ["ses-3"], + "S01": ["1", "2", "3"], + "S02": ["1", "2"], + "S03": ["3"], }, ), ( { - "P01": ["ses-BL"], - "P02": ["ses-V01"], - "P03": ["ses-V03"], + "P01": ["BL"], + "P02": ["V01"], + "P03": ["V03"], }, { - "P01": ["ses-BL"], - "P02": ["ses-BL", "ses-V01"], - "P03": ["ses-BL", "ses-V03"], + "P01": ["BL"], + "P02": ["BL", "V01"], + "P03": ["BL", "V03"], }, ), ], @@ -374,12 +382,12 @@ def test_run_main( manifest: Manifest = prepare_dataset( participants_and_sessions_manifest=participants_and_sessions_manifest, participants_and_sessions_downloaded=participants_and_sessions_downloaded, - dpath_downloaded=workflow.layout.dpath_raw_dicom, + dpath_downloaded=workflow.layout.dpath_raw_imaging, ) config = get_config( dataset_name=dataset_name, - visits=list(manifest[Manifest.col_visit].unique()), + visit_ids=list(manifest[Manifest.col_visit_id].unique()), ) manifest.save_with_backup(workflow.layout.fpath_manifest) @@ -387,15 +395,17 @@ def test_run_main( workflow.run_main() - for participant, sessions in participants_and_sessions_manifest.items(): - for session in sessions: + for participant_id, session_ids in participants_and_sessions_manifest.items(): + for session_id in session_ids: dpath_to_check: Path = ( - workflow.layout.dpath_sourcedata / participant / session + workflow.layout.dpath_sourcedata + / participant_id_to_bids_participant(participant_id) + / session_id_to_bids_session(session_id) ) if ( - participant in participants_and_sessions_downloaded - and session in participants_and_sessions_downloaded[participant] + participant_id in participants_and_sessions_downloaded + and session_id in participants_and_sessions_downloaded[participant_id] ): # check that directory exists assert dpath_to_check.exists() @@ -413,7 +423,9 @@ def test_run_main( # check that the doughnut has been updated assert workflow.doughnut.get_status( - participant, session, workflow.doughnut.col_organized + participant_id=participant_id, + session_id=session_id, + col=workflow.doughnut.col_in_sourcedata, ) else: diff --git a/nipoppy_cli/tests/test_workflow_doughnut.py b/nipoppy_cli/tests/test_workflow_doughnut.py index a99641c3..84324c64 100644 --- a/nipoppy_cli/tests/test_workflow_doughnut.py +++ b/nipoppy_cli/tests/test_workflow_doughnut.py @@ -29,22 +29,22 @@ ), [ ( - {"01": ["ses-BL", "ses-M12"], "02": ["ses-BL", "ses-M12"]}, + {"01": ["BL", "M12"], "02": ["BL", "M12"]}, { - "01": ["ses-BL", "ses-M12"], - "02": ["ses-BL", "ses-M12"], - "03": ["ses-BL", "ses-M12"], + "01": ["BL", "M12"], + "02": ["BL", "M12"], + "03": ["BL", "M12"], }, - {"01": ["ses-BL", "ses-M12"], "02": ["ses-BL"]}, - {"01": ["ses-BL"], "02": ["ses-BL"], "03": ["ses-BL"]}, - {"01": ["ses-BL", "ses-M12"], "03": ["ses-M12"]}, + {"01": ["BL", "M12"], "02": ["BL"]}, + {"01": ["BL"], "02": ["BL"], "03": ["BL"]}, + {"01": ["BL", "M12"], "03": ["M12"]}, ), ( - {"PD01": ["ses-BL"], "PD02": ["ses-BL"]}, - {"PD01": ["ses-BL", "ses-M12"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL", "ses-M12"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL"], "PD02": ["ses-BL"]}, + {"PD01": ["BL"], "PD02": ["BL"]}, + {"PD01": ["BL", "M12"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL", "M12"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL"], "PD02": ["BL"]}, ), ], ) @@ -60,7 +60,7 @@ def test_run( ): dpath_root = tmp_path / "my_dataset" - dpath_downloaded = dpath_root / ATTR_TO_DPATH_MAP["dpath_raw_dicom"] + dpath_downloaded = dpath_root / ATTR_TO_DPATH_MAP["dpath_raw_imaging"] dpath_organized = dpath_root / ATTR_TO_DPATH_MAP["dpath_sourcedata"] dpath_bidsified = dpath_root / ATTR_TO_DPATH_MAP["dpath_bids"] fpath_manifest = dpath_root / ATTR_TO_FPATH_MAP["fpath_manifest"] @@ -81,7 +81,7 @@ def test_run( # prepare config file config = get_config( - visits=list(manifest1[Manifest.col_visit].unique()), + visit_ids=list(manifest1[Manifest.col_visit_id].unique()), ) save_json(config.model_dump(mode="json"), fpath_config) @@ -127,16 +127,16 @@ def test_run( ), [ ( - {"01": ["ses-BL", "ses-M12"], "02": ["ses-BL", "ses-M12"]}, - {"01": ["ses-BL", "ses-M12"], "02": ["ses-BL"]}, - {"01": ["ses-BL"], "02": ["ses-BL"], "03": ["ses-BL"]}, - {"01": ["ses-BL", "ses-M12"], "03": ["ses-M12"]}, + {"01": ["BL", "M12"], "02": ["BL", "M12"]}, + {"01": ["BL", "M12"], "02": ["BL"]}, + {"01": ["BL"], "02": ["BL"], "03": ["BL"]}, + {"01": ["BL", "M12"], "03": ["M12"]}, ), ( - {"PD01": ["ses-BL", "ses-M12"], "PD02": ["ses-BL"]}, - {"PD01": ["ses-BL", "ses-M12"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL"], "PD02": ["ses-BL", "ses-M12"]}, - {"PD01": ["ses-BL"], "PD02": ["ses-BL"]}, + {"PD01": ["BL", "M12"], "PD02": ["BL"]}, + {"PD01": ["BL", "M12"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL"], "PD02": ["BL", "M12"]}, + {"PD01": ["BL"], "PD02": ["BL"]}, ), ], ) @@ -152,7 +152,7 @@ def test_run_regenerate( dpath_root = tmp_path / "my_dataset" create_empty_dataset(dpath_root) - dpath_downloaded = dpath_root / ATTR_TO_DPATH_MAP["dpath_raw_dicom"] + dpath_downloaded = dpath_root / ATTR_TO_DPATH_MAP["dpath_raw_imaging"] dpath_organized = dpath_root / ATTR_TO_DPATH_MAP["dpath_sourcedata"] dpath_bidsified = dpath_root / ATTR_TO_DPATH_MAP["dpath_bids"] fpath_manifest = dpath_root / ATTR_TO_FPATH_MAP["fpath_manifest"] @@ -172,26 +172,24 @@ def test_run_regenerate( # prepare config file config = get_config( - visits=list(manifest[Manifest.col_visit].unique()), + visit_ids=list(manifest[Manifest.col_visit_id].unique()), ) save_json(config.model_dump(mode="json"), fpath_config) # to be overwritten doughnut_records = [] for _, manifest_record in manifest.iterrows(): - participant = manifest_record[Manifest.col_participant_id] + participant_id = manifest_record[Manifest.col_participant_id] doughnut_records.append( { - Doughnut.col_participant_id: participant, - Doughnut.col_visit: manifest_record[Manifest.col_visit], - Doughnut.col_session: manifest_record[Manifest.col_session], + Doughnut.col_participant_id: participant_id, + Doughnut.col_visit_id: manifest_record[Manifest.col_visit_id], + Doughnut.col_session_id: manifest_record[Manifest.col_session_id], Doughnut.col_datatype: manifest_record[Manifest.col_datatype], - Doughnut.col_participant_dicom_dir: participant, - Doughnut.col_dicom_id: participant, - Doughnut.col_bids_id: f"sub-{participant}", - Doughnut.col_downloaded: True, - Doughnut.col_organized: True, - Doughnut.col_bidsified: True, + Doughnut.col_participant_dicom_dir: participant_id, + Doughnut.col_in_raw_imaging: True, + Doughnut.col_in_sourcedata: True, + Doughnut.col_in_bids: True, } ) doughnut_old = Doughnut(doughnut_records) diff --git a/nipoppy_cli/tests/test_workflow_pipeline.py b/nipoppy_cli/tests/test_workflow_pipeline.py index d2f2bcae..5db46381 100644 --- a/nipoppy_cli/tests/test_workflow_pipeline.py +++ b/nipoppy_cli/tests/test_workflow_pipeline.py @@ -12,7 +12,7 @@ from nipoppy.config.boutiques import BoutiquesConfig from nipoppy.config.pipeline import PipelineConfig from nipoppy.config.pipeline_step import PipelineStepConfig -from nipoppy.utils import StrOrPathLike, strip_session +from nipoppy.utils import StrOrPathLike from nipoppy.workflows.pipeline import BasePipelineWorkflow from .conftest import datetime_fixture # noqa F401 @@ -26,8 +26,8 @@ def __init__( pipeline_name: str, pipeline_version: Optional[str] = None, pipeline_step: Optional[str] = None, - participant: str = None, - session: str = None, + participant_id: str = None, + session_id: str = None, fpath_layout: Optional[StrOrPathLike] = None, logger: Optional[logging.Logger] = None, dry_run: bool = False, @@ -40,25 +40,25 @@ def __init__( pipeline_name=pipeline_name, pipeline_version=pipeline_version, pipeline_step=pipeline_step, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, fpath_layout=fpath_layout, logger=logger, dry_run=dry_run, ) def get_participants_sessions_to_run( - self, participant: Optional[str], session: Optional[str] + self, participant_id: Optional[str], session_id: Optional[str] ): - """Only run on participant/sessions with BIDS data.""" + """Only run on participant_id/sessions with BIDS data.""" return self.doughnut.get_bidsified_participants_sessions( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) - def run_single(self, subject: str, session: str): - """Run on a single participant/session.""" + def run_single(self, subject: str, session_id: str): + """Run on a single participant_id/session_id.""" self._n_runs += 1 - self.logger.info(f"Running on {subject}/{session}") + self.logger.info(f"Running on {subject}/{session_id}") if subject == "FAIL": self._n_errors += 1 raise RuntimeError("FAIL") @@ -73,7 +73,7 @@ def workflow(tmp_path: Path): ) # write config config = get_config( - visits=["1"], + visit_ids=["1"], bids_pipelines=[ # built-in pipelines { @@ -109,12 +109,6 @@ def workflow(tmp_path: Path): return workflow -def _make_dummy_json(fpath: StrOrPathLike): - fpath: Path = Path(fpath) - fpath.parent.mkdir(parents=True, exist_ok=True) - fpath.write_text("{}\n") - - @pytest.mark.parametrize( "args", [ @@ -136,14 +130,35 @@ def test_init(args): assert hasattr(workflow, "pipeline_name") assert hasattr(workflow, "pipeline_version") assert hasattr(workflow, "pipeline_step") - assert hasattr(workflow, "participant") - assert hasattr(workflow, "session") + assert hasattr(workflow, "participant_id") + assert hasattr(workflow, "session_id") assert isinstance(workflow.dpath_pipeline, Path) assert isinstance(workflow.dpath_pipeline_output, Path) assert isinstance(workflow.dpath_pipeline_work, Path) assert isinstance(workflow.dpath_pipeline_bids_db, Path) +@pytest.mark.parametrize( + "participant_id,session_id,participant_expected,session_expected", + [ + ("01", "BL", "01", "BL"), + ("sub-01", "ses-BL", "01", "BL"), + ], +) +def test_init_participant_session( + participant_id, session_id, participant_expected, session_expected +): + workflow = PipelineWorkflow( + dpath_root="my_dataset", + pipeline_name="my_pipeline", + pipeline_version="1.0", + participant_id=participant_id, + session_id=session_id, + ) + assert workflow.participant_id == participant_expected + assert workflow.session_id == session_expected + + def test_pipeline_version_optional(): workflow = PipelineWorkflow( dpath_root="my_dataset", @@ -348,13 +363,13 @@ class Test: processed = workflow.process_template_json( { - "[[NIPOPPY_BIDS_ID]]": "[[NIPOPPY_PARTICIPANT]]", - "[[NIPOPPY_SESSION]]": "[[NIPOPPY_SESSION_SHORT]]", + "[[NIPOPPY_BIDS_PARTICIPANT]]": "[[NIPOPPY_PARTICIPANT_ID]]", + "[[NIPOPPY_BIDS_SESSION]]": "[[NIPOPPY_SESSION_ID]]", "[[NIPOPPY_DPATH_PIPELINE]]": "[[NIPOPPY_DPATH_BIDS]]", "[[NIPOPPY_EXTRA1]]": "[[NIPOPPY_EXTRA2]]", }, - participant="01", - session="ses-1", + participant_id="01", + session_id="1", extra1="extra_kwarg", objs=[Test()], return_str=return_str, @@ -368,10 +383,10 @@ class Test: # check that everything was replaced for pattern in [ - "[[NIPOPPY_BIDS_ID]]", - "[[NIPOPPY_PARTICIPANT]]", - "[[NIPOPPY_SESSION]]", - "[[NIPOPPY_SESSION_SHORT]]", + "[[NIPOPPY_BIDS_PARTICIPANT]]", + "[[NIPOPPY_PARTICIPANT_ID]]", + "[[NIPOPPY_BIDS_SESSION]]", + "[[NIPOPPY_SESSION_ID]]", "[[NIPOPPY_DPATH_PIPELINE]]", "[[NIPOPPY_DPATH_BIDS]]", "[[NIPOPPY_EXTRA1]]", @@ -380,19 +395,21 @@ class Test: assert pattern not in processed -@pytest.mark.parametrize("participant,session", [("123", None), (None, "ses-1")]) -def test_process_template_json_error(participant, session, tmp_path: Path): +@pytest.mark.parametrize("participant_id,session_id", [("123", None), (None, "1")]) +def test_process_template_json_error(participant_id, session_id, tmp_path: Path): workflow = PipelineWorkflow( dpath_root=tmp_path / "my_dataset", pipeline_name="my_pipeline", pipeline_version="1.0", ) - with pytest.raises(ValueError, match="participant and session must be strings"): + with pytest.raises( + ValueError, match="participant_id and session_id must be strings" + ): workflow.process_template_json( {}, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) @@ -440,20 +457,24 @@ def test_boutiques_config_invalid(tmp_path: Path): @pytest.mark.parametrize( - "participant,session,expected_count", + "participant_id,session_id,expected_count", [ (None, None, 12), ("01", None, 6), ("02", None, 6), - (None, "ses-1", 5), - (None, "ses-2", 5), - (None, "ses-3", 2), - ("01", "ses-3", 2), - ("02", "ses-3", 0), + (None, "1", 5), + (None, "2", 5), + (None, "3", 2), + ("01", "3", 2), + ("02", "3", 0), ], ) def test_set_up_bids_db( - workflow: PipelineWorkflow, participant, session, expected_count, tmp_path: Path + workflow: PipelineWorkflow, + participant_id, + session_id, + expected_count, + tmp_path: Path, ): dpath_bids_db = tmp_path / "bids_db" fids.create_fake_bids_dataset( @@ -470,8 +491,8 @@ def test_set_up_bids_db( ) bids_layout = workflow.set_up_bids_db( dpath_bids_db=dpath_bids_db, - participant=participant, - session=strip_session(session), + participant_id=participant_id, + session_id=session_id, ) assert dpath_bids_db.exists() assert len(bids_layout.get(extension=".nii.gz")) == expected_count @@ -512,19 +533,19 @@ def test_run_setup_create_directories(dry_run: bool, tmp_path: Path): @pytest.mark.parametrize( - "participant,session,expected_count", - [(None, None, 4), ("01", None, 3), ("01", "ses-2", 1)], + "participant_id,session_id,expected_count", + [(None, None, 4), ("01", None, 3), ("01", "2", 1)], ) def test_run_main( workflow: PipelineWorkflow, - participant, - session, + participant_id, + session_id, expected_count, ): - workflow.participant = participant - workflow.session = session + workflow.participant_id = participant_id + workflow.session_id = session_id - participants_and_sessions = {"01": ["ses-1", "ses-2", "ses-3"], "02": ["ses-1"]} + participants_and_sessions = {"01": ["1", "2", "3"], "02": ["1"]} manifest = prepare_dataset( participants_and_sessions_manifest=participants_and_sessions, participants_and_sessions_bidsified=participants_and_sessions, @@ -536,10 +557,10 @@ def test_run_main( def test_run_main_catch_errors(workflow: PipelineWorkflow): - workflow.participant = "FAIL" - workflow.session = "ses-1" + workflow.participant_id = "FAIL" + workflow.session_id = "1" - participants_and_sessions = {workflow.participant: [workflow.session]} + participants_and_sessions = {workflow.participant_id: [workflow.session_id]} manifest = prepare_dataset( participants_and_sessions_manifest=participants_and_sessions, participants_and_sessions_bidsified=participants_and_sessions, @@ -552,7 +573,7 @@ def test_run_main_catch_errors(workflow: PipelineWorkflow): @pytest.mark.parametrize( - "pipeline_name,pipeline_version,participant,session,expected_stem", + "pipeline_name,pipeline_version,participant_id,session_id,expected_stem", [ ( "my_pipeline", @@ -568,22 +589,22 @@ def test_run_main_catch_errors(workflow: PipelineWorkflow): None, "test/my_pipeline-1.0/my_pipeline-1.0-sub1", ), - ("fmriprep", None, None, "ses-1", "test/fmriprep-23.1.3/fmriprep-23.1.3-1"), + ("fmriprep", None, None, "1", "test/fmriprep-23.1.3/fmriprep-23.1.3-1"), ], ) def test_generate_fpath_log( pipeline_name, pipeline_version, - participant, - session, + participant_id, + session_id, expected_stem, workflow: PipelineWorkflow, datetime_fixture, # noqa F811 ): workflow.pipeline_name = pipeline_name workflow.pipeline_version = pipeline_version - workflow.participant = participant - workflow.session = session + workflow.participant_id = participant_id + workflow.session_id = session_id fpath_log = workflow.generate_fpath_log() assert ( fpath_log == workflow.layout.dpath_logs / f"{expected_stem}-20240404_1234.log" diff --git a/nipoppy_cli/tests/test_workflow_runner.py b/nipoppy_cli/tests/test_workflow_runner.py index f268f6b1..47654cd7 100644 --- a/nipoppy_cli/tests/test_workflow_runner.py +++ b/nipoppy_cli/tests/test_workflow_runner.py @@ -9,7 +9,6 @@ from nipoppy.config.main import Config from nipoppy.tabular.bagel import Bagel from nipoppy.tabular.doughnut import Doughnut -from nipoppy.utils import strip_session from nipoppy.workflows.runner import PipelineRunner from .conftest import create_empty_dataset, get_config @@ -45,7 +44,7 @@ def config(tmp_path: Path): "custom": {"nipoppy": {"CONTAINER_SUBCOMMAND": "exec"}}, } invocation = { - "arg1": "[[NIPOPPY_PARTICIPANT]] [[NIPOPPY_SESSION]]", + "arg1": "[[NIPOPPY_PARTICIPANT_ID]] [[NIPOPPY_BIDS_SESSION]]", "arg2": 10, } @@ -53,7 +52,7 @@ def config(tmp_path: Path): fpath_invocation.write_text(json.dumps(invocation)) return get_config( - visits=["BL", "V04"], + visit_ids=["BL", "V04"], container_config={"COMMAND": "echo"}, # dummy command proc_pipelines=[ { @@ -93,24 +92,24 @@ def test_launch_boutiques_run(simulate, config: Config, tmp_path: Path): ) config.save(runner.layout.fpath_config) - participant = "01" - session = "ses-BL" + participant_id = "01" + session_id = "BL" fids.create_fake_bids_dataset( runner.layout.dpath_bids, - subjects=participant, - sessions=strip_session(session), + subjects=participant_id, + sessions=session_id, ) runner.dpath_pipeline_output.mkdir(parents=True, exist_ok=True) runner.dpath_pipeline_work.mkdir(parents=True, exist_ok=True) descriptor_str, invocation_str = runner.launch_boutiques_run( - participant, session, container_command="" + participant_id, session_id, container_command="" ) assert "[[NIPOPPY_DPATH_BIDS]]" not in descriptor_str - assert "[[NIPOPPY_PARTICIPANT]]" not in invocation_str - assert "[[NIPOPPY_SESSION]]" not in invocation_str + assert "[[NIPOPPY_PARTICIPANT_ID]]" not in invocation_str + assert "[[NIPOPPY_BIDS_SESSION]]" not in invocation_str def test_process_container_config_boutiques_subcommand(config: Config, tmp_path: Path): @@ -123,14 +122,16 @@ def test_process_container_config_boutiques_subcommand(config: Config, tmp_path: config.save(runner.layout.fpath_config) - participant = "01" - session = "ses-BL" + participant_id = "01" + session_id = "BL" # the container command in the config is "echo" # because otherwise the check for the container command fails # if Singularity/Apptainer is not on the PATH assert ( - runner.process_container_config(participant=participant, session=session) + runner.process_container_config( + participant_id=participant_id, session_id=session_id + ) == "echo exec" ) @@ -140,36 +141,36 @@ def test_process_container_config_boutiques_subcommand(config: Config, tmp_path: [ ( [ - ["01", "ses-1", False], - ["01", "ses-2", True], - ["01", "ses-3", True], + ["01", "1", False], + ["01", "2", True], + ["01", "3", True], ], None, "dummy_pipeline", "1.0.0", - [("01", "ses-2"), ("01", "ses-3")], + [("01", "2"), ("01", "3")], ), ( [ - ["01", "ses-1", False], - ["01", "ses-2", True], - ["01", "ses-3", True], + ["01", "1", False], + ["01", "2", True], + ["01", "3", True], ], [], "dummy_pipeline", "1.0.0", - [("01", "ses-2"), ("01", "ses-3")], + [("01", "2"), ("01", "3")], ), ( [ - ["01", "ses-1", False], - ["01", "ses-2", True], - ["01", "ses-3", True], + ["01", "1", False], + ["01", "2", True], + ["01", "3", True], ], [ - ["01", "ses-1", "dummy_pipeline", "1.0.0", Bagel.status_success], - ["01", "ses-2", "dummy_pipeline", "1.0.0", Bagel.status_success], - ["01", "ses-3", "dummy_pipeline", "1.0.0", Bagel.status_success], + ["01", "1", "dummy_pipeline", "1.0.0", Bagel.status_success], + ["01", "2", "dummy_pipeline", "1.0.0", Bagel.status_success], + ["01", "3", "dummy_pipeline", "1.0.0", Bagel.status_success], ], "dummy_pipeline", "1.0.0", @@ -177,35 +178,35 @@ def test_process_container_config_boutiques_subcommand(config: Config, tmp_path: ), ( [ - ["01", "ses-1", True], - ["01", "ses-2", True], - ["01", "ses-3", True], + ["01", "1", True], + ["01", "2", True], + ["01", "3", True], ], [ - ["01", "ses-1", "dummy_pipeline", "1.0.0", Bagel.status_fail], - ["01", "ses-2", "dummy_pipeline", "1.0.0", Bagel.status_success], - ["01", "ses-3", "dummy_pipeline", "1.0.0", Bagel.status_fail], - ["01", "ses-1", "dummy_pipeline", "2.0", Bagel.status_success], + ["01", "1", "dummy_pipeline", "1.0.0", Bagel.status_fail], + ["01", "2", "dummy_pipeline", "1.0.0", Bagel.status_success], + ["01", "3", "dummy_pipeline", "1.0.0", Bagel.status_fail], + ["01", "1", "dummy_pipeline", "2.0", Bagel.status_success], ], "dummy_pipeline", "1.0.0", - [("01", "ses-1"), ("01", "ses-3")], + [("01", "1"), ("01", "3")], ), ( [ - ["01", "ses-1", True], - ["01", "ses-2", True], - ["01", "ses-3", True], + ["01", "1", True], + ["01", "2", True], + ["01", "3", True], ], [ - ["01", "ses-1", "dummy_pipeline", "1.0.0", Bagel.status_fail], - ["01", "ses-2", "dummy_pipeline", "1.0.0", Bagel.status_success], - ["01", "ses-3", "dummy_pipeline", "1.0.0", Bagel.status_fail], - ["01", "ses-1", "dummy_pipeline", "2.0", Bagel.status_success], + ["01", "1", "dummy_pipeline", "1.0.0", Bagel.status_fail], + ["01", "2", "dummy_pipeline", "1.0.0", Bagel.status_success], + ["01", "3", "dummy_pipeline", "1.0.0", Bagel.status_fail], + ["01", "1", "dummy_pipeline", "2.0", Bagel.status_success], ], "dummy_pipeline", None, - [("01", "ses-1"), ("01", "ses-3")], + [("01", "1"), ("01", "3")], ), ], ) @@ -218,29 +219,27 @@ def test_get_participants_sessions_to_run( config: Config, tmp_path: Path, ): - participant = None - session = None + participant_id = None + session_id = None runner = PipelineRunner( dpath_root=tmp_path, pipeline_name=pipeline_name, pipeline_version=pipeline_version, - participant=participant, - session=session, + participant_id=participant_id, + session_id=session_id, ) config.save(runner.layout.fpath_config) runner.doughnut = Doughnut().add_or_update_records( records=[ { Doughnut.col_participant_id: data[0], - Doughnut.col_session: data[1], - Doughnut.col_visit: data[1], - Doughnut.col_bidsified: data[2], + Doughnut.col_session_id: data[1], + Doughnut.col_visit_id: data[1], + Doughnut.col_in_bids: data[2], Doughnut.col_datatype: None, Doughnut.col_participant_dicom_dir: "", - Doughnut.col_dicom_id: "", - Doughnut.col_bids_id: "", - Doughnut.col_downloaded: False, - Doughnut.col_organized: False, + Doughnut.col_in_raw_imaging: False, + Doughnut.col_in_sourcedata: False, } for data in doughnut_data ] @@ -250,7 +249,7 @@ def test_get_participants_sessions_to_run( bagel_data, columns=[ Bagel.col_participant_id, - Bagel.col_session, + Bagel.col_session_id, Bagel.col_pipeline_name, Bagel.col_pipeline_version, Bagel.col_pipeline_complete, @@ -260,6 +259,6 @@ def test_get_participants_sessions_to_run( assert [ tuple(x) for x in runner.get_participants_sessions_to_run( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) ] == expected diff --git a/nipoppy_cli/tests/test_workflow_tracker.py b/nipoppy_cli/tests/test_workflow_tracker.py index 79f71175..d8efcf10 100644 --- a/nipoppy_cli/tests/test_workflow_tracker.py +++ b/nipoppy_cli/tests/test_workflow_tracker.py @@ -21,8 +21,8 @@ def tracker(tmp_path: Path): pipeline_name = "test_pipeline" pipeline_version = "0.1.0" participants_and_sessions = { - "01": ["ses-1", "ses-2"], - "02": ["ses-1", "ses-2"], + "01": ["1", "2"], + "02": ["1", "2"], } tracker = PipelineTracker( @@ -44,7 +44,7 @@ def tracker(tmp_path: Path): { "NAME": "pipeline_complete", "PATHS": [ - "[[NIPOPPY_PARTICIPANT]]/[[NIPOPPY_SESSION]]/results.txt", + "[[NIPOPPY_PARTICIPANT_ID]]/[[NIPOPPY_BIDS_SESSION]]/results.txt", "file.txt", ], }, @@ -52,7 +52,7 @@ def tracker(tmp_path: Path): fpath_tracker_config.write_text(json.dumps(tracker_config)) config: Config = get_config( - visits=["1", "2"], + visit_ids=["1", "2"], proc_pipelines=[ { "NAME": pipeline_name, @@ -75,7 +75,7 @@ def test_run_setup_existing_bagel(tracker: PipelineTracker): bagel = Bagel( data={ Bagel.col_participant_id: ["01"], - Bagel.col_session: ["ses-1"], + Bagel.col_session_id: ["1"], Bagel.col_pipeline_name: ["some_pipeline"], Bagel.col_pipeline_version: ["some_version"], Bagel.col_pipeline_complete: [Bagel.status_success], @@ -105,32 +105,32 @@ def test_check_status(tracker: PipelineTracker, relative_paths, expected_status) @pytest.mark.parametrize( - "doughnut_data,participant,session,expected", + "doughnut_data,participant_id,session_id,expected", [ ( [ - ["S01", "ses-1", False], - ["S01", "ses-2", True], - ["S02", "ses-3", False], + ["S01", "1", False], + ["S01", "2", True], + ["S02", "3", False], ], None, None, - [("S01", "ses-2")], + [("S01", "2")], ), ( [ - ["P01", "ses-A", False], - ["P01", "ses-B", True], - ["P02", "ses-B", True], + ["P01", "A", False], + ["P01", "B", True], + ["P02", "B", True], ], "P01", - "ses-B", - [("P01", "ses-B")], + "B", + [("P01", "B")], ), ], ) def test_get_participants_sessions_to_run( - doughnut_data, participant, session, expected, tmp_path: Path + doughnut_data, participant_id, session_id, expected, tmp_path: Path ): tracker = PipelineTracker( dpath_root=tmp_path, @@ -141,15 +141,13 @@ def test_get_participants_sessions_to_run( records=[ { Doughnut.col_participant_id: data[0], - Doughnut.col_session: data[1], - Doughnut.col_visit: data[1], - Doughnut.col_bidsified: data[2], + Doughnut.col_session_id: data[1], + Doughnut.col_visit_id: data[1], + Doughnut.col_in_bids: data[2], Doughnut.col_datatype: None, Doughnut.col_participant_dicom_dir: "", - Doughnut.col_dicom_id: "", - Doughnut.col_bids_id: "", - Doughnut.col_downloaded: False, - Doughnut.col_organized: False, + Doughnut.col_in_raw_imaging: False, + Doughnut.col_in_sourcedata: False, } for data in doughnut_data ] @@ -158,16 +156,18 @@ def test_get_participants_sessions_to_run( assert [ tuple(x) for x in tracker.get_participants_sessions_to_run( - participant=participant, session=session + participant_id=participant_id, session_id=session_id ) ] == expected @pytest.mark.parametrize( - "participant,session,expected_status", - [("01", "ses-1", Bagel.status_success), ("02", "ses-2", Bagel.status_fail)], + "participant_id,session_id,expected_status", + [("01", "1", Bagel.status_success), ("02", "2", Bagel.status_fail)], ) -def test_run_single(participant, session, expected_status, tracker: PipelineTracker): +def test_run_single( + participant_id, session_id, expected_status, tracker: PipelineTracker +): for relative_path_to_write in [ "01/ses-1/results.txt", "file.txt", @@ -177,10 +177,10 @@ def test_run_single(participant, session, expected_status, tracker: PipelineTrac fpath.mkdir(parents=True, exist_ok=True) fpath.touch() - assert tracker.run_single(participant, session) == expected_status + assert tracker.run_single(participant_id, session_id) == expected_status assert ( - tracker.bagel.set_index([Bagel.col_participant_id, Bagel.col_session]) + tracker.bagel.set_index([Bagel.col_participant_id, Bagel.col_session_id]) .loc[:, Bagel.col_pipeline_complete] .item() ) == expected_status @@ -194,7 +194,7 @@ def test_run_single_multiple_configs( {"NAME": "tracker2", "PATHS": ["path2"]}, ] tracker.pipeline_config.TRACKER_CONFIG_FILE.write_text(json.dumps(tracker_configs)) - tracker.run_single("01", "ses-1") + tracker.run_single("01", "1") assert any( [ @@ -208,7 +208,7 @@ def test_run_single_multiple_configs( def test_run_single_no_config(tracker: PipelineTracker): tracker.pipeline_config.TRACKER_CONFIG_FILE = None with pytest.raises(ValueError, match="No tracker config file specified for"): - tracker.run_single("01", "ses-1") + tracker.run_single("01", "1") @pytest.mark.parametrize( @@ -218,7 +218,7 @@ def test_run_single_no_config(tracker: PipelineTracker): Bagel( data={ Bagel.col_participant_id: ["01"], - Bagel.col_session: ["ses-1"], + Bagel.col_session_id: ["1"], Bagel.col_pipeline_name: ["some_pipeline"], Bagel.col_pipeline_version: ["some_version"], Bagel.col_pipeline_complete: [Bagel.status_success],