diff --git a/datamint/dataset/base.py b/datamint/dataset/base.py index d03cda0b..e789839d 100644 --- a/datamint/dataset/base.py +++ b/datamint/dataset/base.py @@ -501,7 +501,7 @@ def _annotation_spec_payload(annotation_spec: AnnotationSpec) -> dict[str, Any]: if isinstance(annotation_spec, CategoryAnnotationSpec): payload['values'] = sorted(annotation_spec.values) return payload - + def _raise_ambiguous_worklist_schema(self, summary: str, details: Sequence[str]) -> None: for detail in details: _LOGGER.error(f"{summary}: {detail}") @@ -1159,6 +1159,8 @@ def split( use_server_splits: bool | None = None, use_project_splits: bool | None = None, as_of_timestamp: str | None = None, + by_patient: bool = False, + none_patient_id_strategy: Literal['individual', 'group', 'skip', 'error'] = 'individual', **splits: float, ) -> dict[str, 'DatamintBaseDataset']: """Split the dataset into multiple named subsets. @@ -1176,7 +1178,10 @@ def split( # Local split parts = dataset.split(train=0.7, val=0.15, test=0.15, seed=42) - train_ds = parts['train'] + train_ds = parts['train'] + + # Patient-wise split + parts = dataset.split(train = 0.8, test = 0.2, by_patient=True, seed=42) # Project-scoped split — inferred for project-backed datasets parts = dataset.split() @@ -1186,6 +1191,12 @@ def split( Args: seed: Random seed for reproducible local splitting. + by_patient: If ``True``, shuffle and assign whole patients to + splits rather than individual resources, preventing cross-patient + data leakage. Requires ratio kwards; mutually exclusive with + ``use_project_splits`` and ``use_server_splits``. + none_patient_id_strategy: Strategy for handling resources without patient IDs + when ``by_patient=True``. See :meth:`group_by_patient` for details. use_project_splits: If ``True``, read split assignments from the project splits API. If ``None`` (default), project-backed datasets prefer this mode when no ratios are provided. @@ -1204,6 +1215,18 @@ def split( Raises: ValueError: If ratios are invalid or arguments conflict. """ + if by_patient: + if use_project_splits or use_server_splits: + raise ValueError( + "by_patient=True cannot be combined with use_project_splits or use_server_splits." + ) + + if not splits: + raise ValueError( + "by_patient=True requires ratio kwargs (e.g. train=0.8, test=0.2) to determine split sizes." + ) + return self._split_locally_by_patient(dict(splits), seed, none_patient_id_strategy) + if use_project_splits is None and use_server_splits is None and not splits: use_project_splits = getattr(self, 'project', None) is not None or as_of_timestamp is not None @@ -1349,6 +1372,112 @@ def _split_locally( return result + def _group_resources_indices_by_patient( + self, + none_patient_id_strategy: Literal['individual', 'group', 'skip', 'error'], + ) -> 'dict[str | None, list[int]]': + from collections import defaultdict + + patient_indices: dict[str | None, list[int]] = defaultdict(list) + + for idx, resource in enumerate(self.resources): + pid = resource.get_patient_id() + + if pid is None: + if none_patient_id_strategy == 'error': + raise ValueError(( + f"Resource at index {idx} (id={getattr(resource, 'id', '?')!r}) has no patient_id." + "Set none_patient_id_strategy='individual' to treat each as its own patient, " + "'group' to group all together, or 'skip' to exclude them." + )) + elif none_patient_id_strategy == 'skip': + continue + elif none_patient_id_strategy == 'individual': + pid = f'__no_patient_{getattr(resource, "id", idx)}__' + + patient_indices[pid].append(idx) + + return dict(patient_indices) + + def group_by_patient( + self, + none_patient_id_strategy: Literal['individual', 'group', 'skip', 'error'] = 'individual', + ) -> 'dict[str | None, DatamintBaseDataset]': + """ Group dataset resources by patient ID. + Returns one-subdataset per unique patient. Useful for patient-level operations such as leave-one-patient-out cross-validation. + + Args: + none_patient_id_strategy: How to handle resources with no patient_id. + - 'individual': Treat each resource with no patient_id as its own unique patient (default). + - 'group': Group all resources with no patient_id into a single "None" patient group. + - 'skip': Exclude resources with no patient_id from the result. + - 'error': Raise an error if any resource has no patient_id. + + Returns: + Dict mapping patient_id (or None) to a DatamintBaseDataset containing only resources for that patient. + + """ + + _valid_strategies = 'individual', 'group', 'skip', 'error' + + if none_patient_id_strategy not in _valid_strategies: + raise ValueError( + f"none_patient_id_strategy must be one of {_valid_strategies}, got {none_patient_id_strategy!r}" + ) + + patient_indices = self._group_resources_indices_by_patient(none_patient_id_strategy) + + return {pid: self.subset(indices) for pid, indices in patient_indices.items()} + + def _split_locally_by_patient( + self, + splits: dict[str, float], + seed: int | None, + none_patient_id_strategy: Literal['individual', 'group', 'skip', 'error'], + ) -> 'dict[str, DatamintBaseDataset]': + """Split dataset by patient groups, ensuring all resources from the same patient are in the same split.""" + + if len(splits) < 2: + raise ValueError("At least 2 splits are required (e.g. train=0.7, test=0.3).") + + for name, ratio in splits.items(): + if ratio <= 0: + raise ValueError (f"Split ratio for '{name}' must be positive, got {ratio}.") + + total = sum(splits.values()) + if abs(total - 1.0) > 0.01: + raise ValueError( + f"Split ratios must sum to 1.0 (got {total:.4f}). Provided: {splits} " + ) + + patient_indices = self._group_resources_indices_by_patient(none_patient_id_strategy) + patients_ids = list(patient_indices.keys()) + + import random + rng = random.Random(seed) + rng.shuffle(patients_ids) + + n = len(patients_ids) + split_items = list(splits.items()) + split_resource_indices: dict[str, list[int]] = {name: [] for name in splits} + + start = 0 + for i, (name, ratio) in enumerate(split_items): + end = n if i == len(split_items) - 1 else start + round(ratio * n) + for pid in patients_ids[start:end]: + split_resource_indices[name].extend(patient_indices[pid]) + start = end + + result = {name: self.subset(indices) for name, indices in split_resource_indices.items()} + + for name, ds in result.items(): + ds.split_name = name + ds.split_source = 'local_by_patient' + ds.split_as_of_timestamp = None + + return result + + def filter( self, *, diff --git a/datamint/entities/resource.py b/datamint/entities/resource.py index 82ca7f15..1e55215a 100644 --- a/datamint/entities/resource.py +++ b/datamint/entities/resource.py @@ -484,7 +484,12 @@ def is_cached(self) -> bool: version_info = self._generate_version_info() cached_data = self._cache.get(self.id, _IMAGE_CACHEKEY, version_info) return cached_data is not None - + + def get_patient_id(self) -> str | None: + if self.patient_id is not None: + return self.patient_id + return self.metadata.get('PatientID') if isinstance(self.metadata, dict) else None + @property def filepath_cached(self) -> Path | None: """Get the file path of the cached resource data, if available. diff --git a/notebooks/patient_wise_split.ipynb b/notebooks/patient_wise_split.ipynb new file mode 100644 index 00000000..0475a25a --- /dev/null +++ b/notebooks/patient_wise_split.ipynb @@ -0,0 +1,353 @@ +{ + "cells": [ + { + "cell_type": "markdown", + "id": "a1b2c3d4", + "metadata": {}, + "source": [ + "# Patient-wise Dataset Splitting\n", + "\n", + "This notebook shows how to use `group_by_patient()` and `split(by_patient=True)` on a DICOM dataset.\n", + "\n", + "## The Problem: Data Leakage\n", + "\n", + "Standard random splitting shuffles individual resources. If a patient has **multiple scans** (e.g. a baseline CT and a follow-up CT), both scans can end up in different splits (one in `train`, one in `test`). Because the images share patient anatomy, the model effectively sees the test patient during training, inflating metrics.\n", + "\n", + "**Patient-wise splitting** fixes this by shuffling *patients* instead of *resources*: every scan belonging to a patient lands in the same split.\n", + "\n", + "```\n", + "Resource-wise (risky) Patient-wise (safe)\n", + "────────────────────── ──────────────────────\n", + "train: scan_A1, scan_B1 train: patient_A (scan_A1, scan_A2)\n", + "test: scan_A2, scan_B2 test: patient_B (scan_B1, scan_B2)\n", + " ↑ same patient! ↑ clean boundary\n", + "```\n", + "\n", + "## What You'll Learn\n", + "\n", + "1. How `patient_id` is exposed on resources\n", + "2. `group_by_patient()` — organise a dataset by patient for inspection or custom splits\n", + "3. `split(by_patient=True)` — an option for the split\n", + "4. How to persist patient-wise splits to the server and reload them reproducibly\n", + "5. How to handle resources without a `patient_id`" + ] + }, + { + "cell_type": "markdown", + "id": "b2c3d4e5", + "metadata": {}, + "source": [ + "## 1. Setup\n", + "\n", + "We use a small set of DICOM files bundled with **pydicom** as example data. Each file already carries a `PatientID` tag in its header.\n", + "\n", + "The upload runs only once: if the project already exists we skip straight to loading the dataset." + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "c3d4e5f6", + "metadata": {}, + "outputs": [], + "source": [ + "import pydicom\n", + "import pydicom.data\n", + "from pathlib import Path\n", + "\n", + "from datamint import Api\n", + "from datamint.dataset import ImageDataset\n", + "\n", + "PROJECT_NAME = \"patient_wise_split_demo\"\n", + "\n", + "# 8 patients selected from pydicom's \n", + "SELECTED_PATIENTS = {\n", + " '4MR1', '8NM1', '13US1', 'CQ500-CT-310',\n", + " '1CT1', '021234567', '11-05-25-142825', '204',\n", + "}\n", + "\n", + "def _collect_dicom_files() -> list[str]:\n", + " data_root = Path(pydicom.data.DATA_ROOT) / 'test_files'\n", + " selected = []\n", + " for f in sorted(data_root.glob('*.dcm')):\n", + " try:\n", + " ds = pydicom.dcmread(str(f), stop_before_pixels=True, force=True)\n", + " if getattr(ds, 'PatientID', None) in SELECTED_PATIENTS:\n", + " selected.append(str(f))\n", + " except Exception:\n", + " pass\n", + " return selected\n", + "\n", + "api = Api()\n", + "\n", + "project = api.projects.get_by_name(PROJECT_NAME)\n", + "if project is None:\n", + " project = api.projects.create(PROJECT_NAME, description=\"Patient Split Demo\")\n", + " dicom_files = _collect_dicom_files()\n", + " print(f\"Uploading {len(dicom_files)} DICOM files for {len(SELECTED_PATIENTS)} patients...\")\n", + " api.resources.upload_resources(\n", + " dicom_files,\n", + " publish_to=project,\n", + " assemble_dicoms=False,\n", + " progress_bar=True,\n", + " )\n", + " print(\"Upload complete.\")\n", + "else:\n", + " print(f\"Project '{PROJECT_NAME}' already exists, skipping upload.\")\n", + "\n", + "dataset = ImageDataset(project=project, include_unannotated=True)\n", + "print(f\"\\nLoaded {len(dataset)} resources\")" + ] + }, + { + "cell_type": "markdown", + "id": "d4e5f6a7", + "metadata": {}, + "source": [ + "## 2. Inspecting `patient_id`\n", + "\n", + "For DICOM files the platform extracts `PatientID` from the DICOM header automatically at upload time and stores it as a top-level field on the resource. No manual assignment needed." + ] + }, + { + "cell_type": "code", + "execution_count": 3, + "id": "e5f6a7b8", + "metadata": {}, + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "MR_small_RLE.dcm patient_id='4MR1'\n", + "JPEG2000.dcm patient_id='8NM1'\n", + "JPEG2000-embedded-sequence-delimiter.dcm patient_id='8NM1'\n", + "MR_truncated.dcm patient_id='4MR1'\n", + "MR_small_expb.dcm patient_id='4MR1'\n" + ] + } + ], + "source": [ + "for r in dataset.resources[:5]:\n", + " print(f\"{r.filename:40s} patient_id={r.patient_id!r}\")" + ] + }, + { + "cell_type": "markdown", + "id": "a7b8c9d0", + "metadata": {}, + "source": [ + "## 3. `group_by_patient()`\n", + "\n", + "`group_by_patient()` returns a `dict` mapping each patient ID to a sub-dataset containing only that patient's resources.\n", + "\n", + "### Handling `patient_id=None`\n", + "\n", + "Well-formed DICOM files always carry a `PatientID` tag, so all resources here should have one. In edge cases, anonymised DICOMs where the tag was stripped, `patient_id` will be `None`. The `none_patient_id_strategy` parameter controls what to do:\n", + "\n", + "| Strategy | Behaviour |\n", + "|---|---|\n", + "| `'individual'` (default) | Each `None`-patient resource becomes its own group |\n", + "| `'group'` | All grouped together under key `None` |\n", + "| `'skip'` | Excluded from the output entirely |\n", + "| `'error'` | Raises `ValueError` immediately |" + ] + }, + { + "cell_type": "code", + "execution_count": 6, + "id": "b8c9d0e1", + "metadata": {}, + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "Patient groups : 8\n", + "Total resources: 20\n", + "Per-group size : min=1, max=9\n", + "\n", + " 4MR1 9 resource(s)\n", + " 8NM1 4 resource(s)\n", + " 1CT1 1 resource(s)\n", + " 13US1 2 resource(s)\n", + " CQ500-CT-310 1 resource(s)\n", + " 204 1 resource(s)\n", + " 11-05-25-142825 1 resource(s)\n", + " 021234567 1 resource(s)\n" + ] + } + ], + "source": [ + "groups = dataset.group_by_patient()\n", + "\n", + "sizes = [len(g) for g in groups.values()]\n", + "print(f\"Patient groups : {len(groups)}\")\n", + "print(f\"Total resources: {sum(sizes)}\")\n", + "print(f\"Per-group size : min={min(sizes)}, max={max(sizes)}\")\n", + "print()\n", + "for pid, g in groups.items():\n", + " print(f\" {pid:20s} {len(g):2d} resource(s)\")" + ] + }, + { + "cell_type": "markdown", + "id": "c9d0e1f2", + "metadata": {}, + "source": [ + "## 4. `split(by_patient=True)` — Compute Patient-wise Splits Locally\n", + "\n", + "When passing by_patient=True, the split shuffles patients instead of individual resources. Internally it shuffles **patients**, assigns patient buckets to splits by ratio, then collects all resources from each bucket." + ] + }, + { + "cell_type": "code", + "execution_count": 7, + "id": "d0e1f2a3", + "metadata": {}, + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "train : 7 resources\n", + "val : 9 resources\n", + "test : 4 resources\n" + ] + } + ], + "source": [ + "parts = dataset.split(\n", + " train=0.7,\n", + " val=0.15,\n", + " test=0.15,\n", + " by_patient=True,\n", + " seed=42,\n", + ")\n", + "\n", + "for name, ds in parts.items():\n", + " print(f\"{name:6s}: {len(ds):4d} resources\")" + ] + }, + { + "cell_type": "markdown", + "id": "f2a3b4c5", + "metadata": {}, + "source": [ + "## 5. Persist Splits to the Server\n", + "\n", + "The `parts` dict computed above lives only in memory. To make the split **reproducible across sessions and shareable with the team**, write the assignments back to the project using `api.projects.assign_splits()`.\n", + "\n", + "Once persisted, anyone can reload exactly the same split (without re-running the patient split logic) by calling `dataset.split()` with no arguments (project-backed datasets prefer the server assignments automatically)." + ] + }, + { + "cell_type": "code", + "execution_count": 8, + "id": "a3b4c5d6", + "metadata": {}, + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "Assigned 7 resources → 'train'\n", + "Assigned 9 resources → 'val'\n", + "Assigned 4 resources → 'test'\n", + "\n", + "Patient-wise split assignments saved to the project.\n" + ] + } + ], + "source": [ + "for split_name, split_ds in parts.items():\n", + " api.projects.assign_splits(project, split_ds.resources, split_name=split_name)\n", + " print(f\"Assigned {len(split_ds):4d} resources → '{split_name}'\")\n", + "\n", + "print(\"\\nPatient-wise split assignments saved to the project.\")" + ] + }, + { + "cell_type": "markdown", + "id": "b4c5d6e7", + "metadata": {}, + "source": [ + "## 6. Reload Splits from the Server\n", + "\n", + "After persisting, the splits are loaded back just like any project-scoped split, no `by_patient`, no ratios. The `split_as_of_timestamp` returned on each sub-dataset can be stored and replayed later to recover the exact same snapshot." + ] + }, + { + "cell_type": "code", + "execution_count": 11, + "id": "c5d6e7f8", + "metadata": {}, + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "val : 9 resources\n", + "test : 4 resources\n", + "train : 7 resources\n" + ] + } + ], + "source": [ + "# Reload the dataset fresh (simulates a new session)\n", + "dataset_fresh = ImageDataset(project=project, include_unannotated=True)\n", + "\n", + "# No by_patient, no ratios, reads from project API\n", + "reloaded = dataset_fresh.split()\n", + "\n", + "for name, ds in reloaded.items():\n", + " print(f\"{name:6s}: {len(ds):4d} resources\")" + ] + }, + { + "cell_type": "code", + "execution_count": 10, + "id": "d6e7f8a9", + "metadata": {}, + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "Replaying split snapshot from 2026-06-16T12:49:59.199793Z\n", + "{'val': 9, 'test': 4, 'train': 7}\n" + ] + } + ], + "source": [ + "# Replay the exact same snapshot at any later point\n", + "snapshot_ts = reloaded['train'].split_as_of_timestamp\n", + "\n", + "replayed = dataset_fresh.split(as_of_timestamp=snapshot_ts)\n", + "print(f\"Replaying split snapshot from {snapshot_ts}\")\n", + "print({name: len(ds) for name, ds in replayed.items()})" + ] + } + ], + "metadata": { + "kernelspec": { + "display_name": "env", + "language": "python", + "name": "python3" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.12.3" + } + }, + "nbformat": 4, + "nbformat_minor": 5 +} diff --git a/tests/test_dataset_patient_split.py b/tests/test_dataset_patient_split.py new file mode 100644 index 00000000..9d399ae4 --- /dev/null +++ b/tests/test_dataset_patient_split.py @@ -0,0 +1,261 @@ +"""Tests for patient-wise dataset splitting: group_by_patient() and split(by_patient=True).""" +import pytest + +from datamint.dataset.base import DatamintBaseDataset + + +# --------------------------------------------------------------------------- +# Test setup +# --------------------------------------------------------------------------- + +class _TestDataset(DatamintBaseDataset): + """Minimal concrete subclass that bypasses API calls entirely.""" + + def _reinit_api(self) -> None: + pass # no-op: prevents copy.copy → __setstate__ from touching the network + + def _get_raw_item(self, index: int) -> dict: + return {'image': None, 'metainfo': {}, 'annotations': []} + + def apply_alb_transform(self, img, segmentations): + return {'image': img, 'segmentations': segmentations} + + +class _MockResource: + def __init__(self, resource_id: str, patient_id: str | None, metadata: dict | None = None): + self.id = resource_id + self.patient_id = patient_id + self.metadata = metadata or {} + + def get_patient_id(self) -> str | None: + if self.patient_id is not None: + return self.patient_id + return self.metadata.get('PatientID') + + +def _make_resource(resource_id: str, patient_id: str | None, metadata: dict | None = None): + return _MockResource(resource_id, patient_id, metadata) + + +def _make_dataset(patient_ids: list[str | None]) -> _TestDataset: + """Create a prepared dataset with one resource per entry in patient_ids.""" + ds = object.__new__(_TestDataset) + resources = [_make_resource(f'res-{i}', pid) for i, pid in enumerate(patient_ids)] + ds.__dict__.update({ + 'resources': resources, + 'resource_annotations': [[] for _ in resources], + 'project': None, + 'split_name': None, + 'split_source': None, + 'split_as_of_timestamp': None, + '_is_prepared': True, + '_DatamintBaseDataset__api': None, + '_server_url': None, + '_api_key': None, + '_auto_update': False, + '_logged_uint16_conversion': False, + }) + return ds + + +# --------------------------------------------------------------------------- +# group_by_patient() +# --------------------------------------------------------------------------- + +class TestGroupByPatient: + def test_basic_grouping(self): + "Test that resources are grouped by patient_id, and that the correct number of resources end up in each group. " + # pat-A has 2 resources, pat-B has 3, pat-C has 1 + ds = _make_dataset(['pat-A', 'pat-A', 'pat-B', 'pat-B', 'pat-B', 'pat-C']) + groups = ds.group_by_patient() + + assert set(groups.keys()) == {'pat-A', 'pat-B', 'pat-C'} + assert len(groups['pat-A']) == 2 + assert len(groups['pat-B']) == 3 + assert len(groups['pat-C']) == 1 + + def test_resources_correctly_assigned(self): + "Test that the correct resources end up in each patient group, based on their patient_id." + ds = _make_dataset(['pat-A', 'pat-B', 'pat-A']) + groups = ds.group_by_patient() + + a_ids = {r.id for r in groups['pat-A'].resources} + b_ids = {r.id for r in groups['pat-B'].resources} + assert a_ids == {'res-0', 'res-2'} + assert b_ids == {'res-1'} + + def test_total_resources_preserved(self): + "Test that all resources in the original dataset are accounted for in the patient groups, with no duplicates or omissions." + patient_ids = ['A', 'A', 'B', 'C', 'C', 'C'] + ds = _make_dataset(patient_ids) + groups = ds.group_by_patient() + + total = sum(len(g) for g in groups.values()) + assert total == len(ds) + + def test_metadata_fallback(self): + "Test that if a resource has no patient_id, the method falls back to looking for a PatientID in the metadata. " + resources = [ + _make_resource('res-0', None, metadata={'PatientID': 'pat-meta'}), + _make_resource('res-1', 'pat-direct', metadata={}), + ] + ds = object.__new__(_TestDataset) + ds.__dict__.update({ + 'resources': resources, + 'resource_annotations': [[], []], + 'project': None, 'split_name': None, 'split_source': None, + 'split_as_of_timestamp': None, '_is_prepared': True, + '_DatamintBaseDataset__api': None, '_server_url': None, + '_api_key': None, '_auto_update': False, '_logged_uint16_conversion': False, + }) + groups = ds.group_by_patient() + + assert 'pat-meta' in groups + assert 'pat-direct' in groups + assert len(groups) == 2 + + def test_none_strategy_individual(self): + "Test that when none_patient_id_strategy='individual', resources with None patient_id are grouped separately, and do not interfere with real patient groups." + ds = _make_dataset([None, None, 'pat-A']) + groups = ds.group_by_patient(none_patient_id_strategy='individual') + + # 2 individual None groups + 1 real patient + assert len(groups) == 3 + assert 'pat-A' in groups + assert None not in groups + + def test_none_strategy_group(self): + "Test that when none_patient_id_strategy='group', all resources with None patient_id are grouped together under a single None key, and do not interfere with real patient groups." + ds = _make_dataset([None, None, 'pat-A']) + groups = ds.group_by_patient(none_patient_id_strategy='group') + + assert None in groups + assert len(groups[None]) == 2 + assert 'pat-A' in groups + + def test_none_strategy_skip(self): + "Test that when none_patient_id_strategy='skip', resources with None patient_id are excluded from the groups, and do not interfere with real patient groups." + ds = _make_dataset([None, 'pat-A', None]) + groups = ds.group_by_patient(none_patient_id_strategy='skip') + + assert None not in groups + assert set(groups.keys()) == {'pat-A'} + assert len(groups['pat-A']) == 1 + + def test_none_strategy_error(self): + "Test that when none_patient_id_strategy='error', if any resource has a None patient_id, a ValueError is raised indicating that no patient_id was found." + ds = _make_dataset(['pat-A', None]) + with pytest.raises(ValueError, match='no patient_id'): + ds.group_by_patient(none_patient_id_strategy='error') + + def test_invalid_strategy_raises(self): + "Test that if an invalid value is passed for none_patient_id_strategy, a ValueError is raised indicating that the strategy must be one of the allowed options." + ds = _make_dataset(['pat-A']) + with pytest.raises(ValueError, match='must be one of'): + ds.group_by_patient(none_patient_id_strategy='invalid') # type: ignore[arg-type] + + def test_all_same_patient(self): + "Test that if all resources have the same patient_id, they are all grouped together under that patient_id key." + ds = _make_dataset(['pat-A', 'pat-A', 'pat-A']) + groups = ds.group_by_patient() + assert list(groups.keys()) == ['pat-A'] + assert len(groups['pat-A']) == 3 + + +# --------------------------------------------------------------------------- +# split(by_patient=True) +# --------------------------------------------------------------------------- + +class TestSplitByPatient: + def test_no_patient_leakage(self): + "Test that when splitting by patient, no patient appears in more than one split, even if they have multiple resources. " + patient_ids = [pid for pid in 'ABCDEF' for _ in range(2)] + ds = _make_dataset(patient_ids) + parts = ds.split(train=0.7, test=0.3, by_patient=True, seed=42) + + train_pids = {r.get_patient_id() for r in parts['train'].resources} + test_pids = {r.get_patient_id() for r in parts['test'].resources} + assert train_pids & test_pids == set() + + def test_all_resources_accounted_for(self): + "Test that when splitting by patient, all resources from the original dataset are included in one of the splits, with no duplicates or omissions, even if multiple resources belong to the same patient." + ds = _make_dataset(['A', 'A', 'B', 'B', 'B', 'C']) + parts = ds.split(train=0.7, test=0.3, by_patient=True, seed=0) + + total = sum(len(p) for p in parts.values()) + assert total == len(ds) + + def test_split_metadata_set(self): + "Test that when splitting by patient, the resulting split datasets have their split_name and split_source attributes set correctly to indicate the type of split performed." + ds = _make_dataset(['A', 'B', 'C', 'D']) + parts = ds.split(train=0.5, test=0.5, by_patient=True, seed=0) + + assert parts['train'].split_name == 'train' + assert parts['test'].split_name == 'test' + assert parts['train'].split_source == 'local_by_patient' + assert parts['test'].split_source == 'local_by_patient' + + def test_seed_reproducible(self): + "Test that when splitting by patient with a specific random seed, the same patients are assigned to the same splits across multiple runs, even if patients have multiple resources. " + ds = _make_dataset(['A', 'A', 'B', 'B', 'C', 'C']) + parts1 = ds.split(train=0.7, test=0.3, by_patient=True, seed=7) + parts2 = ds.split(train=0.7, test=0.3, by_patient=True, seed=7) + + ids1 = [r.id for r in parts1['train'].resources] + ids2 = [r.id for r in parts2['train'].resources] + assert ids1 == ids2 + + def test_different_seeds_produce_different_splits(self): + "Test that when splitting by patient with different random seeds, different patients are assigned to the splits across runs, even if patients have multiple resources. " + ds = _make_dataset([str(i) for i in range(10)]) + parts1 = ds.split(train=0.5, test=0.5, by_patient=True, seed=1) + parts2 = ds.split(train=0.5, test=0.5, by_patient=True, seed=99) + + ids1 = {r.id for r in parts1['train'].resources} + ids2 = {r.id for r in parts2['train'].resources} + assert ids1 != ids2 + + def test_three_way_split_no_leakage(self): + "Test that when performing a three-way split by patient, no patient appears in more than one split, even if they have multiple resources. " + ds = _make_dataset([str(i) for i in range(9)]) + parts = ds.split(train=0.7, val=0.15, test=0.15, by_patient=True, seed=42) + + assert set(parts.keys()) == {'train', 'val', 'test'} + train_pids = {r.get_patient_id() for r in parts['train'].resources} + val_pids = {r.get_patient_id() for r in parts['val'].resources} + test_pids = {r.get_patient_id() for r in parts['test'].resources} + assert not (train_pids & val_pids) + assert not (train_pids & test_pids) + assert not (val_pids & test_pids) + + def test_patients_not_split_across_boundaries(self): + "Test that when splitting by patient, if a patient has multiple resources, all of their resources end up in the same split, and that no patient appears in more than one split. " + patient_ids = ['big'] * 5 + ['A', 'B', 'C', 'D', 'E'] + ds = _make_dataset(patient_ids) + parts = ds.split(train=0.5, test=0.5, by_patient=True, seed=0) + + all_big_splits = set() + for split_name, split_ds in parts.items(): + for r in split_ds.resources: + if r.patient_id == 'big': + all_big_splits.add(split_name) + + assert len(all_big_splits) == 1, "Patient 'big' appears in more than one split" + + def test_mutual_exclusion_with_project_splits(self): + "Test that when splitting by patient, if use_project_splits=True is also passed, a ValueError is raised indicating that the two options cannot be combined." + ds = _make_dataset(['A', 'B']) + with pytest.raises(ValueError, match='cannot be combined'): + ds.split(train=0.7, test=0.3, by_patient=True, use_project_splits=True) + + def test_mutual_exclusion_with_server_splits(self): + "Test that when splitting by patient, if use_server_splits=True is also passed, a ValueError is raised indicating that the two options cannot be combined." + ds = _make_dataset(['A', 'B']) + with pytest.raises(ValueError, match='cannot be combined'): + ds.split(train=0.7, test=0.3, by_patient=True, use_server_splits=True) + + def test_requires_ratio_kwargs(self): + "Test that when splitting by patient, if no ratio kwargs (train, val, test) are provided, a ValueError is raised indicating that at least one ratio must be specified." + ds = _make_dataset(['A', 'B']) + with pytest.raises(ValueError, match='requires ratio'): + ds.split(by_patient=True)