ENH: Add CHOP-Valve4D auto-download; restructure dataset docs#82
Merged
Conversation
Extend DataDownloadTools/physiotwin4d-download-data with automatic
downloaders for KCL-Heart-Model (Zenodo) and CHOP-Valve4D (the three
CHOP-Valve4D-{Alterra,CT,TPV25}.zip assets on the 2026.07.1 GitHub
release, extracted into matching subdirs). Both are idempotent/resumable
and covered by unit tests.
Replace the monolithic data/README.md with a short top-level overview
table, and give each dataset its own README (Slicer-Heart-CT,
DirLab-4DCT, KCL-Heart-Model, CHOP-Valve4D, plus the pytest-only
data/test cache) documenting the download command, source URL(s),
directory layout, sizes, and a dedicated Citation section. Fold the
content of citation.txt/citations.txt/reading_img_files.url into those
Citation sections and remove the now-redundant files.
Commit the DirLab-4DCT .mhd MetaImage headers (metadata only, a few
hundred bytes each) so the expected case/phase layout is documented
in-repo; .gitignore now excludes *.mha instead of *.mhd so the large raw
volumes stay untracked. Update tutorials/experiments docs to reflect
KCL-Heart-Model's new auto-download path.
WalkthroughThis PR adds automated downloads for KCL-Heart-Model and CHOP-Valve4D, expands dataset and tutorial docs, adds DIR-Lab 4D-CT MetaImage headers and verification, and changes the ignored image extension from ChangesDataset Download and Documentation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR expands PhysioTwin4D’s dataset-management surface by adding auto-download + extraction support for KCL-Heart-Model (Zenodo tarballs) and CHOP-Valve4D (GitHub release zip assets), wiring both into the physiotwin4d-download-data CLI, and updating docs/data READMEs to a per-dataset structure. It also commits DIR-Lab 4DCT MetaImage header files (.mhd) to document expected case/phase layouts in-repo while keeping large volumes untracked.
Changes:
- Add KCL-Heart-Model + CHOP-Valve4D download/extract helpers and route them through
physiotwin4d-download-data. - Add unit tests covering KCL tar extraction and CHOP zip extraction + CLI routing.
- Restructure
data/documentation into a top-level dataset index + per-dataset READMEs; commit DIR-Lab.mhdheaders and adjust ignore patterns to keep large volumes out of git.
Reviewed changes
Copilot reviewed 118 out of 119 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/README.md | Update tutorial index to reflect KCL-Heart-Model is now auto-downloadable. |
| tests/test_download_heart_data.py | Add synthetic tests for KCL tar extraction and CHOP zip extraction behaviors. |
| tests/test_download_data_cli.py | Add CLI routing tests for the new dataset download commands. |
| src/physiotwin4d/data_download_tools.py | Implement KCL tar-member extraction and CHOP zip download/extraction helpers. |
| src/physiotwin4d/cli/download_data.py | Extend CLI to support KCL-Heart-Model and CHOP-Valve4D + default directory mapping. |
| experiments/Heart-Create_Statistical_Model/README.md | Update experiment docs to use the auto-download CLI for KCL-Heart-Model. |
| docs/tutorials.rst | Update tutorial dataset notes to reference physiotwin4d-download-data. |
| docs/cli_scripts/download_data.rst | Document newly supported datasets and their download outputs/behavior. |
| data/test/README.md | Add documentation clarifying data/test/ is pytest-managed cache, not a user dataset. |
| data/Slicer-Heart-CT/README.md | Add dataset-specific README and usage notes for Slicer-Heart-CT. |
| data/Slicer-Heart-CT/download_and_convert.py | Remove legacy download/convert script from the dataset directory. |
| data/README.md | Replace monolithic dataset README with a short index table + guidance. |
| data/KCL-Heart-Model/README.md | Update KCL dataset README to document the auto-download flow and structure. |
| data/KCL-Heart-Model/citation.txt | Remove redundant citation file after folding citation info into README. |
| data/DirLab-4DCT/README.md | Add DIR-Lab dataset README explaining manual download and committed .mhd headers. |
| data/DirLab-4DCT/reading_img_files.url | Remove redundant link file after folding reference into README. |
| data/DirLab-4DCT/citations.txt | Remove redundant citations file after folding citations into README. |
| data/DirLab-4DCT/Case1Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T00 layout. |
| data/DirLab-4DCT/Case1Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T10 layout. |
| data/DirLab-4DCT/Case1Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T20 layout. |
| data/DirLab-4DCT/Case1Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T30 layout. |
| data/DirLab-4DCT/Case1Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T40 layout. |
| data/DirLab-4DCT/Case1Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T50 layout. |
| data/DirLab-4DCT/Case1Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T60 layout. |
| data/DirLab-4DCT/Case1Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T70 layout. |
| data/DirLab-4DCT/Case1Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T80 layout. |
| data/DirLab-4DCT/Case1Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case1 phase T90 layout. |
| data/DirLab-4DCT/Case2Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T00 layout. |
| data/DirLab-4DCT/Case2Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T10 layout. |
| data/DirLab-4DCT/Case2Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T20 layout. |
| data/DirLab-4DCT/Case2Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T30 layout. |
| data/DirLab-4DCT/Case2Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T40 layout. |
| data/DirLab-4DCT/Case2Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T50 layout. |
| data/DirLab-4DCT/Case2Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T60 layout. |
| data/DirLab-4DCT/Case2Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T70 layout. |
| data/DirLab-4DCT/Case2Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T80 layout. |
| data/DirLab-4DCT/Case2Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case2 phase T90 layout. |
| data/DirLab-4DCT/Case3Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T00 layout. |
| data/DirLab-4DCT/Case3Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T10 layout. |
| data/DirLab-4DCT/Case3Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T20 layout. |
| data/DirLab-4DCT/Case3Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T30 layout. |
| data/DirLab-4DCT/Case3Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T40 layout. |
| data/DirLab-4DCT/Case3Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T50 layout. |
| data/DirLab-4DCT/Case3Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T60 layout. |
| data/DirLab-4DCT/Case3Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T70 layout. |
| data/DirLab-4DCT/Case3Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T80 layout. |
| data/DirLab-4DCT/Case3Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case3 phase T90 layout. |
| data/DirLab-4DCT/Case4Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T00 layout. |
| data/DirLab-4DCT/Case4Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T10 layout. |
| data/DirLab-4DCT/Case4Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T20 layout. |
| data/DirLab-4DCT/Case4Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T30 layout. |
| data/DirLab-4DCT/Case4Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T40 layout. |
| data/DirLab-4DCT/Case4Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T50 layout. |
| data/DirLab-4DCT/Case4Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T60 layout. |
| data/DirLab-4DCT/Case4Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T70 layout. |
| data/DirLab-4DCT/Case4Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T80 layout. |
| data/DirLab-4DCT/Case4Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case4 phase T90 layout. |
| data/DirLab-4DCT/Case5Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T00 layout. |
| data/DirLab-4DCT/Case5Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T10 layout. |
| data/DirLab-4DCT/Case5Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T20 layout. |
| data/DirLab-4DCT/Case5Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T30 layout. |
| data/DirLab-4DCT/Case5Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T40 layout. |
| data/DirLab-4DCT/Case5Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T50 layout. |
| data/DirLab-4DCT/Case5Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T60 layout. |
| data/DirLab-4DCT/Case5Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T70 layout. |
| data/DirLab-4DCT/Case5Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T80 layout. |
| data/DirLab-4DCT/Case5Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case5 phase T90 layout. |
| data/DirLab-4DCT/Case6Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T00 layout. |
| data/DirLab-4DCT/Case6Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T10 layout. |
| data/DirLab-4DCT/Case6Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T20 layout. |
| data/DirLab-4DCT/Case6Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T30 layout. |
| data/DirLab-4DCT/Case6Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T40 layout. |
| data/DirLab-4DCT/Case6Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T50 layout. |
| data/DirLab-4DCT/Case6Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T60 layout. |
| data/DirLab-4DCT/Case6Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T70 layout. |
| data/DirLab-4DCT/Case6Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T80 layout. |
| data/DirLab-4DCT/Case6Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case6 phase T90 layout. |
| data/DirLab-4DCT/Case7Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T00 layout. |
| data/DirLab-4DCT/Case7Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T10 layout. |
| data/DirLab-4DCT/Case7Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T20 layout. |
| data/DirLab-4DCT/Case7Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T30 layout. |
| data/DirLab-4DCT/Case7Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T40 layout. |
| data/DirLab-4DCT/Case7Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T50 layout. |
| data/DirLab-4DCT/Case7Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T60 layout. |
| data/DirLab-4DCT/Case7Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T70 layout. |
| data/DirLab-4DCT/Case7Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T80 layout. |
| data/DirLab-4DCT/Case7Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case7 phase T90 layout. |
| data/DirLab-4DCT/Case8Deploy_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T00 layout. |
| data/DirLab-4DCT/Case8Deploy_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T10 layout. |
| data/DirLab-4DCT/Case8Deploy_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T20 layout. |
| data/DirLab-4DCT/Case8Deploy_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T30 layout. |
| data/DirLab-4DCT/Case8Deploy_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T40 layout. |
| data/DirLab-4DCT/Case8Deploy_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T50 layout. |
| data/DirLab-4DCT/Case8Deploy_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T60 layout. |
| data/DirLab-4DCT/Case8Deploy_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T70 layout. |
| data/DirLab-4DCT/Case8Deploy_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T80 layout. |
| data/DirLab-4DCT/Case8Deploy_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case8 phase T90 layout. |
| data/DirLab-4DCT/Case9Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T00 layout. |
| data/DirLab-4DCT/Case9Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T10 layout. |
| data/DirLab-4DCT/Case9Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T20 layout. |
| data/DirLab-4DCT/Case9Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T30 layout. |
| data/DirLab-4DCT/Case9Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T40 layout. |
| data/DirLab-4DCT/Case9Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T50 layout. |
| data/DirLab-4DCT/Case9Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T60 layout. |
| data/DirLab-4DCT/Case9Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T70 layout. |
| data/DirLab-4DCT/Case9Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T80 layout. |
| data/DirLab-4DCT/Case9Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case9 phase T90 layout. |
| data/DirLab-4DCT/Case10Pack_T00.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T00 layout. |
| data/DirLab-4DCT/Case10Pack_T10.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T10 layout. |
| data/DirLab-4DCT/Case10Pack_T20.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T20 layout. |
| data/DirLab-4DCT/Case10Pack_T30.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T30 layout. |
| data/DirLab-4DCT/Case10Pack_T40.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T40 layout. |
| data/DirLab-4DCT/Case10Pack_T50.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T50 layout. |
| data/DirLab-4DCT/Case10Pack_T60.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T60 layout. |
| data/DirLab-4DCT/Case10Pack_T70.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T70 layout. |
| data/DirLab-4DCT/Case10Pack_T80.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T80 layout. |
| data/DirLab-4DCT/Case10Pack_T90.mhd | Add committed MetaImage header documenting DIR-Lab Case10 phase T90 layout. |
| data/CHOP-Valve4D/README.md | Add dataset-specific README describing the GitHub-release convenience bundle + citations. |
| .gitignore | Adjust ignored image extensions to allow committed .mhd headers while ignoring .mha. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/test_download_heart_data.py (1)
116-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a KCL skip test parallel to the CHOP skip test.
DownloadKCLHeartModelDataskips meshes whose target.vtkalready exists and is non-empty (data_download_tools.pylines 117-118), but there's no test verifying this behavior. The CHOP download has a dedicated skip test at lines 195-219. A parallel KCL test would verify the idempotent/resumable claim for KCL.Would you like me to generate the test?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_download_heart_data.py` around lines 116 - 158, Add a KCL-specific skip/resume test alongside the existing CHOP skip coverage to verify DownloadKCLHeartModelData does not re-download meshes when the target .vtk already exists and is non-empty. Reuse the same pattern as test_download_kcl_heart_model_data, but precreate one or more files under the output_dir/input_meshes directory before calling DataDownloadTools.DownloadKCLHeartModelData, and assert the existing files are preserved while the remaining archives are still downloaded and unpacked. Use the unique symbols DownloadKCLHeartModelData, VerifyKCLHeartModelData, and KCL_HEART_MODEL_INDIVIDUAL_URL_TEMPLATE to anchor the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@data/CHOP-Valve4D/README.md`:
- Around line 30-31: The README wording overstates resumability for
DownloadCHOPValve4DData(), since it only skips subdirectories that already
contain files and does not verify a complete extraction. Update the
documentation text to avoid promising interrupted-download resume behavior, and
instead describe the actual behavior using the DownloadCHOPValve4DData() logic
and the Alterra/, CT/, and TPV25/ subdirectory handling, or implement atomic
staging/validation if you want to keep the resumability claim.
- Around line 33-49: The directory tree in the README is an unlabeled fenced
block, which triggers the fenced-code-language lint rule. Update the fenced
listing in the README to use a suitable language tag such as text, keeping the
existing directory-tree content unchanged.
In `@data/DirLab-4DCT/Case1Pack_T40.mhd`:
- Around line 1-11: Update VerifyDirLab4DCTData in data_download_tools.py so it
does not rely only on finding Case1Pack_T*.mhd headers, since committed headers
can create a false positive. Extend the check to validate the actual image
payload referenced by the header, using the ElementDataFile path from the .mhd
file or by confirming the expected .img/.mha files exist alongside it. Keep the
existing verification flow and symbol names unchanged, but make the
downloaded-data test fail unless both the header and its backing data are
present.
In `@data/DirLab-4DCT/README.md`:
- Around line 24-37: The README has unlabeled Markdown code fences in the
directory tree and raw-header examples, which triggers MD040. Update the fenced
blocks in the README to use an explicit language tag, using the same label
consistently for the tree-style and header-content examples, and make sure the
affected fenced sections are updated wherever they appear in the document.
In `@data/Slicer-Heart-CT/README.md`:
- Around line 25-30: The README’s directory listing uses an unlabeled fenced
block, which triggers markdownlint; update the fenced block under the directory
structure section to use a text label instead of an anonymous fence. Keep the
content the same and adjust the markdown in README.md so the tree listing is
explicitly marked as text.
In `@data/test/README.md`:
- Around line 30-32: The Markdown code fence in the test command example is
missing its language label, which causes markdownlint to fail. Update the fenced
block in the README example to use the bash label, keeping the existing pytest
command content unchanged. Use the nearby README test command snippet as the
target location.
In `@src/physiotwin4d/data_download_tools.py`:
- Around line 202-209: The ZIP extraction flow in `_DownloadAndExtractZip` is
non-atomic and unsafe, so incomplete extractions can cause
`DownloadCHOPValve4DData` to skip broken subdirectories and untrusted archive
paths can escape the target directory. Update `_DownloadAndExtractZip` to
extract into a temporary subdirectory, validate each ZIP member path before
extraction, and only rename the temp directory to `target_dir` after successful
completion, following the same atomic pattern used by
`_DownloadAndExtractTarMember`.
---
Nitpick comments:
In `@tests/test_download_heart_data.py`:
- Around line 116-158: Add a KCL-specific skip/resume test alongside the
existing CHOP skip coverage to verify DownloadKCLHeartModelData does not
re-download meshes when the target .vtk already exists and is non-empty. Reuse
the same pattern as test_download_kcl_heart_model_data, but precreate one or
more files under the output_dir/input_meshes directory before calling
DataDownloadTools.DownloadKCLHeartModelData, and assert the existing files are
preserved while the remaining archives are still downloaded and unpacked. Use
the unique symbols DownloadKCLHeartModelData, VerifyKCLHeartModelData, and
KCL_HEART_MODEL_INDIVIDUAL_URL_TEMPLATE to anchor the new test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19627c72-88de-440c-ad5c-b78a02a0fa2a
📒 Files selected for processing (119)
.gitignoredata/CHOP-Valve4D/README.mddata/DirLab-4DCT/Case10Pack_T00.mhddata/DirLab-4DCT/Case10Pack_T10.mhddata/DirLab-4DCT/Case10Pack_T20.mhddata/DirLab-4DCT/Case10Pack_T30.mhddata/DirLab-4DCT/Case10Pack_T40.mhddata/DirLab-4DCT/Case10Pack_T50.mhddata/DirLab-4DCT/Case10Pack_T60.mhddata/DirLab-4DCT/Case10Pack_T70.mhddata/DirLab-4DCT/Case10Pack_T80.mhddata/DirLab-4DCT/Case10Pack_T90.mhddata/DirLab-4DCT/Case1Pack_T00.mhddata/DirLab-4DCT/Case1Pack_T10.mhddata/DirLab-4DCT/Case1Pack_T20.mhddata/DirLab-4DCT/Case1Pack_T30.mhddata/DirLab-4DCT/Case1Pack_T40.mhddata/DirLab-4DCT/Case1Pack_T50.mhddata/DirLab-4DCT/Case1Pack_T60.mhddata/DirLab-4DCT/Case1Pack_T70.mhddata/DirLab-4DCT/Case1Pack_T80.mhddata/DirLab-4DCT/Case1Pack_T90.mhddata/DirLab-4DCT/Case2Pack_T00.mhddata/DirLab-4DCT/Case2Pack_T10.mhddata/DirLab-4DCT/Case2Pack_T20.mhddata/DirLab-4DCT/Case2Pack_T30.mhddata/DirLab-4DCT/Case2Pack_T40.mhddata/DirLab-4DCT/Case2Pack_T50.mhddata/DirLab-4DCT/Case2Pack_T60.mhddata/DirLab-4DCT/Case2Pack_T70.mhddata/DirLab-4DCT/Case2Pack_T80.mhddata/DirLab-4DCT/Case2Pack_T90.mhddata/DirLab-4DCT/Case3Pack_T00.mhddata/DirLab-4DCT/Case3Pack_T10.mhddata/DirLab-4DCT/Case3Pack_T20.mhddata/DirLab-4DCT/Case3Pack_T30.mhddata/DirLab-4DCT/Case3Pack_T40.mhddata/DirLab-4DCT/Case3Pack_T50.mhddata/DirLab-4DCT/Case3Pack_T60.mhddata/DirLab-4DCT/Case3Pack_T70.mhddata/DirLab-4DCT/Case3Pack_T80.mhddata/DirLab-4DCT/Case3Pack_T90.mhddata/DirLab-4DCT/Case4Pack_T00.mhddata/DirLab-4DCT/Case4Pack_T10.mhddata/DirLab-4DCT/Case4Pack_T20.mhddata/DirLab-4DCT/Case4Pack_T30.mhddata/DirLab-4DCT/Case4Pack_T40.mhddata/DirLab-4DCT/Case4Pack_T50.mhddata/DirLab-4DCT/Case4Pack_T60.mhddata/DirLab-4DCT/Case4Pack_T70.mhddata/DirLab-4DCT/Case4Pack_T80.mhddata/DirLab-4DCT/Case4Pack_T90.mhddata/DirLab-4DCT/Case5Pack_T00.mhddata/DirLab-4DCT/Case5Pack_T10.mhddata/DirLab-4DCT/Case5Pack_T20.mhddata/DirLab-4DCT/Case5Pack_T30.mhddata/DirLab-4DCT/Case5Pack_T40.mhddata/DirLab-4DCT/Case5Pack_T50.mhddata/DirLab-4DCT/Case5Pack_T60.mhddata/DirLab-4DCT/Case5Pack_T70.mhddata/DirLab-4DCT/Case5Pack_T80.mhddata/DirLab-4DCT/Case5Pack_T90.mhddata/DirLab-4DCT/Case6Pack_T00.mhddata/DirLab-4DCT/Case6Pack_T10.mhddata/DirLab-4DCT/Case6Pack_T20.mhddata/DirLab-4DCT/Case6Pack_T30.mhddata/DirLab-4DCT/Case6Pack_T40.mhddata/DirLab-4DCT/Case6Pack_T50.mhddata/DirLab-4DCT/Case6Pack_T60.mhddata/DirLab-4DCT/Case6Pack_T70.mhddata/DirLab-4DCT/Case6Pack_T80.mhddata/DirLab-4DCT/Case6Pack_T90.mhddata/DirLab-4DCT/Case7Pack_T00.mhddata/DirLab-4DCT/Case7Pack_T10.mhddata/DirLab-4DCT/Case7Pack_T20.mhddata/DirLab-4DCT/Case7Pack_T30.mhddata/DirLab-4DCT/Case7Pack_T40.mhddata/DirLab-4DCT/Case7Pack_T50.mhddata/DirLab-4DCT/Case7Pack_T60.mhddata/DirLab-4DCT/Case7Pack_T70.mhddata/DirLab-4DCT/Case7Pack_T80.mhddata/DirLab-4DCT/Case7Pack_T90.mhddata/DirLab-4DCT/Case8Deploy_T00.mhddata/DirLab-4DCT/Case8Deploy_T10.mhddata/DirLab-4DCT/Case8Deploy_T20.mhddata/DirLab-4DCT/Case8Deploy_T30.mhddata/DirLab-4DCT/Case8Deploy_T40.mhddata/DirLab-4DCT/Case8Deploy_T50.mhddata/DirLab-4DCT/Case8Deploy_T60.mhddata/DirLab-4DCT/Case8Deploy_T70.mhddata/DirLab-4DCT/Case8Deploy_T80.mhddata/DirLab-4DCT/Case8Deploy_T90.mhddata/DirLab-4DCT/Case9Pack_T00.mhddata/DirLab-4DCT/Case9Pack_T10.mhddata/DirLab-4DCT/Case9Pack_T20.mhddata/DirLab-4DCT/Case9Pack_T30.mhddata/DirLab-4DCT/Case9Pack_T40.mhddata/DirLab-4DCT/Case9Pack_T50.mhddata/DirLab-4DCT/Case9Pack_T60.mhddata/DirLab-4DCT/Case9Pack_T70.mhddata/DirLab-4DCT/Case9Pack_T80.mhddata/DirLab-4DCT/Case9Pack_T90.mhddata/DirLab-4DCT/README.mddata/DirLab-4DCT/citations.txtdata/DirLab-4DCT/reading_img_files.urldata/KCL-Heart-Model/README.mddata/KCL-Heart-Model/citation.txtdata/README.mddata/Slicer-Heart-CT/README.mddata/Slicer-Heart-CT/download_and_convert.pydata/test/README.mddocs/cli_scripts/download_data.rstdocs/tutorials.rstexperiments/Heart-Create_Statistical_Model/README.mdsrc/physiotwin4d/cli/download_data.pysrc/physiotwin4d/data_download_tools.pytests/test_download_data_cli.pytests/test_download_heart_data.pytutorials/README.md
💤 Files with no reviewable changes (4)
- data/DirLab-4DCT/reading_img_files.url
- data/KCL-Heart-Model/citation.txt
- data/DirLab-4DCT/citations.txt
- data/Slicer-Heart-CT/download_and_convert.py
Comment on lines
+1
to
+11
| ObjectType = Image | ||
| NDims = 3 | ||
| DimSize = 256 256 94 | ||
| HeaderSize = -1 | ||
| BinaryData = True | ||
| BinaryDataByteOrderMSB = False | ||
| Orientation = 1 0 0 0 1 0 0 0 -1 | ||
| ElementSpacing = 0.97 0.97 2.5 | ||
| ElementType = MET_SHORT | ||
| ElementNumberOfChannels = 1 | ||
| ElementDataFile = Case1Pack/Images/case1_T40_s.img |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Committed .mhd headers will cause VerifyDirLab4DCTData to report a false positive.
VerifyDirLab4DCTData (in data_download_tools.py) checks any(data_dir.glob("Case1Pack_T*.mhd")) to determine whether DirLab data is downloaded. Since these .mhd headers are now committed to the repo, that check will always return True — even when the referenced raw .img files (e.g., Case1Pack/Images/case1_T40_s.img) are absent. Downstream workflows that gate on this verification will proceed to load images and fail with confusing errors instead of receiving a clear "data not downloaded" message.
Consider updating VerifyDirLab4DCTData to also confirm that at least one referenced ElementDataFile exists on disk, or check for the presence of .img / .mha files alongside the .mhd headers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/DirLab-4DCT/Case1Pack_T40.mhd` around lines 1 - 11, Update
VerifyDirLab4DCTData in data_download_tools.py so it does not rely only on
finding Case1Pack_T*.mhd headers, since committed headers can create a false
positive. Extend the check to validate the actual image payload referenced by
the header, using the ElementDataFile path from the .mhd file or by confirming
the expected .img/.mha files exist alongside it. Keep the existing verification
flow and symbol names unchanged, but make the downloaded-data test fail unless
both the header and its backing data are present.
Comment on lines
+24
to
+37
| **Directory structure after download:** | ||
| ``` | ||
| data/DirLab-4DCT/ | ||
| ├── Case1Pack/ | ||
| │ ├── Images/ # T00-T90 phase images | ||
| │ ├── ExtremePhases/ # T00 and T50 (max inhale/exhale) | ||
| │ └── Sampled4D/ # Sampled time points | ||
| ├── Case1Pack_T00.mhd # Already-committed headers, per phase | ||
| ├── Case1Pack_T10.mhd | ||
| ... | ||
| ├── Case10Pack/ | ||
| ├── Convert4DCTToMHD.py # Documents how the .mhd headers were generated | ||
| └── README.md (this file) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to the unlabeled fences.
The directory tree block and the raw-header example block are missing a fence language, which triggers MD040 and keeps the README lint-noisy.
Proposed fix
-```
+```text
...
-```
+```textAlso applies to: 45-51
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/DirLab-4DCT/README.md` around lines 24 - 37, The README has unlabeled
Markdown code fences in the directory tree and raw-header examples, which
triggers MD040. Update the fenced blocks in the README to use an explicit
language tag, using the same label consistently for the tree-style and
header-content examples, and make sure the affected fenced sections are updated
wherever they appear in the document.
Source: Linters/SAST tools
Comment on lines
+25
to
+30
| **Directory structure after download:** | ||
| ``` | ||
| data/Slicer-Heart-CT/ | ||
| ├── TruncalValve_4DCT.seq.nrrd | ||
| └── README.md (this file) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the directory-tree fence.
markdownlint flags this unlabeled code fence; text is sufficient for the directory listing.
🔧 Proposed fix
-```
+```text
data/Slicer-Heart-CT/
├── TruncalValve_4DCT.seq.nrrd
└── README.md (this file)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
**Directory structure after download:**
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/Slicer-Heart-CT/README.md` around lines 25 - 30, The README’s directory
listing uses an unlabeled fenced block, which triggers markdownlint; update the
fenced block under the directory structure section to use a text label instead
of an anonymous fence. Keep the content the same and adjust the markdown in
README.md so the tree listing is explicitly marked as text.
Source: Linters/SAST tools
Comment on lines
+30
to
+32
| ```bash | ||
| py -m pytest tests/ -v | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the pytest command fence.
This unlabeled fence should be marked bash so markdownlint passes.
🔧 Proposed fix
-```
+```bash
py -m pytest tests/ -v</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/test/README.md` around lines 30 - 32, The Markdown code fence in the
test command example is missing its language label, which causes markdownlint to
fail. Update the fenced block in the README example to use the bash label,
keeping the existing pytest command content unchanged. Use the nearby README
test command snippet as the target location.
Source: Linters/SAST tools
Comment on lines
+155
to
+167
| with tarfile.open(archive_file) as tar: | ||
| try: | ||
| tar.extract(member_name, path=tmp_dir, filter="data") | ||
| except TypeError: | ||
| # Python < 3.12 (without the PEP 706 backport) does not | ||
| # accept the ``filter`` keyword argument. | ||
| tar.extract(member_name, path=tmp_dir) | ||
| extracted_file = tmp_dir / member_name | ||
| if not extracted_file.is_file(): | ||
| raise RuntimeError( | ||
| f"Expected member {member_name!r} not found in archive: {url}" | ||
| ) | ||
| extracted_file.replace(target_file) |
Comment on lines
+252
to
+253
| with zipfile.ZipFile(archive_file) as archive: | ||
| archive.extractall(target_dir) |
Comment on lines
+190
to
+194
| CT volume and Simpleware segmentation). A subdirectory that already | ||
| has its expected files (see ``_CHOPValve4DSubdirIsPopulated``) is | ||
| left alone, so re-running resumes an interrupted download without | ||
| re-fetching a subdirectory a partial extraction left behind. See | ||
| ``data/CHOP-Valve4D/README.md`` for what this converted data |
Comment on lines
+206
to
+207
| if DataDownloadTools._CHOPValve4DSubdirIsPopulated(subdir_name, target_dir): | ||
| continue |
Comment on lines
+264
to
+266
| has_ct = DataDownloadTools._CHOPValve4DSubdirIsPopulated("CT", data_dir / "CT") | ||
| has_alterra = DataDownloadTools._CHOPValve4DSubdirIsPopulated( | ||
| "Alterra", data_dir / "Alterra" |
Comment on lines
+223
to
+224
| def _write_chop_valve4d_expected_marker(target_dir: Path, subdir_name: str) -> None: | ||
| """Create the marker file/dir _CHOPValve4DSubdirIsPopulated looks for.""" |
Comment on lines
+166
to
+168
| urls_to_archives[DataDownloadTools.KCL_HEART_MODEL_AVERAGE_URL] = make_archive( | ||
| "average.vtk", b"# vtk average\n" | ||
| ) |
| urls_to_archives = {} | ||
| for subdir_name, asset_name in DataDownloadTools.CHOP_VALVE4D_ASSETS.items(): | ||
| url = DataDownloadTools.CHOP_VALVE4D_RELEASE_URL + asset_name | ||
| member_name = "RVOT28-Dias.mha" if subdir_name == "CT" else "frame_0000.vtk" |
Comment on lines
+91
to
+94
| The command uses | ||
| :meth:`physiotwin4d.data_download_tools.DataDownloadTools.DownloadCHOPValve4DData`, | ||
| which fetches each subdirectory's zip archive from the PhysioTwin4D GitHub | ||
| release and skips any subdirectory that already contains files. |
Comment on lines
+30
to
+31
| A subdirectory that already contains files is left alone, so re-running the | ||
| command resumes an interrupted download. |
Comment on lines
+252
to
+253
| with zipfile.ZipFile(archive_file) as archive: | ||
| archive.extractall(target_dir) |
Comment on lines
+155
to
+166
| with tarfile.open(archive_file) as tar: | ||
| try: | ||
| tar.extract(member_name, path=tmp_dir, filter="data") | ||
| except TypeError: | ||
| # Python < 3.12 (without the PEP 706 backport) does not | ||
| # accept the ``filter`` keyword argument. | ||
| tar.extract(member_name, path=tmp_dir) | ||
| extracted_file = tmp_dir / member_name | ||
| if not extracted_file.is_file(): | ||
| raise RuntimeError( | ||
| f"Expected member {member_name!r} not found in archive: {url}" | ||
| ) |
Comment on lines
+30
to
+31
| A subdirectory that already contains files is left alone, so re-running the | ||
| command resumes an interrupted download. |
| assert extracted_file.read_text() == f"# {subdir_name}\n" | ||
|
|
||
| @staticmethod | ||
| def _write_chop_valve4d_expected_marker(target_dir: Path, subdir_name: str) -> None: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/physiotwin4d/data_download_tools.py (1)
233-253: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
_DownloadAndExtractZipstill extracts non-atomically and without path validation.The past review (marked addressed in 0ccd5f1) improved skip logic via
_CHOPValve4DSubdirIsPopulated, but the extraction itself was not fixed.archive.extractall(target_dir)at line 253 still:
- Non-atomic extraction — writes directly to
target_dir. If extraction fails partway, partial files remain. On re-run,_CHOPValve4DSubdirIsPopulatedmay return True (CT ifRVOT28-Dias.nii.gzwas extracted; Alterra/TPV25 if any single.vtkwas extracted), permanently skipping re-download.- Zip Slip —
extractallperforms no member-path validation. A compromised release asset with../../entries could write outsidetarget_dir.
_DownloadAndExtractTarMemberalready demonstrates the correct pattern: extract to temp, validate, atomically replace. The test attest_download_heart_data.pyline 294 (assert (partial_dir / "partial_download.tmp").exists()) confirms this behavior — it would need to flip tonot .exists()after the fix.🔒 Proposed fix: atomic + safe zip extraction
`@staticmethod` def _DownloadAndExtractZip(url: str, target_dir: Path) -> None: # noqa: N802 """Stream-download a ``.zip`` archive and extract it into ``target_dir``.""" - target_dir.mkdir(parents=True, exist_ok=True) + target_dir.parent.mkdir(parents=True, exist_ok=True) with tempfile.TemporaryDirectory(dir=str(target_dir.parent)) as tmp_dir_name: archive_file = Path(tmp_dir_name) / "archive.zip" with ( urllib.request.urlopen( # noqa: S310 url, timeout=_DOWNLOAD_TIMEOUT_SECONDS ) as response, open(archive_file, "wb") as out, ): shutil.copyfileobj(response, out, length=1024 * 1024) if archive_file.stat().st_size == 0: raise RuntimeError(f"Downloaded archive is empty: {url}") - with zipfile.ZipFile(archive_file) as archive: - archive.extractall(target_dir) + extract_dir = Path(tmp_dir_name) / "extracted" + extract_dir.mkdir() + extract_root = extract_dir.resolve() + with zipfile.ZipFile(archive_file) as archive: + for member in archive.namelist(): + resolved = (extract_dir / member).resolve() + if extract_root not in resolved.parents: + raise RuntimeError( + f"Unsafe path in zip archive: {member}" + ) + archive.extractall(extract_dir) + if target_dir.exists(): + shutil.rmtree(target_dir) + extract_dir.rename(target_dir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/data_download_tools.py` around lines 233 - 253, The `_DownloadAndExtractZip` flow still writes directly into `target_dir` via `zipfile.ZipFile.extractall`, so update it to match the atomic-safe pattern used by `_DownloadAndExtractTarMember`: extract into a temporary directory first, validate each zip member path stays within the extraction root to prevent Zip Slip, then atomically replace the final target on success. Keep the fix centered around `_DownloadAndExtractZip` and ensure `_CHOPValve4DSubdirIsPopulated` cannot see partially extracted content on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/physiotwin4d/data_download_tools.py`:
- Around line 233-253: The `_DownloadAndExtractZip` flow still writes directly
into `target_dir` via `zipfile.ZipFile.extractall`, so update it to match the
atomic-safe pattern used by `_DownloadAndExtractTarMember`: extract into a
temporary directory first, validate each zip member path stays within the
extraction root to prevent Zip Slip, then atomically replace the final target on
success. Keep the fix centered around `_DownloadAndExtractZip` and ensure
`_CHOPValve4DSubdirIsPopulated` cannot see partially extracted content on
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d41d74ee-eaa6-4d28-8aab-21d44b61240d
📒 Files selected for processing (6)
docs/cli_scripts/4dct_reconstruction.rstdocs/cli_scripts/download_data.rstdocs/quickstart.rstdocs/tutorials.rstsrc/physiotwin4d/data_download_tools.pytests/test_download_heart_data.py
✅ Files skipped from review due to trivial changes (3)
- docs/cli_scripts/4dct_reconstruction.rst
- docs/cli_scripts/download_data.rst
- docs/tutorials.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend DataDownloadTools/physiotwin4d-download-data with automatic downloaders for KCL-Heart-Model (Zenodo) and CHOP-Valve4D (the three CHOP-Valve4D-{Alterra,CT,TPV25}.zip assets on the 2026.07.1 GitHub release, extracted into matching subdirs). Both are idempotent/resumable and covered by unit tests.
Replace the monolithic data/README.md with a short top-level overview table, and give each dataset its own README (Slicer-Heart-CT, DirLab-4DCT, KCL-Heart-Model, CHOP-Valve4D, plus the pytest-only data/test cache) documenting the download command, source URL(s), directory layout, sizes, and a dedicated Citation section. Fold the content of citation.txt/citations.txt/reading_img_files.url into those Citation sections and remove the now-redundant files.
Commit the DirLab-4DCT .mhd MetaImage headers (metadata only, a few hundred bytes each) so the expected case/phase layout is documented in-repo; .gitignore now excludes *.mha instead of *.mhd so the large raw volumes stay untracked. Update tutorials/experiments docs to reflect KCL-Heart-Model's new auto-download path.
Summary by CodeRabbit