From 4ed61a66850ba493daf0c4a9631075906d9b32f4 Mon Sep 17 00:00:00 2001 From: at24_bioeng625-pc Date: Wed, 26 Aug 2026 14:49:22 +0100 Subject: [PATCH 1/2] fix: keep the lowest-numbered spleen cases for --num_cases N (#1060) `reorganise_spleen_dataset` chose which MSD Task09 cases to keep with a plain `sorted(os.listdir(...))`. Case numbers are unpadded, so that sort is lexicographic: `spleen_19` lands before `spleen_2` ('1' < '2') and `spleen_2` before `spleen_20` ('.' < '0'). `--num_cases 10` therefore kept cases {10, 12, 13, 14, 16, 17, 18, 19, 2, 20} rather than the ten lowest-numbered ones, skipping 3, 6, 8 and 9. Sort on the parsed case number instead, and move the hidden-file filter ahead of the sort so the archive's macOS resource forks (`._spleen_.nii.gz`) never reach the key -- they are not cases, and one counted as a case would silently cost a real one its slot. This does not change coverage and is not a training bug: against the `aicentreflip/trust-data` 21/20 `source_trust` split, `--num_cases 10` yields five usable image/label pairs per trust under either ordering, and a full run still needs `NUM_CASES=41`. It makes `--num_cases N` mean what it says, which matters if a subset is ever selected deliberately. Covered by a new CPU-only test in `fl-tutorials/tests/` -- empty fixture files, no dataset download, no GPU -- so it runs in the existing FL Tutorials CI. Signed-off-by: at24_bioeng625-pc --- .../utils/download_spleen_dataset.py | 42 +++++- .../tests/test_spleen_case_selection.py | 131 ++++++++++++++++++ 2 files changed, 167 insertions(+), 6 deletions(-) create mode 100644 fl-tutorials/tests/test_spleen_case_selection.py diff --git a/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py b/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py index 169c0b973..194e3d781 100644 --- a/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py +++ b/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py @@ -12,12 +12,34 @@ import argparse import os +import re import shutil +import sys from monai.apps.utils import download_and_extract MAX_CASES = 41 +# An imagesTr entry, e.g. "spleen_9.nii.gz". The case number is unpadded, which is why it has to be +# parsed out rather than sorted on as text. +_CASE_FILENAME = re.compile(r"spleen_(\d+)\.nii\.gz") + + +def _case_number(image_filename): + """Sort key for an ``imagesTr`` entry: the case number in ``spleen_.nii.gz``. + + Args: + image_filename (str): The bare filename, e.g. ``spleen_9.nii.gz``. + + Returns: + tuple[int, str]: The case number, then the filename. The filename is a tiebreak so an entry + that does not follow the convention still orders deterministically — it sorts last rather + than raising, leaving the decision on whether to keep it where it already lives (the + image/label existence check below). + """ + match = _CASE_FILENAME.fullmatch(image_filename) + return (int(match.group(1)) if match else sys.maxsize, image_filename) + def download_spleen_dataset(filepath, output_dir): """ @@ -65,7 +87,8 @@ def reorganise_spleen_dataset(output_dir, num_cases): Args: output_dir (str): The directory where the original downloaded dataset is located and where the reorganized dataset will be saved. - num_cases (int): Number of cases to keep from the dataset. + num_cases (int): Number of cases to keep from the dataset — the ``num_cases`` cases with + the lowest case numbers. """ base_dir = os.path.join(output_dir, "Task09_Spleen") images_dir = os.path.join(base_dir, "imagesTr") @@ -74,16 +97,22 @@ def reorganise_spleen_dataset(output_dir, num_cases): # Ensure output directory exists os.makedirs(output_dir, exist_ok=True) - # List all image files - image_files = sorted(os.listdir(images_dir)) + # List all image files, lowest case number first. Case numbers are unpadded (spleen_2 … + # spleen_63), so sorting the names as text is lexicographic: it puts spleen_19 before spleen_2 + # ('1' < '2') and spleen_2 before spleen_20 ('.' < '0'), which made --num_cases N keep an + # arbitrary-looking subset instead of the N lowest-numbered cases (FLIP#1060). + # + # The hidden-file filter runs before the sort, not inside the loop: the archive carries macOS + # resource forks (._spleen_.nii.gz) alongside the real volumes, and they must not reach the + # sort key — they are not cases, and one counted as a case would silently cost a real one its + # slot. + image_files = sorted((f for f in os.listdir(images_dir) if not f.startswith(".")), key=_case_number) # Process each image file print(f"Copying up to {num_cases} images and labels to subject folders in {output_dir}...") copied_cases = 0 for img_file in image_files: - if img_file.startswith("."): - continue if copied_cases >= num_cases: break @@ -135,7 +164,8 @@ def reorganise_spleen_dataset(output_dir, num_cases): "-n", type=int, default=10, - help=f"number of cases to keep after download and reorganization (1 to {MAX_CASES}).", + help=f"number of cases to keep after download and reorganization (1 to {MAX_CASES}); " + "the lowest-numbered cases are kept.", ) args = parser.parse_args() diff --git a/fl-tutorials/tests/test_spleen_case_selection.py b/fl-tutorials/tests/test_spleen_case_selection.py new file mode 100644 index 000000000..40b3fbc86 --- /dev/null +++ b/fl-tutorials/tests/test_spleen_case_selection.py @@ -0,0 +1,131 @@ +# Copyright (c) 2026 Guy's and St Thomas' NHS Foundation Trust & King's College London +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# http://www.apache.org/licenses/LICENSE-2.0 +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Which MSD cases ``--num_cases N`` keeps (FLIP#1060). + +The reorganiser touches nothing but the filesystem, so the fixtures here are empty +``spleen_.nii.gz`` files and the suite stays CPU-only -- no dataset download, no MONAI +transform, no GPU. The downloader is a loose script rather than a package (same as the tutorial +apps in ``tutorial_apps.py``), so it is loaded from its path. + +MSD Task09 case numbers are sparse and unpadded, which is the whole point: a codepoint sort of +``spleen_.nii.gz`` puts ``spleen_19`` before ``spleen_2`` (``'1' < '2'``) and ``spleen_2`` +before ``spleen_20`` (``'.' < '0'``), so ``--num_cases 10`` used to keep {10, 12, 13, 14, 16, 17, +18, 19, 2, 20} rather than the ten lowest-numbered cases. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from types import ModuleType + +import pytest + +TUTORIALS_ROOT = Path(__file__).resolve().parents[1] +DOWNLOADER_PATH = ( + TUTORIALS_ROOT / "nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py" +) + +# The real Task09_Spleen training set: 41 cases, sparsely numbered from 2 to 63. +MSD_CASE_NUMBERS = ( + 2, 3, 6, 8, 9, 10, 12, 13, 14, 16, 17, 18, 19, 20, 21, 22, 24, 25, 26, 27, + 28, 29, 31, 32, 33, 38, 40, 41, 44, 45, 46, 47, 49, 52, 53, 56, 59, 60, 61, 62, 63, +) # fmt: skip + + +@pytest.fixture(scope="module") +def downloader() -> ModuleType: + """The downloader script, imported from its path under a unique module name.""" + module_name = "fl_tutorials_under_test.download_spleen_dataset" + if module_name in sys.modules: + return sys.modules[module_name] + + spec = importlib.util.spec_from_file_location(module_name, DOWNLOADER_PATH) + if spec is None or spec.loader is None: + raise ImportError(f"cannot load {DOWNLOADER_PATH}") + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + try: + spec.loader.exec_module(module) + except BaseException: + del sys.modules[module_name] + raise + return module + + +def _write_extracted_dataset(root: Path, case_numbers: tuple[int, ...] = MSD_CASE_NUMBERS) -> Path: + """Lay out an empty stand-in for the extracted archive and return the directory.""" + base_dir = root / "Task09_Spleen" + images_dir = base_dir / "imagesTr" + labels_dir = base_dir / "labelsTr" + images_dir.mkdir(parents=True) + labels_dir.mkdir(parents=True) + for case in case_numbers: + (images_dir / f"spleen_{case}.nii.gz").touch() + (labels_dir / f"spleen_{case}.nii.gz").touch() + return base_dir + + +def _kept_case_numbers(output_dir: Path) -> list[int]: + """The cases that survived reorganisation, in numeric order.""" + return sorted(int(path.name.removeprefix("subject_")) for path in output_dir.glob("subject_*")) + + +@pytest.mark.parametrize( + ("num_cases", "expected"), + [ + (1, [2]), + # The ten lowest-numbered cases. Under the old codepoint sort this was + # [2, 10, 12, 13, 14, 16, 17, 18, 19, 20] -- 3, 6, 8 and 9 skipped for 17..20. + (10, [2, 3, 6, 8, 9, 10, 12, 13, 14, 16]), + (41, list(MSD_CASE_NUMBERS)), + ], +) +def test_num_cases_keeps_the_lowest_numbered_cases( + downloader: ModuleType, tmp_path: Path, num_cases: int, expected: list[int] +) -> None: + """``--num_cases N`` keeps the N numerically-lowest cases, not the N lexicographically first.""" + _write_extracted_dataset(tmp_path) + + downloader.reorganise_spleen_dataset(str(tmp_path), num_cases) + + assert _kept_case_numbers(tmp_path) == expected + + +def test_kept_cases_carry_both_an_image_and_a_label(downloader: ModuleType, tmp_path: Path) -> None: + """Each kept subject gets the input/label pair the trainer's zero-pairs guard looks for.""" + _write_extracted_dataset(tmp_path) + + downloader.reorganise_spleen_dataset(str(tmp_path), 3) + + for case in (2, 3, 6): + scans = tmp_path / f"subject_{case}" / "scans" + assert (scans / f"input_spleen_{case}.nii.gz").is_file() + assert (scans / f"label_spleen_{case}.nii.gz").is_file() + + +def test_macos_resource_forks_are_skipped_without_consuming_a_slot( + downloader: ModuleType, tmp_path: Path +) -> None: + """The archive ships ``._spleen_.nii.gz`` siblings; they are not cases. + + They must not be sorted as cases either -- keying on the number alone would raise on a name + that has none, and counting one would silently cost a real case its slot. + """ + base_dir = _write_extracted_dataset(tmp_path) + for case in MSD_CASE_NUMBERS[:5]: + (base_dir / "imagesTr" / f"._spleen_{case}.nii.gz").touch() + + downloader.reorganise_spleen_dataset(str(tmp_path), 5) + + assert _kept_case_numbers(tmp_path) == [2, 3, 6, 8, 9] From 3dcc52de4b0096f8f12c62c9eb2cb9717d896b2f Mon Sep 17 00:00:00 2001 From: at24_bioeng625-pc Date: Wed, 26 Aug 2026 14:43:07 +0100 Subject: [PATCH 2/2] refactor: sort the spleen case list with natsort, not a hand-rolled key natsort is already a declared dependency of the spleen tutorial (3d_spleen_segmentation/pyproject.toml) and already used by the sibling utils/create_spleen_accession_csv.py, which natural-sorts the subject IDs derived from these same filenames. The two orderings have to agree, so use the same tool rather than a bespoke regex key. fl-tutorials/tests/ runs in flip-utils' environment (see tests/pytest.ini), so natsort joins flip-utils' dev group to let the test import the script under test -- the same test-only shape as the flwr entry beside it, and not a runtime dependency. The hidden-file filter stays ahead of the sort: natsort would happily interleave the archive's macOS resource forks with the real volumes, and one counted as a case would silently cost a real case its slot. Signed-off-by: at24_bioeng625-pc --- .../utils/download_spleen_dataset.py | 36 +++++-------------- .../tests/test_spleen_case_selection.py | 4 +-- flip-utils/pyproject.toml | 5 +++ flip-utils/uv.lock | 13 ++++++- 4 files changed, 27 insertions(+), 31 deletions(-) diff --git a/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py b/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py index 194e3d781..c123b4ef6 100644 --- a/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py +++ b/fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py @@ -12,34 +12,13 @@ import argparse import os -import re import shutil -import sys from monai.apps.utils import download_and_extract +from natsort import natsorted MAX_CASES = 41 -# An imagesTr entry, e.g. "spleen_9.nii.gz". The case number is unpadded, which is why it has to be -# parsed out rather than sorted on as text. -_CASE_FILENAME = re.compile(r"spleen_(\d+)\.nii\.gz") - - -def _case_number(image_filename): - """Sort key for an ``imagesTr`` entry: the case number in ``spleen_.nii.gz``. - - Args: - image_filename (str): The bare filename, e.g. ``spleen_9.nii.gz``. - - Returns: - tuple[int, str]: The case number, then the filename. The filename is a tiebreak so an entry - that does not follow the convention still orders deterministically — it sorts last rather - than raising, leaving the decision on whether to keep it where it already lives (the - image/label existence check below). - """ - match = _CASE_FILENAME.fullmatch(image_filename) - return (int(match.group(1)) if match else sys.maxsize, image_filename) - def download_spleen_dataset(filepath, output_dir): """ @@ -100,13 +79,14 @@ def reorganise_spleen_dataset(output_dir, num_cases): # List all image files, lowest case number first. Case numbers are unpadded (spleen_2 … # spleen_63), so sorting the names as text is lexicographic: it puts spleen_19 before spleen_2 # ('1' < '2') and spleen_2 before spleen_20 ('.' < '0'), which made --num_cases N keep an - # arbitrary-looking subset instead of the N lowest-numbered cases (FLIP#1060). + # arbitrary-looking subset instead of the N lowest-numbered cases (FLIP#1060). natsorted keys + # on the digit run, as create_spleen_accession_csv.py already does for the subject IDs derived + # from these same names — the two orderings have to agree. # - # The hidden-file filter runs before the sort, not inside the loop: the archive carries macOS - # resource forks (._spleen_.nii.gz) alongside the real volumes, and they must not reach the - # sort key — they are not cases, and one counted as a case would silently cost a real one its - # slot. - image_files = sorted((f for f in os.listdir(images_dir) if not f.startswith(".")), key=_case_number) + # The hidden-file filter runs before the sort rather than inside the loop below: the archive + # carries macOS resource forks (._spleen_.nii.gz) beside the real volumes, and one of those + # counted as a case would silently cost a real case its slot. + image_files = natsorted(f for f in os.listdir(images_dir) if not f.startswith(".")) # Process each image file print(f"Copying up to {num_cases} images and labels to subject folders in {output_dir}...") diff --git a/fl-tutorials/tests/test_spleen_case_selection.py b/fl-tutorials/tests/test_spleen_case_selection.py index 40b3fbc86..24cb72723 100644 --- a/fl-tutorials/tests/test_spleen_case_selection.py +++ b/fl-tutorials/tests/test_spleen_case_selection.py @@ -119,8 +119,8 @@ def test_macos_resource_forks_are_skipped_without_consuming_a_slot( ) -> None: """The archive ships ``._spleen_.nii.gz`` siblings; they are not cases. - They must not be sorted as cases either -- keying on the number alone would raise on a name - that has none, and counting one would silently cost a real case its slot. + They must not reach the sort either: natsort would happily interleave them with the real + volumes, and one counted as a case would silently cost a real case its slot. """ base_dir = _write_extracted_dataset(tmp_path) for case in MSD_CASE_NUMBERS[:5]: diff --git a/flip-utils/pyproject.toml b/flip-utils/pyproject.toml index 60ae1019e..cf7a7163c 100644 --- a/flip-utils/pyproject.toml +++ b/flip-utils/pyproject.toml @@ -183,6 +183,11 @@ dev = [ "huggingface-hub>=1.6.0", "matplotlib>=3.10.7", "mypy>=1.18.2,<2.0", + # Test-only: fl-tutorials/tests/ runs in this environment (see fl-tutorials/tests/pytest.ini) + # and imports the spleen downloader, which natural-sorts its case list. natsort is a real + # dependency of the spleen tutorial itself (3d_spleen_segmentation/pyproject.toml); it is here + # only so the test can import the script under test. Deliberately NOT a runtime dep. + "natsort>=8.4.0", "pyqt5>=5.15.11", "pytest>=8.4.2", "pytest-cov>=7.0.0", diff --git a/flip-utils/uv.lock b/flip-utils/uv.lock index cc4a22036..18559684d 100644 --- a/flip-utils/uv.lock +++ b/flip-utils/uv.lock @@ -3,7 +3,7 @@ revision = 3 requires-python = ">=3.12" [options] -exclude-newer = "2026-08-04T10:38:41.208290437Z" +exclude-newer = "0001-01-01T00:00:00Z" # This has no effect and is included for backwards compatibility when using relative exclude-newer values. exclude-newer-span = "P3D" [manifest] @@ -764,6 +764,7 @@ dev = [ { name = "huggingface-hub" }, { name = "matplotlib" }, { name = "mypy" }, + { name = "natsort" }, { name = "pyqt5" }, { name = "pytest" }, { name = "pytest-cov" }, @@ -799,6 +800,7 @@ dev = [ { name = "huggingface-hub", specifier = ">=1.6.0" }, { name = "matplotlib", specifier = ">=3.10.7" }, { name = "mypy", specifier = ">=1.18.2,<2.0" }, + { name = "natsort", specifier = ">=8.4.0" }, { name = "pyqt5", specifier = ">=5.15.11" }, { name = "pytest", specifier = ">=8.4.2" }, { name = "pytest-cov", specifier = ">=7.0.0" }, @@ -1797,6 +1799,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/48/ca/36339329c4604adbcc99c899b7eb1ce1a555c499b6a6860757dc9bfed36d/narwhals-2.22.1-py3-none-any.whl", hash = "sha256:60567d774edf77db53906f89d9fbd164e66e56d66d388e1e6990f17ac33cfb53", size = 454815, upload-time = "2026-06-05T12:34:32.289Z" }, ] +[[package]] +name = "natsort" +version = "8.4.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/e2/a9/a0c57aee75f77794adaf35322f8b6404cbd0f89ad45c87197a937764b7d0/natsort-8.4.0.tar.gz", hash = "sha256:45312c4a0e5507593da193dedd04abb1469253b601ecaf63445ad80f0a1ea581", size = 76575, upload-time = "2023-06-20T04:17:19.925Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/ef/82/7a9d0550484a62c6da82858ee9419f3dd1ccc9aa1c26a1e43da3ecd20b0d/natsort-8.4.0-py3-none-any.whl", hash = "sha256:4732914fb471f56b5cce04d7bae6f164a592c7712e1c85f9ef585e197299521c", size = 38268, upload-time = "2023-06-20T04:17:17.522Z" }, +] + [[package]] name = "networkx" version = "3.6"