Conversation
WalkthroughRemoved VISTA‑3D segmentation code and tests, updated ensemble/workflow to rely on TotalSegmentator, added Codecov tokens to CI uploads, revised numerous docs and API map entries to remove VISTA‑3D references, changed a test data URL and fixture error handling, and added Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 tightens up the project’s testing and CI plumbing by improving external test-data download behavior in CI, ensuring Codecov uploads include the repo token, and aligning documentation badges with the nightly health workflow.
Changes:
- Update the TruncalValve test-data download URL and fail (instead of skip) on download HTTP errors when running in CI.
- Add
CODECOV_TOKENto Codecov upload steps in CI and slow-test workflows. - Update docs CI badge to point at the
nightly-health.ymlworkflow and refresh the generated API map.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/conftest.py |
Switches test-data download URL and changes CI behavior to fail on download HTTP errors. |
docs/index.rst |
Updates the CI badge to reflect the nightly health workflow. |
docs/API_MAP.md |
Regenerates API map line numbers after code changes. |
.github/workflows/test-slow.yml |
Adds Codecov token to coverage upload. |
.github/workflows/ci.yml |
Adds Codecov token to coverage uploads across unit/integration/GPU jobs. |
.claude/.gitignore |
Ignores local Claude settings file. |
💡 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 297-305: The except block currently catches urllib.error.HTTPError
only, so network/DNS failures (urllib.error.URLError) bypass the CI/local
fail-or-skip logic; change the handler to catch urllib.error.URLError (or both
urllib.error.HTTPError and urllib.error.URLError) instead of only HTTPError so
that the same pytest.fail/pytest.skip behavior (using data_dir) runs for
network-level errors; update the except clause where urllib.error.HTTPError is
referenced to use urllib.error.URLError (or a tuple) and keep the existing msg,
pytest.fail and pytest.skip calls unchanged.
🪄 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: a5a7fc0d-8da7-480b-9770-0a4561357932
📒 Files selected for processing (6)
.claude/.gitignore.github/workflows/ci.yml.github/workflows/test-slow.ymldocs/API_MAP.mddocs/index.rsttests/conftest.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
=======================================
Coverage ? 13.98%
=======================================
Files ? 47
Lines ? 6409
Branches ? 0
=======================================
Hits ? 896
Misses ? 5513
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| urllib.request.urlretrieve(input_image_url, str(input_image_filename)) | ||
| print(f"Downloaded to {input_image_filename}") | ||
| except urllib.error.HTTPError as e: | ||
| pytest.skip( | ||
| f"Could not download test data: {e}. Please manually place TruncalValve_4DCT.seq.nrrd in {data_dir}" | ||
| except urllib.error.URLError as e: | ||
| msg = ( | ||
| f"Could not download test data: {e}. " |
There was a problem hiding this comment.
The except urllib.error.URLError clause will fail if triggered because urllib.error isn’t imported (only urllib.request is). Import urllib.error (or from urllib.error import URLError) so download failures are handled as intended.
| **Segmentation Classes**: | ||
| * :doc:`base` - Base class for all segmentation methods | ||
| * :doc:`totalsegmentator` - TotalSegmentator implementation | ||
| * :doc:`vista3d` - VISTA-3D foundation model | ||
| * :doc:`vista3d_nim` - VISTA-3D NIM for cloud deployment | ||
| * :doc:`ensemble` - Ensemble segmentation |
There was a problem hiding this comment.
This PR removes the vista3d/vista3d_nim pages from the segmentation docs, but other docs in the repo still reference :doc:vista3d`` (e.g., docs/api/segmentation/base.rst and `docs/api/segmentation/totalsegmentator.rst`) and `docs/api/workflows.rst` mentions `segmentation_method="vista3d"`. Those references should be updated (or stub pages kept) to avoid broken Sphinx links.
| from physiomotion4d import SegmentChestTotalSegmentator | ||
| import itk | ||
|
|
||
| # Initialize VISTA-3D segmentation | ||
| segmenter = SegmentChestVista3D() | ||
| # Initialize TotalSegmentator segmentation | ||
| segmenter = SegmentChestTotalSegmentator() | ||
|
|
||
| # Load and segment image | ||
| image = itk.imread("chest_ct.nrrd") |
There was a problem hiding this comment.
In this example, SegmentChestTotalSegmentator.segment() returns a dict of masks (e.g., result['labelmap'], result['heart']). The snippet below currently assigns to masks = ... and then unpacks multiple values; after switching to TotalSegmentator the example should be updated to use the dict keys instead of tuple unpacking.
| from physiomotion4d import SegmentChestTotalSegmentator | ||
| import itk | ||
|
|
||
| # Initialize segmenter | ||
| segmenter = SegmentChestVista3D() | ||
| segmenter = SegmentChestTotalSegmentator() | ||
|
|
||
| # Load and segment image | ||
| image = itk.imread("chest_ct.nrrd") |
There was a problem hiding this comment.
SegmentChestTotalSegmentator.segment() returns a dict of masks, but this section’s code block unpacks the return value into multiple variables. After updating the import/constructor here, please also update the subsequent lines to use keys like result['heart'] / result['labelmap'] rather than tuple unpacking.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/developer/segmentation.rst (1)
104-128: Single-method ensemble example may be misleading.The ensemble documentation states it "combines multiple segmentation methods" (line 105), but the example uses only
methods=['totalsegmentator']. This inconsistency could confuse developers.Recommendation: Either update the feature description to note that it currently uses a single backend, or remove the
methodsparameter from the example sinceSegmentChestEnsemblenow directly inherits and delegates toSegmentChestTotalSegmentator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/developer/segmentation.rst` around lines 104 - 128, The docs example is inconsistent: SegmentChestEnsemble claims to combine multiple methods but the example passes methods=['totalsegmentator']; update the documentation to remove or clarify the misleading parameter. Edit the example usage for SegmentChestEnsemble to either (A) remove the methods argument and show direct instantiation (since SegmentChestEnsemble delegates to SegmentChestTotalSegmentator), or (B) change the feature text to state it currently uses a single backend and keep the example; reference SegmentChestEnsemble, methods, and SegmentChestTotalSegmentator when making the change.docs/examples.rst (1)
106-109: Single-method ensemble may confuse users.The ensemble example now uses only
methods=['totalsegmentator'], which technically isn't an ensemble (ensembles combine multiple methods). While this aligns with the VISTA-3D removal, users may find it confusing to use an "ensemble" class with a single method.Consider either:
- Updating the example text to clarify that
SegmentChestEnsemblecurrently delegates to a single backend but maintains API stability- Recommending users call
SegmentChestTotalSegmentatordirectly for single-method segmentation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/examples.rst` around lines 106 - 109, Update the example to avoid confusion about using an "ensemble" with a single method: in the docs surrounding SegmentChestEnsemble mention that when methods=['totalsegmentator'] the class currently delegates to a single backend for API stability, and add a brief note recommending using SegmentChestTotalSegmentator directly for single-method cases; reference the SegmentChestEnsemble constructor (methods and fusion_strategy parameters) and the alternative SegmentChestTotalSegmentator symbol so readers can choose the clearer direct class for single-method segmentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/developer/segmentation.rst`:
- Around line 104-128: The docs example is inconsistent: SegmentChestEnsemble
claims to combine multiple methods but the example passes
methods=['totalsegmentator']; update the documentation to remove or clarify the
misleading parameter. Edit the example usage for SegmentChestEnsemble to either
(A) remove the methods argument and show direct instantiation (since
SegmentChestEnsemble delegates to SegmentChestTotalSegmentator), or (B) change
the feature text to state it currently uses a single backend and keep the
example; reference SegmentChestEnsemble, methods, and
SegmentChestTotalSegmentator when making the change.
In `@docs/examples.rst`:
- Around line 106-109: Update the example to avoid confusion about using an
"ensemble" with a single method: in the docs surrounding SegmentChestEnsemble
mention that when methods=['totalsegmentator'] the class currently delegates to
a single backend for API stability, and add a brief note recommending using
SegmentChestTotalSegmentator directly for single-method cases; reference the
SegmentChestEnsemble constructor (methods and fusion_strategy parameters) and
the alternative SegmentChestTotalSegmentator symbol so readers can choose the
clearer direct class for single-method segmentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62bbd42b-6451-4a24-a998-c4c46dfae4d4
📒 Files selected for processing (31)
.github/workflows/ci.ymlCLAUDE.mdREADME.mddocs/API_MAP.mddocs/README.mddocs/api/index.rstdocs/api/segmentation/ensemble.rstdocs/api/segmentation/index.rstdocs/api/segmentation/vista3d.rstdocs/api/segmentation/vista3d_nim.rstdocs/developer/architecture.rstdocs/developer/segmentation.rstdocs/examples.rstdocs/quickstart.rstdocs/testing.rstexperiments/Heart-GatedCT_To_USD/test_vista3d_class.pyexperiments/Heart-GatedCT_To_USD/test_vista3d_inMem.pyexperiments/Lung-GatedCT_To_USD/0-register_dirlab_4dct.pyexperiments/Lung-GatedCT_To_USD/Experiment_SegReg.pyexperiments/README.mdpyproject.tomlsrc/physiomotion4d/__init__.pysrc/physiomotion4d/cli/convert_ct_to_vtk.pysrc/physiomotion4d/segment_chest_ensemble.pysrc/physiomotion4d/workflow_convert_ct_to_vtk.pytests/GITHUB_WORKFLOWS.mdtests/README.mdtests/TESTING_GUIDE.mdtests/conftest.pytests/test_experiments.pytests/test_segment_chest_vista_3d.py
💤 Files with no reviewable changes (11)
- CLAUDE.md
- experiments/Lung-GatedCT_To_USD/0-register_dirlab_4dct.py
- tests/test_experiments.py
- docs/api/segmentation/vista3d_nim.rst
- docs/api/segmentation/vista3d.rst
- experiments/Lung-GatedCT_To_USD/Experiment_SegReg.py
- pyproject.toml
- experiments/Heart-GatedCT_To_USD/test_vista3d_class.py
- src/physiomotion4d/init.py
- tests/test_segment_chest_vista_3d.py
- experiments/Heart-GatedCT_To_USD/test_vista3d_inMem.py
✅ Files skipped from review due to trivial changes (4)
- src/physiomotion4d/cli/convert_ct_to_vtk.py
- experiments/README.md
- docs/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
Summary by CodeRabbit
Documentation
Chores
Refactor
Tests