From 64b209766acd2d0cde27143f545adb6f92b463f4 Mon Sep 17 00:00:00 2001 From: Tien Tong Date: Fri, 17 Jan 2025 11:48:29 -0500 Subject: [PATCH 1/8] Add is_longitudinal attribute to the CuBIDS class --- cubids/cubids.py | 88 ++++++++++++++++++++++++++++++-------- cubids/metadata_merge.py | 36 ++++++++++------ cubids/tests/test_apply.py | 27 +++++++----- cubids/tests/test_bond.py | 7 +-- cubids/workflows.py | 4 ++ 5 files changed, 119 insertions(+), 43 deletions(-) diff --git a/cubids/cubids.py b/cubids/cubids.py index 158e97d05..3329444ff 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -84,6 +84,8 @@ class CuBIDS(object): A data dictionary for TSV outputs. use_datalad : :obj:`bool` If True, use datalad to track changes to the BIDS dataset. + is_longitudinal : :obj:`bool` + If True, includes "ses" in filepath. Default is False. """ def __init__( @@ -93,6 +95,7 @@ def __init__( acq_group_level="subject", grouping_config=None, force_unlock=False, + is_longitudinal=False, ): self.path = os.path.abspath(data_root) self._layout = None @@ -110,10 +113,11 @@ def __init__( self.cubids_code_dir = Path(self.path + "/code/CuBIDS").is_dir() self.data_dict = {} # data dictionary for TSV outputs self.use_datalad = use_datalad # True if flag set, False if flag unset + self.is_longitudinal = is_longitudinal # True if flag set, False if flag unset if self.use_datalad: self.init_datalad() - if self.acq_group_level == "session": + if self.is_longitudinal and self.acq_group_level == "session": NON_KEY_ENTITIES.remove("session") @property @@ -452,7 +456,7 @@ def apply_tsv_changes(self, summary_tsv, files_tsv, new_prefix, raise_on_error=T # remove renames file that gets created under the hood subprocess.run(["rm", "-rf", "renames"]) - def change_filename(self, filepath, entities): + def change_filename(self, filepath, entities, is_longitudinal=False): """Apply changes to a filename based on the renamed entity sets. This function takes into account the new entity set names @@ -464,6 +468,8 @@ def change_filename(self, filepath, entities): Path prefix to a file in the affected entity set change. entities : :obj:`dict` A pybids dictionary of entities parsed from the new entity set name. + is_longitudinal : :obj:`bool`, optional + If True, includes "ses" in filepath. Default is False. Notes ----- @@ -473,6 +479,7 @@ def change_filename(self, filepath, entities): filepath=filepath, entities=entities, out_dir=str(self.path), + is_longitudinal=self.is_longitudinal ) exts = Path(filepath).suffixes @@ -481,7 +488,8 @@ def change_filename(self, filepath, entities): suffix = entities["suffix"] sub = get_entity_value(filepath, "sub") - ses = get_entity_value(filepath, "ses") + if self.is_longitudinal: + ses = get_entity_value(filepath, "ses") # Add the scan path + new path to the lists of old, new filenames self.old_filenames.append(filepath) @@ -577,7 +585,10 @@ def change_filename(self, filepath, entities): self.new_filenames.append(new_labeling) # RENAME INTENDED FORS! - ses_path = self.path + "/" + sub + "/" + ses + if self.is_longitudinal: + ses_path = self.path + "/" + sub + "/" + ses + elif not self.is_longitudinal: + ses_path = self.path + "/" + sub files_with_if = [] files_with_if += Path(ses_path).rglob("fmap/*.json") files_with_if += Path(ses_path).rglob("perf/*_m0scan.json") @@ -595,7 +606,7 @@ def change_filename(self, filepath, entities): # Coerce IntendedFor to a list. data["IntendedFor"] = listify(data["IntendedFor"]) for item in data["IntendedFor"]: - if item == _get_participant_relative_path(filepath): + if item == _get_participant_relative_path(filepath): # remove old filename data["IntendedFor"].remove(item) # add new filename @@ -1363,6 +1374,7 @@ def get_layout(self): return self.layout +# XXX: Remove _validate_json? def _validate_json(): """Validate a JSON file's contents. @@ -1402,8 +1414,29 @@ def _get_participant_relative_path(scan): This is what will appear in the IntendedFor field of any association. + Examples: + >>> _get_participant_relative_path( + ... "/path/to/dset/sub-01/ses-01/func/sub-01_ses-01_bold.nii.gz", + ... ) + 'ses-01/func/sub-01_ses-01_bold.nii.gz' + + >>> _get_participant_relative_path( + ... "/path/to/dset/sub-01/func/sub-01_bold.nii.gz", + ... ) + 'func/sub-01_bold.nii.gz' + + >>> _get_participant_relative_path( + ... "/path/to/dset/ses-01/func/ses-01_bold.nii.gz", + ... ) + Traceback (most recent call last): + ValueError: Could not find subject in ... """ - return "/".join(Path(scan).parts[-3:]) + parts = Path(scan).parts + # Find the first part that starts with "sub-" + for i, part in enumerate(parts): + if part.startswith("sub-"): + return "/".join(parts[i+1:]) + raise ValueError(f"Could not find subject in {scan}") def _get_bidsuri(filename, dataset_root): @@ -1734,7 +1767,7 @@ def get_entity_value(path, key): return part -def build_path(filepath, entities, out_dir): +def build_path(filepath, entities, out_dir, is_longitudinal=False): """Build a new path for a file based on its BIDS entities. Parameters @@ -1746,6 +1779,8 @@ def build_path(filepath, entities, out_dir): This should include all of the entities in the filename *except* for subject and session. out_dir : str The output directory for the new file. + is_longitudinal : bool, optional + If True, add "ses" to file path. Default is False. Returns ------- @@ -1758,6 +1793,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/anat/sub-01_ses-01_T1w.nii.gz", ... {"acquisition": "VAR", "suffix": "T2w"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/anat/sub-01_ses-01_acq-VAR_T2w.nii.gz' @@ -1766,6 +1802,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/func/sub-01_ses-01_task-rest_run-01_bold.nii.gz", ... {"task": "rest", "run": "2", "acquisition": "VAR", "suffix": "bold"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/func/sub-01_ses-01_task-rest_acq-VAR_run-2_bold.nii.gz' @@ -1775,6 +1812,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/func/sub-01_ses-01_task-rest_run-00001_bold.nii.gz", ... {"task": "rest", "run": 2, "acquisition": "VAR", "suffix": "bold"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/func/sub-01_ses-01_task-rest_acq-VAR_run-00002_bold.nii.gz' @@ -1784,6 +1822,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/func/sub-01_ses-01_task-rest_run-1_bold.nii.gz", ... {"task": "rest", "run": 2, "acquisition": "VAR", "suffix": "bold"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/func/sub-01_ses-01_task-rest_acq-VAR_run-2_bold.nii.gz' @@ -1792,6 +1831,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/func/sub-01_ses-01_task-rest_run-1_bold.nii.gz", ... {"task": "rest", "run": "2", "acquisition": "VAR", "suffix": "bold"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/func/sub-01_ses-01_task-rest_acq-VAR_run-2_bold.nii.gz' @@ -1801,6 +1841,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/func/sub-01_ses-01_task-rest_run-01_bold.nii.gz", ... {"task": "rest", "acquisition": "VAR", "suffix": "bold"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/func/sub-01_ses-01_task-rest_acq-VAR_bold.nii.gz' @@ -1809,6 +1850,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/func/sub-01_ses-01_task-rest_run-01_bold.nii.gz", ... {"subject": "02", "task": "rest", "acquisition": "VAR", "suffix": "bold"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/func/sub-01_ses-01_task-rest_acq-VAR_bold.nii.gz' @@ -1817,6 +1859,7 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/func/sub-01_ses-01_task-rest_run-01_bold.nii.gz", ... {"task": "rest", "acquisition": "VAR", "echo": 1, "suffix": "bold"}, ... "/output", + ... True, ... ) '/output/sub-01/ses-01/func/sub-01_ses-01_task-rest_acq-VAR_bold.nii.gz' @@ -1825,19 +1868,19 @@ def build_path(filepath, entities, out_dir): ... "/input/sub-01/ses-01/anat/sub-01_ses-01_asl.nii.gz", ... {"datatype": "perf", "acquisition": "VAR", "suffix": "asl"}, ... "/output", + ... True, ... ) WARNING: DATATYPE CHANGE DETECTED '/output/sub-01/ses-01/perf/sub-01_ses-01_acq-VAR_asl.nii.gz' - It expects a longitudinal structure, so providing a cross-sectional filename won't work. - XXX: This is a bug. + It also works for cross-sectional filename. >>> build_path( ... "/input/sub-01/func/sub-01_task-rest_run-01_bold.nii.gz", - ... {"task": "rest", "acquisition": "VAR", "echo": 1, "suffix": "bold"}, + ... {"task": "rest", "acquisition": "VAR", "suffix": "bold"}, ... "/output", + ... False, ... ) - Traceback (most recent call last): - ValueError: Could not extract subject or session from ... + '/output/sub-01/func/sub-01_task-rest_acq-VAR_bold.nii.gz' """ exts = Path(filepath).suffixes old_ext = "".join(exts) @@ -1853,9 +1896,13 @@ def build_path(filepath, entities, out_dir): entity_file_keys.append(key) sub = get_entity_value(filepath, "sub") - ses = get_entity_value(filepath, "ses") - if sub is None or ses is None: - raise ValueError(f"Could not extract subject or session from {filepath}") + if sub is None: + raise ValueError(f"Could not extract subject from {filepath}") + + if is_longitudinal: + ses = get_entity_value(filepath, "ses") + if ses is None: + raise ValueError(f"Could not extract session from {filepath}") # Add leading zeros to run entity if it's an integer. # If it's a string, respect the value provided. @@ -1874,7 +1921,10 @@ def build_path(filepath, entities, out_dir): .replace("reconstruction", "rec") ) if len(filename) > 0: - filename = f"{sub}_{ses}_{filename}_{suffix}{old_ext}" + if is_longitudinal: + filename = f"{sub}_{ses}_{filename}_{suffix}{old_ext}" + elif not is_longitudinal: + filename = f"{sub}_{filename}_{suffix}{old_ext}" else: raise ValueError(f"Could not construct new filename for {filepath}") @@ -1894,5 +1944,9 @@ def build_path(filepath, entities, out_dir): dtype_new = dtype_orig # Construct the new filename - new_path = str(Path(out_dir) / sub / ses / dtype_new / filename) + if is_longitudinal: + new_path = str(Path(out_dir) / sub / ses / dtype_new / filename) + elif not is_longitudinal: + new_path = str(Path(out_dir) / sub / dtype_new / filename) + return new_path diff --git a/cubids/metadata_merge.py b/cubids/metadata_merge.py index 6562f35b7..0779db090 100644 --- a/cubids/metadata_merge.py +++ b/cubids/metadata_merge.py @@ -243,13 +243,13 @@ def merge_json_into_json(from_file, to_file, raise_on_error=False): return 0 -def get_acq_dictionary(): +def get_acq_dictionary(is_longitudinal=False): """Create a BIDS data dictionary from dataframe columns. Parameters ---------- - df : :obj:`pandas.DataFrame` - Pre export TSV that will be converted to a json dictionary. + is_longitudinal : :obj:`bool`, optional + If True, add "session" to acq_dict. Default is False. Returns ------- @@ -258,7 +258,8 @@ def get_acq_dictionary(): """ acq_dict = {} acq_dict["subject"] = {"Description": "Participant ID"} - acq_dict["session"] = {"Description": "Session ID"} + if is_longitudinal: + acq_dict["session"] = {"Description": "Session ID"} docs = " https://cubids.readthedocs.io/en/latest/about.html#definitions" desc = "Acquisition Group. See Read the Docs for more information" acq_dict["AcqGroup"] = {"Description": desc + docs} @@ -266,7 +267,7 @@ def get_acq_dictionary(): return acq_dict -def group_by_acquisition_sets(files_tsv, output_prefix, acq_group_level): +def group_by_acquisition_sets(files_tsv, output_prefix, acq_group_level, is_longitudinal=False): """Find unique sets of Key/Param groups across subjects. This writes out the following files: @@ -284,6 +285,8 @@ def group_by_acquisition_sets(files_tsv, output_prefix, acq_group_level): Prefix for output files. acq_group_level : {"subject", "session"} Level at which to group acquisitions. + is_longitudinal : :obj:`bool`, optional + If True, add "session" to acq_dict. Default is False. """ from bids import config from bids.layout import parse_file_entities @@ -298,9 +301,12 @@ def group_by_acquisition_sets(files_tsv, output_prefix, acq_group_level): file_entities = parse_file_entities(row.FilePath) if acq_group_level == "subject": - acq_id = (file_entities.get("subject"), file_entities.get("session")) + if is_longitudinal: + acq_id = (file_entities.get("subject"), file_entities.get("session")) + elif not is_longitudinal: + acq_id = (file_entities.get("subject")) acq_groups[acq_id].append((row.EntitySet, row.ParamGroup)) - else: + elif is_longitudinal and acq_group_level == "session": acq_id = (file_entities.get("subject"), None) acq_groups[acq_id].append( (row.EntitySet, row.ParamGroup, file_entities.get("session")) @@ -326,17 +332,23 @@ def group_by_acquisition_sets(files_tsv, output_prefix, acq_group_level): for groupnum, content_id_row in enumerate(descending_order, start=1): content_id = content_ids[content_id_row] acq_group_info.append((groupnum, content_id_counts[content_id_row]) + content_id) - for subject, session in contents_to_subjects[content_id]: - grouped_sub_sess.append( - {"subject": "sub-" + subject, "session": session, "AcqGroup": groupnum} - ) + if is_longitudinal: + for subject, session in contents_to_subjects[content_id]: + grouped_sub_sess.append( + {"subject": "sub-" + subject, "session": session, "AcqGroup": groupnum} + ) + elif not is_longitudinal: + for subject in contents_to_subjects[content_id]: + grouped_sub_sess.append( + {"subject": "sub-" + subject, "AcqGroup": groupnum} + ) # Write the mapping of subject/session to acq_group_df = pd.DataFrame(grouped_sub_sess) acq_group_df.to_csv(output_prefix + "_AcqGrouping.tsv", sep="\t", index=False) # Create data dictionary for acq group tsv - acq_dict = get_acq_dictionary() + acq_dict = get_acq_dictionary(is_longitudinal) with open(output_prefix + "_AcqGrouping.json", "w") as outfile: json.dump(acq_dict, outfile, indent=4) diff --git a/cubids/tests/test_apply.py b/cubids/tests/test_apply.py index ba92b6034..ebe67818b 100644 --- a/cubids/tests/test_apply.py +++ b/cubids/tests/test_apply.py @@ -237,33 +237,35 @@ def summary_data(): @pytest.mark.parametrize( - ("name", "skeleton", "intended_for", "expected"), + ("name", "skeleton", "intended_for", "is_longitudinal", "expected"), [ - ( + ( # doesn't have acq-VAR "relpath_long", relpath_intendedfor_long, "ses-01/dwi/sub-01_ses-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", + True, "pass", ), - ( + ( # doesn't have ses-01 "bidsuri_long", bidsuri_intendedfor_long, "bids::sub-01/ses-01/dwi/sub-01_ses-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", + True, "pass", ), - ( + ( # doesn't have acq-VAR "relpath_cs", relpath_intendedfor_cs, - # XXX: CuBIDS enforces longitudinal dataset, so this fails. "dwi/sub-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", - ValueError, + False, + "pass", ), - ( + ( # pass "bidsuri_cs", bidsuri_intendedfor_cs, - # XXX: CuBIDS enforces longitudinal dataset, so this fails. "bids::sub-01/dwi/sub-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", - ValueError, + False, + "pass", ), ], ) @@ -274,6 +276,7 @@ def test_cubids_apply_intendedfor( name, skeleton, intended_for, + is_longitudinal, expected, ): """Test cubids apply with different IntendedFor types. @@ -308,7 +311,7 @@ def test_cubids_apply_intendedfor( bids_dir = tmpdir / name generate_bids_skeleton(str(bids_dir), skeleton) - if "long" in name: + if is_longitudinal: fdata = files_data["longitudinal"] fmap_json = bids_dir / "sub-01/ses-01/fmap/sub-01_ses-01_dir-AP_epi.json" else: @@ -336,6 +339,7 @@ def test_cubids_apply_intendedfor( files_tsv=files_tsv, new_tsv_prefix=None, container=None, + is_longitudinal=is_longitudinal, ) with open(fmap_json) as f: @@ -353,4 +357,5 @@ def test_cubids_apply_intendedfor( files_tsv=files_tsv, new_tsv_prefix=None, container=None, - ) + is_longitudinal=is_longitudinal, + ) \ No newline at end of file diff --git a/cubids/tests/test_bond.py b/cubids/tests/test_bond.py index a4da48a23..fdcde2eee 100644 --- a/cubids/tests/test_bond.py +++ b/cubids/tests/test_bond.py @@ -489,7 +489,7 @@ def test_tsv_merge_changes(tmp_path): The temporary path where the test data will be copied. """ data_root = get_data(tmp_path) - bod = CuBIDS(data_root / "inconsistent", use_datalad=True) + bod = CuBIDS(data_root / "inconsistent", use_datalad=True, is_longitudinal=True) bod.datalad_save() assert bod.is_datalad_clean() @@ -946,7 +946,8 @@ def test_session_apply(tmp_path): data_root = get_data(tmp_path) - ses_cubids = CuBIDS(data_root / "inconsistent", acq_group_level="session", use_datalad=True) + ses_cubids = CuBIDS(data_root / "inconsistent", acq_group_level="session", + use_datalad=True, is_longitudinal=True) ses_cubids.get_tsvs(str(tmp_path / "originals")) @@ -1193,6 +1194,7 @@ def test_bids_version(tmp_path): ), f"Schema version {schema_version} is less than minimum {min_schema_version}" +<<<<<<< HEAD def test_docker(): """Verify that docker is installed and the user has permission to run docker images. @@ -1218,7 +1220,6 @@ def test_docker(): return_status = 0 assert return_status - # def test_image(image='pennlinc/bond:latest'): # """Check whether image is present on local system.""" # ret = subprocess.run(['docker', 'images', '-q', image], diff --git a/cubids/workflows.py b/cubids/workflows.py index c09366d1e..07d03bfe9 100644 --- a/cubids/workflows.py +++ b/cubids/workflows.py @@ -432,6 +432,7 @@ def apply( files_tsv, new_tsv_prefix, container, + is_longitudinal=False, ): """Apply the tsv changes. @@ -453,6 +454,8 @@ def apply( Path to the new tsv prefix. container : :obj:`str` Container in which to run the workflow. + is_longitudinal : :obj:`bool` + If True, includes "ses" in filepath. Default is False. """ # Run directly from python using if container is None: @@ -461,6 +464,7 @@ def apply( use_datalad=use_datalad, acq_group_level=acq_group_level, grouping_config=config, + is_longitudinal=is_longitudinal, ) if use_datalad: if not bod.is_datalad_clean(): From 8f1b3c4cd966f287cd1f843d1a09bdfdbfe12fc3 Mon Sep 17 00:00:00 2001 From: Tien Tong Date: Fri, 17 Jan 2025 13:08:32 -0500 Subject: [PATCH 2/8] fix lint issues --- cubids/config.py | 1 + cubids/cubids.py | 12 ++++++------ cubids/metadata_merge.py | 6 ++---- cubids/tests/test_apply.py | 12 ++++++------ cubids/tests/test_bond.py | 34 ++++++---------------------------- 5 files changed, 21 insertions(+), 44 deletions(-) diff --git a/cubids/config.py b/cubids/config.py index a863d4eab..6c9b67887 100644 --- a/cubids/config.py +++ b/cubids/config.py @@ -4,6 +4,7 @@ import importlib.resources import yaml + def load_config(config_file): """Load a YAML file containing a configuration for param groups. diff --git a/cubids/cubids.py b/cubids/cubids.py index 3329444ff..3dd554ef5 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -113,7 +113,7 @@ def __init__( self.cubids_code_dir = Path(self.path + "/code/CuBIDS").is_dir() self.data_dict = {} # data dictionary for TSV outputs self.use_datalad = use_datalad # True if flag set, False if flag unset - self.is_longitudinal = is_longitudinal # True if flag set, False if flag unset + self.is_longitudinal = is_longitudinal # True if flag set, False if flag unset if self.use_datalad: self.init_datalad() @@ -479,7 +479,7 @@ def change_filename(self, filepath, entities, is_longitudinal=False): filepath=filepath, entities=entities, out_dir=str(self.path), - is_longitudinal=self.is_longitudinal + is_longitudinal=self.is_longitudinal, ) exts = Path(filepath).suffixes @@ -606,7 +606,7 @@ def change_filename(self, filepath, entities, is_longitudinal=False): # Coerce IntendedFor to a list. data["IntendedFor"] = listify(data["IntendedFor"]) for item in data["IntendedFor"]: - if item == _get_participant_relative_path(filepath): + if item == _get_participant_relative_path(filepath): # remove old filename data["IntendedFor"].remove(item) # add new filename @@ -1419,12 +1419,12 @@ def _get_participant_relative_path(scan): ... "/path/to/dset/sub-01/ses-01/func/sub-01_ses-01_bold.nii.gz", ... ) 'ses-01/func/sub-01_ses-01_bold.nii.gz' - + >>> _get_participant_relative_path( ... "/path/to/dset/sub-01/func/sub-01_bold.nii.gz", ... ) 'func/sub-01_bold.nii.gz' - + >>> _get_participant_relative_path( ... "/path/to/dset/ses-01/func/ses-01_bold.nii.gz", ... ) @@ -1435,7 +1435,7 @@ def _get_participant_relative_path(scan): # Find the first part that starts with "sub-" for i, part in enumerate(parts): if part.startswith("sub-"): - return "/".join(parts[i+1:]) + return "/".join(parts[i + 1 :]) raise ValueError(f"Could not find subject in {scan}") diff --git a/cubids/metadata_merge.py b/cubids/metadata_merge.py index 0779db090..3dc02bbf0 100644 --- a/cubids/metadata_merge.py +++ b/cubids/metadata_merge.py @@ -304,7 +304,7 @@ def group_by_acquisition_sets(files_tsv, output_prefix, acq_group_level, is_long if is_longitudinal: acq_id = (file_entities.get("subject"), file_entities.get("session")) elif not is_longitudinal: - acq_id = (file_entities.get("subject")) + acq_id = file_entities.get("subject") acq_groups[acq_id].append((row.EntitySet, row.ParamGroup)) elif is_longitudinal and acq_group_level == "session": acq_id = (file_entities.get("subject"), None) @@ -339,9 +339,7 @@ def group_by_acquisition_sets(files_tsv, output_prefix, acq_group_level, is_long ) elif not is_longitudinal: for subject in contents_to_subjects[content_id]: - grouped_sub_sess.append( - {"subject": "sub-" + subject, "AcqGroup": groupnum} - ) + grouped_sub_sess.append({"subject": "sub-" + subject, "AcqGroup": groupnum}) # Write the mapping of subject/session to acq_group_df = pd.DataFrame(grouped_sub_sess) diff --git a/cubids/tests/test_apply.py b/cubids/tests/test_apply.py index ebe67818b..a44ebf451 100644 --- a/cubids/tests/test_apply.py +++ b/cubids/tests/test_apply.py @@ -239,28 +239,28 @@ def summary_data(): @pytest.mark.parametrize( ("name", "skeleton", "intended_for", "is_longitudinal", "expected"), [ - ( # doesn't have acq-VAR + ( # doesn't have acq-VAR "relpath_long", relpath_intendedfor_long, "ses-01/dwi/sub-01_ses-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", True, "pass", ), - ( # doesn't have ses-01 + ( # doesn't have ses-01 "bidsuri_long", bidsuri_intendedfor_long, "bids::sub-01/ses-01/dwi/sub-01_ses-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", - True, + True, "pass", ), - ( # doesn't have acq-VAR + ( # doesn't have acq-VAR "relpath_cs", relpath_intendedfor_cs, "dwi/sub-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", False, "pass", ), - ( # pass + ( # pass "bidsuri_cs", bidsuri_intendedfor_cs, "bids::sub-01/dwi/sub-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", @@ -358,4 +358,4 @@ def test_cubids_apply_intendedfor( new_tsv_prefix=None, container=None, is_longitudinal=is_longitudinal, - ) \ No newline at end of file + ) diff --git a/cubids/tests/test_bond.py b/cubids/tests/test_bond.py index fdcde2eee..bc57593c0 100644 --- a/cubids/tests/test_bond.py +++ b/cubids/tests/test_bond.py @@ -946,8 +946,12 @@ def test_session_apply(tmp_path): data_root = get_data(tmp_path) - ses_cubids = CuBIDS(data_root / "inconsistent", acq_group_level="session", - use_datalad=True, is_longitudinal=True) + ses_cubids = CuBIDS( + data_root / "inconsistent", + acq_group_level="session", + use_datalad=True, + is_longitudinal=True, + ) ses_cubids.get_tsvs(str(tmp_path / "originals")) @@ -1194,32 +1198,6 @@ def test_bids_version(tmp_path): ), f"Schema version {schema_version} is less than minimum {min_schema_version}" -<<<<<<< HEAD -def test_docker(): - """Verify that docker is installed and the user has permission to run docker images. - - Returns - ------- - int - -1 if Docker can't be found. - 0 if Docker is found, but the user can't connect to the daemon. - 1 if the test run is OK. - """ - try: - return_status = 1 - ret = subprocess.run(["docker", "version"], stdout=subprocess.PIPE, stderr=subprocess.PIPE) - except OSError as e: - from errno import ENOENT - - if e.errno == ENOENT: - print("Cannot find Docker engine!") - return_status = 0 - raise e - if ret.stderr.startswith(b"Cannot connect to the Docker daemon."): - print("Cannot connect to Docker daemon!") - return_status = 0 - assert return_status - # def test_image(image='pennlinc/bond:latest'): # """Check whether image is present on local system.""" # ret = subprocess.run(['docker', 'images', '-q', image], From 4aacea48e7a9493aec2d7902c563dd3072d15940 Mon Sep 17 00:00:00 2001 From: Tien Tong Date: Fri, 17 Jan 2025 13:16:01 -0500 Subject: [PATCH 3/8] still fixing lint issues --- cubids/cubids.py | 5 +---- cubids/tests/test_apply.py | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/cubids/cubids.py b/cubids/cubids.py index 3dd554ef5..a985d695c 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -456,7 +456,7 @@ def apply_tsv_changes(self, summary_tsv, files_tsv, new_prefix, raise_on_error=T # remove renames file that gets created under the hood subprocess.run(["rm", "-rf", "renames"]) - def change_filename(self, filepath, entities, is_longitudinal=False): + def change_filename(self, filepath, entities): """Apply changes to a filename based on the renamed entity sets. This function takes into account the new entity set names @@ -468,8 +468,6 @@ def change_filename(self, filepath, entities, is_longitudinal=False): Path prefix to a file in the affected entity set change. entities : :obj:`dict` A pybids dictionary of entities parsed from the new entity set name. - is_longitudinal : :obj:`bool`, optional - If True, includes "ses" in filepath. Default is False. Notes ----- @@ -479,7 +477,6 @@ def change_filename(self, filepath, entities, is_longitudinal=False): filepath=filepath, entities=entities, out_dir=str(self.path), - is_longitudinal=self.is_longitudinal, ) exts = Path(filepath).suffixes diff --git a/cubids/tests/test_apply.py b/cubids/tests/test_apply.py index a44ebf451..3f0e2b5af 100644 --- a/cubids/tests/test_apply.py +++ b/cubids/tests/test_apply.py @@ -239,28 +239,28 @@ def summary_data(): @pytest.mark.parametrize( ("name", "skeleton", "intended_for", "is_longitudinal", "expected"), [ - ( # doesn't have acq-VAR + ( "relpath_long", relpath_intendedfor_long, "ses-01/dwi/sub-01_ses-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", True, "pass", ), - ( # doesn't have ses-01 + ( "bidsuri_long", bidsuri_intendedfor_long, "bids::sub-01/ses-01/dwi/sub-01_ses-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", True, "pass", ), - ( # doesn't have acq-VAR + ( "relpath_cs", relpath_intendedfor_cs, "dwi/sub-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", False, "pass", ), - ( # pass + ( "bidsuri_cs", bidsuri_intendedfor_cs, "bids::sub-01/dwi/sub-01_acq-VAR_dir-AP_run-01_dwi.nii.gz", From fe68deaee83d1eed809e387683b71f1d5d35ba80 Mon Sep 17 00:00:00 2001 From: Taylor Salo Date: Fri, 17 Jan 2025 13:37:35 -0500 Subject: [PATCH 4/8] Update cubids.py --- cubids/cubids.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cubids/cubids.py b/cubids/cubids.py index a985d695c..d6ca6818d 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -477,6 +477,7 @@ def change_filename(self, filepath, entities): filepath=filepath, entities=entities, out_dir=str(self.path), + is_longitudinal=self.is_longitudinal, ) exts = Path(filepath).suffixes @@ -608,6 +609,7 @@ def change_filename(self, filepath, entities): data["IntendedFor"].remove(item) # add new filename data["IntendedFor"].append(_get_participant_relative_path(new_path)) + if item == _get_bidsuri(filepath, self.path): # remove old filename data["IntendedFor"].remove(item) From 1dcde70d5c3e1fa0e406bdebe849a085f6d97421 Mon Sep 17 00:00:00 2001 From: Tien Tong Date: Fri, 17 Jan 2025 14:39:47 -0500 Subject: [PATCH 5/8] unset default value for is_longitudinal and add a method to infer is_longitudinal from data structure --- cubids/cubids.py | 18 ++++++++++++------ cubids/tests/test_apply.py | 4 ++-- cubids/tests/test_bond.py | 3 +-- cubids/workflows.py | 4 ---- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/cubids/cubids.py b/cubids/cubids.py index d6ca6818d..f958f04b3 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -85,7 +85,7 @@ class CuBIDS(object): use_datalad : :obj:`bool` If True, use datalad to track changes to the BIDS dataset. is_longitudinal : :obj:`bool` - If True, includes "ses" in filepath. Default is False. + If True, adds "ses" in filepath. """ def __init__( @@ -95,7 +95,6 @@ def __init__( acq_group_level="subject", grouping_config=None, force_unlock=False, - is_longitudinal=False, ): self.path = os.path.abspath(data_root) self._layout = None @@ -113,12 +112,15 @@ def __init__( self.cubids_code_dir = Path(self.path + "/code/CuBIDS").is_dir() self.data_dict = {} # data dictionary for TSV outputs self.use_datalad = use_datalad # True if flag set, False if flag unset - self.is_longitudinal = is_longitudinal # True if flag set, False if flag unset + self.is_longitudinal = self._infer_longitudinal() # inferred from dataset structure + if self.use_datalad: self.init_datalad() if self.is_longitudinal and self.acq_group_level == "session": NON_KEY_ENTITIES.remove("session") + elif not self.is_longitudinal and self.acq_group_level == "session": + raise ValueError('Data is not longitudinal, so "session" is not a valid grouping level.') @property def layout(self): @@ -132,6 +134,10 @@ def layout(self): # print("LAYOUT OBJECT SET") return self._layout + def _infer_longitudinal(self): + """Infer if the dataset is longitudinal based on its structure.""" + return any("ses-" in str(f) for f in Path(self.path).rglob("*")) + def reset_bids_layout(self, validate=False): """Reset the BIDS layout. @@ -1766,7 +1772,7 @@ def get_entity_value(path, key): return part -def build_path(filepath, entities, out_dir, is_longitudinal=False): +def build_path(filepath, entities, out_dir, is_longitudinal): """Build a new path for a file based on its BIDS entities. Parameters @@ -1778,8 +1784,8 @@ def build_path(filepath, entities, out_dir, is_longitudinal=False): This should include all of the entities in the filename *except* for subject and session. out_dir : str The output directory for the new file. - is_longitudinal : bool, optional - If True, add "ses" to file path. Default is False. + is_longitudinal : bool + If True, add "ses" to file path. Returns ------- diff --git a/cubids/tests/test_apply.py b/cubids/tests/test_apply.py index 3f0e2b5af..51afa64f0 100644 --- a/cubids/tests/test_apply.py +++ b/cubids/tests/test_apply.py @@ -295,6 +295,8 @@ def test_cubids_apply_intendedfor( BIDS skeleton structure. intended_for : str IntendedFor field value. + is_longitudinal : bool + Indicate whether the data structure is longitudinal or cross-sectional. expected : str or Exception Expected result or exception. @@ -339,7 +341,6 @@ def test_cubids_apply_intendedfor( files_tsv=files_tsv, new_tsv_prefix=None, container=None, - is_longitudinal=is_longitudinal, ) with open(fmap_json) as f: @@ -357,5 +358,4 @@ def test_cubids_apply_intendedfor( files_tsv=files_tsv, new_tsv_prefix=None, container=None, - is_longitudinal=is_longitudinal, ) diff --git a/cubids/tests/test_bond.py b/cubids/tests/test_bond.py index bc57593c0..9f7f5b399 100644 --- a/cubids/tests/test_bond.py +++ b/cubids/tests/test_bond.py @@ -489,7 +489,7 @@ def test_tsv_merge_changes(tmp_path): The temporary path where the test data will be copied. """ data_root = get_data(tmp_path) - bod = CuBIDS(data_root / "inconsistent", use_datalad=True, is_longitudinal=True) + bod = CuBIDS(data_root / "inconsistent", use_datalad=True) bod.datalad_save() assert bod.is_datalad_clean() @@ -950,7 +950,6 @@ def test_session_apply(tmp_path): data_root / "inconsistent", acq_group_level="session", use_datalad=True, - is_longitudinal=True, ) ses_cubids.get_tsvs(str(tmp_path / "originals")) diff --git a/cubids/workflows.py b/cubids/workflows.py index 07d03bfe9..c09366d1e 100644 --- a/cubids/workflows.py +++ b/cubids/workflows.py @@ -432,7 +432,6 @@ def apply( files_tsv, new_tsv_prefix, container, - is_longitudinal=False, ): """Apply the tsv changes. @@ -454,8 +453,6 @@ def apply( Path to the new tsv prefix. container : :obj:`str` Container in which to run the workflow. - is_longitudinal : :obj:`bool` - If True, includes "ses" in filepath. Default is False. """ # Run directly from python using if container is None: @@ -464,7 +461,6 @@ def apply( use_datalad=use_datalad, acq_group_level=acq_group_level, grouping_config=config, - is_longitudinal=is_longitudinal, ) if use_datalad: if not bod.is_datalad_clean(): From 9db0657697736095f606e8daa96d8d461652673a Mon Sep 17 00:00:00 2001 From: Tien Tong Date: Fri, 17 Jan 2025 14:43:10 -0500 Subject: [PATCH 6/8] fix lint issues --- cubids/cubids.py | 4 +++- docs/conf.py | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/cubids/cubids.py b/cubids/cubids.py index f958f04b3..b666656ac 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -120,7 +120,9 @@ def __init__( if self.is_longitudinal and self.acq_group_level == "session": NON_KEY_ENTITIES.remove("session") elif not self.is_longitudinal and self.acq_group_level == "session": - raise ValueError('Data is not longitudinal, so "session" is not a valid grouping level.') + raise ValueError( + 'Data is not longitudinal, so "session" is not a valid grouping level.' + ) @property def layout(self): diff --git a/docs/conf.py b/docs/conf.py index 5512a64d6..9da1b27fb 100755 --- a/docs/conf.py +++ b/docs/conf.py @@ -56,7 +56,7 @@ "sphinx_gallery.load_style", "sphinxarg.ext", # argparse extension "sphinxcontrib.bibtex", # bibtex-based bibliographies - "sphinx_design", # for adding in-line badges etc + "sphinx_design", # for adding in-line badges etc ] # Mock modules in autodoc: @@ -266,4 +266,6 @@ # ----------------------------------------------------------------------------- # Configuration for sphinx_copybutton to remove shell prompts, i.e. $ copybutton_prompt_text = "$ " -copybutton_only_copy_prompt_lines = False # ensures all lines are copied, even those without a prompt \ No newline at end of file +copybutton_only_copy_prompt_lines = ( + False # ensures all lines are copied, even those without a prompt +) From ad265ef8c585840fb7bda6bbf079fdc504fee181 Mon Sep 17 00:00:00 2001 From: Tien Tong Date: Fri, 17 Jan 2025 14:56:42 -0500 Subject: [PATCH 7/8] remove is_longitudinal from CuBIDS class docstring as Taylor suggested --- cubids/cubids.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/cubids/cubids.py b/cubids/cubids.py index b666656ac..8cbb4dc49 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -84,8 +84,6 @@ class CuBIDS(object): A data dictionary for TSV outputs. use_datalad : :obj:`bool` If True, use datalad to track changes to the BIDS dataset. - is_longitudinal : :obj:`bool` - If True, adds "ses" in filepath. """ def __init__( From 66a79acacedf512f43036186414422025c8efe4e Mon Sep 17 00:00:00 2001 From: Tien Tong Date: Fri, 17 Jan 2025 15:03:43 -0500 Subject: [PATCH 8/8] add is_longitudinal as an attribute in docstring --- cubids/cubids.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cubids/cubids.py b/cubids/cubids.py index 8cbb4dc49..b666656ac 100644 --- a/cubids/cubids.py +++ b/cubids/cubids.py @@ -84,6 +84,8 @@ class CuBIDS(object): A data dictionary for TSV outputs. use_datalad : :obj:`bool` If True, use datalad to track changes to the BIDS dataset. + is_longitudinal : :obj:`bool` + If True, adds "ses" in filepath. """ def __init__(