[Bug]: Spleen dataset downloader picks cases lexicographically, so --num_cases N is not the first N - #1062
Conversation
|
✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description. |
`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_<N>.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 <alexandre.triay_bagur@kcl.ac.uk>
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 <alexandre.triay_bagur@kcl.ac.uk>
deda885 to
3dcc52d
Compare
|
Ready for review 🟢 CI — all 31 checks green on Tests — Two things worth a reviewer's eye:
Verification beyond CI — exercised Copilot review also requested. Split out of #998, which has been rewritten separately to record why the full spleen e2e can't run in CI. |
There was a problem hiding this comment.
Pull request overview
Fixes the spleen tutorial dataset reorganiser so --num_cases N selects the N lowest-numbered MSD Task09 cases (natural/numeric ordering) rather than the first N in lexicographic filename order, and adds a CPU-only regression test to pin the behaviour in CI.
Changes:
- Switch
reorganise_spleen_dataset()tonatsorted(...)and filter hidden files before sorting so macOS resource forks (._spleen_*.nii.gz) never consume a case slot. - Add a CPU-only pytest (
fl-tutorials/tests/) that exercises case selection, kept output structure, and the resource-fork skip behaviour with empty fixture files (no download / no GPU). - Add
natsorttoflip-utils’ dev dependency group (and updateuv.lock) so the fl-tutorials test environment can import the downloader script under test.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
fl-tutorials/nvflare/image_segmentation/3d_spleen_segmentation/utils/download_spleen_dataset.py |
Natural-sort case filenames and pre-filter hidden entries; clarify docstring/CLI help about “lowest-numbered cases”. |
fl-tutorials/tests/test_spleen_case_selection.py |
New CPU-only regression tests for --num_cases ordering and hidden resource-fork handling. |
flip-utils/pyproject.toml |
Adds natsort to the dev group so fl-tutorials tests (run in the flip-utils env) can import the downloader. |
flip-utils/uv.lock |
Lockfile update reflecting the dev dependency addition and current lock options format. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Closes #1060. Split out of #998, where this was a footnote.
What was wrong
reorganise_spleen_datasetpicked which MSD Task09 cases to keep with a plainsorted(os.listdir(images_dir)). Case numbers are unpadded, so that sort is lexicographic —spleen_19lands beforespleen_2('1' < '2'), andspleen_2beforespleen_20('.' < '0'):So
--num_cases 10kept cases 17–20 and skipped 3, 6, 8 and 9.What changed
natsorted.natsort>=8.4.0is already a declared dependency of this tutorial (3d_spleen_segmentation/pyproject.toml:8) and already used by the siblingutils/create_spleen_accession_csv.py:17,50, which natural-sorts the subject IDs derived from these same filenames. The two orderings have to agree, so this uses the same tool rather than a bespoke key.._spleen_<N>.nii.gz) beside the real volumes; natsort would happily interleave them, and one counted as a case would silently cost a real case its slot.natsortadded toflip-utils'devgroup.fl-tutorials/tests/runs in flip-utils' environment by design (tests/pytest.ini), so the test needs to be able to import the script under test. Same test-only shape as theflwrentry beside it — not a runtime dependency, and[dependency-groups]is not part of the publishedflip-utilsdistribution.--num_caseshelp now say which cases are kept.Scope — this is predictability, not coverage
Checked against the
aicentreflip/trust-datasource_trustsplit (21/20):--num_cases 10yields 5 usable image/label pairs per trust under either ordering, because the trusts alternate across the case numbers. A full run still needsNUM_CASES=41, and #955 already warns with the coverage fraction and names the missing cases. The value here is that--num_cases Nmeans what it says — which matters if a subset is ever selected deliberately, and stops the flag looking broken.Verification
New CPU-only test
fl-tutorials/tests/test_spleen_case_selection.py— empty fixture files, no dataset download, no GPU — so it runs in the existingFL Tutorials CIworkflow (already path-filtered onfl-tutorials/**). It pins the observable behaviour (whichsubject_<N>/directories exist afterwards), not an internal helper. Red before the fix, green after:Also exercised
natsortedread-only against the real 41-caseTask09_Spleen/imagesTron the dev box: 82 raw entries → 41 kept, first ten2 3 6 8 9 10 12 13 14 16, and the full list equalssorted(..., key=int). Nomake -C fl-tutorials download-spleen-datare-run: the reorganiserrmtrees its source, so a live run would have destroyed the existing local dataset for no extra signal.Acceptance Criteria
Imported from issue #1060
--num_cases Nkeeps the N numerically-lowest cases (N=10→ cases 2, 3, 6, 8, 9, 10, 12, 13, 14, 16).fl-tutorials/tests/— no dataset download, no GPU — so it runs in the existingFL Tutorials CIworkflow.._spleen_*.nii.gz) are still skipped and never consume a case slot.