ENH: HeartSimplewareTrimmedBranches + opt-in test marker overhaul#53
Merged
Merged
Conversation
Default method for work involving KCL heart data
WalkthroughAdds a HeartSimplewareTrimmedBranches segmentation backend, extracts/uses a _trim_branches flag in SegmentHeartSimpleware, conditions trimming in image-to-VTK/USD workflows and CLIs, changes WorkflowFitStatisticalModelToPatient’s default to the trimmed backend, and migrates test selection to opt-in --run-* flags with marker and documentation updates. ChangesHeart Simpleware Trimmed-Branch Segmentation Backend
Estimated code review effort: 🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/physiomotion4d/workflow_fit_statistical_model_to_patient.py`:
- Line 140: Update tests to cover the new default segmentation contract:
add/modify unit tests for WorkflowFitStatisticalModelToPatient to assert that
when segmentation_method is omitted it selects the new default backend
(segmentation_method default = "HeartSimplewareTrimmedBranches") and that the
routing logic invokes WorkflowConvertImageToVTK with the
HeartSimplewareTrimmedBranches path; also add a test exercising
WorkflowConvertImageToVTK handling for
segmentation_method="HeartSimplewareTrimmedBranches" (happy-path and any
expected side-effects/outputs) so the change in public API is covered. Locate
usages of WorkflowFitStatisticalModelToPatient and WorkflowConvertImageToVTK in
existing test modules and update or add assertions to verify the default
selection and the HeartSimplewareTrimmedBranches code path.
🪄 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: a2ed8885-6c36-44cc-bfa4-57690438b13e
📒 Files selected for processing (8)
docs/API_MAP.mdexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.pysrc/physiomotion4d/cli/convert_image_to_usd.pysrc/physiomotion4d/cli/convert_image_to_vtk.pysrc/physiomotion4d/segment_heart_simpleware.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_convert_image_to_vtk.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.py
| patient_models: list[pv.DataSet] | None = None, | ||
| patient_image: Optional[itk.Image] = None, | ||
| segmentation_method: str = "HeartSimpleware", | ||
| segmentation_method: str = "HeartSimplewareTrimmedBranches", |
There was a problem hiding this comment.
Add regression coverage for the new default segmentation contract.
This changes public API behavior (segmentation_method default and routing), but corresponding tests are not updated in this diff. Please add coverage for: (1) default backend selection in WorkflowFitStatisticalModelToPatient, and (2) the HeartSimplewareTrimmedBranches path through WorkflowConvertImageToVTK. As per coding guidelines, "Update existing tests when changing APIs in Python code".
Also applies to: 154-159
🤖 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/physiomotion4d/workflow_fit_statistical_model_to_patient.py` at line 140,
Update tests to cover the new default segmentation contract: add/modify unit
tests for WorkflowFitStatisticalModelToPatient to assert that when
segmentation_method is omitted it selects the new default backend
(segmentation_method default = "HeartSimplewareTrimmedBranches") and that the
routing logic invokes WorkflowConvertImageToVTK with the
HeartSimplewareTrimmedBranches path; also add a test exercising
WorkflowConvertImageToVTK handling for
segmentation_method="HeartSimplewareTrimmedBranches" (happy-path and any
expected side-effects/outputs) so the change in public API is covered. Locate
usages of WorkflowFitStatisticalModelToPatient and WorkflowConvertImageToVTK in
existing test modules and update or add assertions to verify the default
selection and the HeartSimplewareTrimmedBranches code path.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new segmentation option (HeartSimplewareTrimmedBranches) across the image→VTK/USD workflows and CLIs to make Simpleware branch trimming explicitly opt-in, while updating model-to-patient fitting to default to the trimmed-branches behavior to better match KCL-derived template extents.
Changes:
- Add
HeartSimplewareTrimmedBranchessegmentation method toWorkflowConvertImageToVTK,WorkflowConvertImageToUSD, and both convert-image CLIs, wiring it toSegmentHeartSimpleware.set_trim_branches(True). - Change
WorkflowFitStatisticalModelToPatientdefault segmentation method toHeartSimplewareTrimmedBranchesand update the CHOP-Valve experiment accordingly. - Rename internal
SegmentHeartSimplewarestate from_trim_maskto_trim_branchesto match the public API.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/physiomotion4d/workflow_fit_statistical_model_to_patient.py | Switch default segmentation method to trimmed-branches and update docstring options. |
| src/physiomotion4d/workflow_convert_image_to_vtk.py | Add new segmentation option and make branch trimming conditional on method name. |
| src/physiomotion4d/workflow_convert_image_to_usd.py | Add new segmentation option and make branch trimming conditional on method name (with validation). |
| src/physiomotion4d/segment_heart_simpleware.py | Rename internal flag to _trim_branches and update usage sites. |
| src/physiomotion4d/cli/convert_image_to_vtk.py | Expose new segmentation choice and document it in CLI help. |
| src/physiomotion4d/cli/convert_image_to_usd.py | Expose new segmentation choice and document it in CLI help. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py | Update experiment to use the new trimmed-branches segmentation method. |
| docs/API_MAP.md | Refresh generated API map line references affected by code movement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
135
to
141
| def __init__( | ||
| self, | ||
| template_model: pv.DataSet, | ||
| patient_models: list[pv.DataSet] | None = None, | ||
| patient_image: Optional[itk.Image] = None, | ||
| segmentation_method: str = "HeartSimpleware", | ||
| segmentation_method: str = "HeartSimplewareTrimmedBranches", | ||
| log_level: int | str = logging.INFO, |
Comment on lines
+93
to
+94
| "HeartSimpleware | HeartSimplewareTrimmedBranches " | ||
| "(HeartSimpleware with pulmonary/great-vessel branches trimmed " |
Comment on lines
+80
to
+81
| "HeartSimpleware, or HeartSimplewareTrimmedBranches " | ||
| "(HeartSimpleware with pulmonary/great-vessel branches trimmed " |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 29.26% 30.62% +1.35%
==========================================
Files 49 49
Lines 6806 6808 +2
==========================================
+ Hits 1992 2085 +93
+ Misses 4814 4723 -91
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:
|
- Add --run-slow, --run-gpu, --run-simpleware pytest flags;
collection-hook auto-skips the matching markers unless the flag is
passed. Mirrors the pre-existing --run-experiments / --run-tutorials
pattern.
- Retire the requires_data marker: 29 decorator lines removed across 17
test files. Tests that need downloadable data request the session
fixtures (test_directories, download_test_data, test_images) and the
data is fetched on first use.
- Mark TestRegisterImagesICON, TestSegmentHeartSimpleware, and
TestSegmentChestTotalSegmentator with requires_gpu so they fall into
the new --run-gpu bucket.
- Update self-hosted GPU workflows (ci.yml gpu-tests, test-slow.yml,
nightly-health.yml) to pass every opt-in flag
(--run-gpu --run-slow --run-simpleware --run-experiments
--run-tutorials). Tests whose host requirements aren't met (e.g. a
licensed Simpleware install) runtime-skip cleanly.
- Add three unit tests to test_workflow_fit_statistical_model_to_patient
covering the new HeartSimplewareTrimmedBranches default and the
WorkflowConvertImageToVTK dispatch path.
- Update documentation throughout (CLAUDE.md, AGENTS.md, README.md,
tests/README.md, docs/testing.rst, docs/contributing.rst,
docs/developer/{core,extending,segmentation}.rst,
docs/api/segmentation/base.rst, .agents/agents/testing.md,
.agents/skills/test-feature/SKILL.md). The test-feature skill and
AGENTS.md testing role now strongly prefer real (downloaded) test
data with baseline comparison over synthetic data.
Address PR Project-MONAI#53 review feedback: - Add a "Behavior change" note to the WorkflowConvertImageToVTK and WorkflowConvertImageToUSD class docstrings warning that the 'HeartSimpleware' option no longer implicitly trims branches; callers that want trimming should now select 'HeartSimplewareTrimmedBranches'. - Replace the silent else: branch in WorkflowConvertImageToUSD.__init__ with an explicit elif self.segmentation_method in (...) plus else: raise ValueError(...), matching the dispatch shape in WorkflowConvertImageToVTK._create_segmenter so a future fourth option cannot silently behave like HeartSimpleware. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aylward
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tests/conftest.py`:
- Around line 49-69: Add registrations for the missing pytest markers inside the
pytest_configure function: call config.addinivalue_line for "slow" and for
"requires_gpu" (similar to existing registrations for "requires_simpleware",
"experiment", and "tutorial") so pytest stops warning about unregistered
markers; update the marker descriptions to briefly explain each marker's purpose
and ensure the symbol names "slow" and "requires_gpu" match those referenced in
the auto-skip logic.
In `@tests/README.md`:
- Around line 153-165: Update the README heading and entries to avoid implying
every marker is an opt-in flag: change the heading from "Test Markers (all
opt-in via a matching `--run-*` flag)" to something like "Test Markers (most are
opt-in via a matching `--run-*` flag)"; keep the opt-in language for
`@pytest.mark.slow`, `@pytest.mark.requires_gpu`,
`@pytest.mark.requires_simpleware`, `@pytest.mark.experiment`, and
`@pytest.mark.tutorial` but explicitly mark `@pytest.mark.integration` as a
classification/filter-only marker and `@pytest.mark.timeout(seconds)` as a
per-test configuration marker (not opt-in), referencing those symbols so readers
understand they are not tied to `--run-*` flags.
In `@tests/test_workflow_fit_statistical_model_to_patient.py`:
- Around line 96-178: Update the three test docstrings
(test_fit_workflow_default_segmentation_method_is_trimmed_branches,
test_fit_workflow_routes_default_to_image_to_vtk_with_trimmed_branches,
test_image_to_vtk_segmenter_dispatch_for_trimmed_branches) to include the
synthetic image metadata: state the image shape and axis order (for example
"shape (X, Y, Z, T) = (3, 3, 3, 1), LPS world frame") and any relevant
world/frame assumptions so readers know the test image dimensions and axis
ordering; add this brief metadata sentence to each test's docstring directly
above the test body.
🪄 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: 11961af1-37f5-4fe7-9056-e5b0374afe7e
📒 Files selected for processing (44)
.agents/agents/testing.md.agents/skills/test-feature/SKILL.md.github/workflows/ci.yml.github/workflows/nightly-health.yml.github/workflows/test-slow.ymlAGENTS.mdCLAUDE.mdREADME.mddocs/API_MAP.mddocs/api/segmentation/base.rstdocs/contributing.rstdocs/developer/core.rstdocs/developer/extending.rstdocs/developer/segmentation.rstdocs/testing.rstpyproject.tomlsrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_convert_image_to_vtk.pytests/EXPERIMENT_FLAG_USAGE.mdtests/EXPERIMENT_TESTS_GUIDE.mdtests/EXPERIMENT_TESTS_SUMMARY.mdtests/GITHUB_WORKFLOWS.mdtests/PARALLEL_EXECUTION_GUIDE.mdtests/README.mdtests/TESTING_GUIDE.mdtests/conftest.pytests/test_contour_tools.pytests/test_convert_image_4d_to_3d.pytests/test_convert_vtk_to_usd.pytests/test_download_heart_data.pytests/test_experiments.pytests/test_image_tools.pytests/test_register_images_ants.pytests/test_register_images_greedy.pytests/test_register_images_icon.pytests/test_register_time_series_images.pytests/test_segment_chest_total_segmentator.pytests/test_segment_heart_simpleware.pytests/test_transform_tools.pytests/test_tutorials.pytests/test_usd_merge.pytests/test_usd_time_preservation.pytests/test_vtk_to_usd_library.pytests/test_workflow_fit_statistical_model_to_patient.py
💤 Files with no reviewable changes (20)
- tests/TESTING_GUIDE.md
- tests/EXPERIMENT_TESTS_GUIDE.md
- tests/GITHUB_WORKFLOWS.md
- tests/EXPERIMENT_FLAG_USAGE.md
- tests/EXPERIMENT_TESTS_SUMMARY.md
- tests/PARALLEL_EXECUTION_GUIDE.md
- tests/test_convert_image_4d_to_3d.py
- tests/test_contour_tools.py
- tests/test_download_heart_data.py
- tests/test_usd_time_preservation.py
- tests/test_register_time_series_images.py
- tests/test_register_images_greedy.py
- tests/test_image_tools.py
- tests/test_vtk_to_usd_library.py
- tests/test_convert_vtk_to_usd.py
- tests/test_transform_tools.py
- tests/test_usd_merge.py
- tests/test_register_images_ants.py
- tests/test_tutorials.py
- tests/test_experiments.py
✅ Files skipped from review due to trivial changes (7)
- docs/developer/segmentation.rst
- docs/api/segmentation/base.rst
- CLAUDE.md
- .agents/skills/test-feature/SKILL.md
- AGENTS.md
- docs/developer/extending.rst
- .agents/agents/testing.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/physiomotion4d/workflow_convert_image_to_vtk.py
- src/physiomotion4d/workflow_convert_image_to_usd.py
Comment on lines
+49
to
+69
| parser.addoption( | ||
| "--run-simpleware", | ||
| action="store_true", | ||
| default=False, | ||
| help=( | ||
| "Run tests that require a local Synopsys Simpleware Medical " | ||
| "installation (ASCardio module)" | ||
| ), | ||
| ) | ||
| parser.addoption( | ||
| "--run-slow", | ||
| action="store_true", | ||
| default=False, | ||
| help="Run tests marked 'slow' (skipped by default)", | ||
| ) | ||
| parser.addoption( | ||
| "--run-gpu", | ||
| action="store_true", | ||
| default=False, | ||
| help="Run tests marked 'requires_gpu' (skipped by default)", | ||
| ) |
There was a problem hiding this comment.
Register the slow and requires_gpu markers.
The auto-skip logic at lines 144-155 references slow and requires_gpu markers, but these markers are not registered via config.addinivalue_line() like requires_simpleware (lines 97-101), experiment, and tutorial (lines 89-96). Pytest will emit warnings about unregistered markers.
📝 Proposed fix to register missing markers
config.addinivalue_line(
"markers",
"requires_simpleware: marks tests that need a local Synopsys Simpleware "
"Medical installation (skipped unless --run-simpleware is passed)",
)
+ config.addinivalue_line(
+ "markers",
+ "slow: marks tests that take significant time to run "
+ "(skipped unless --run-slow is passed)",
+ )
+ config.addinivalue_line(
+ "markers",
+ "requires_gpu: marks tests that require GPU hardware "
+ "(skipped unless --run-gpu is passed)",
+ )
# Initialize test timing storage📝 Committable suggestion
‼️ 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.
Suggested change
| parser.addoption( | |
| "--run-simpleware", | |
| action="store_true", | |
| default=False, | |
| help=( | |
| "Run tests that require a local Synopsys Simpleware Medical " | |
| "installation (ASCardio module)" | |
| ), | |
| ) | |
| parser.addoption( | |
| "--run-slow", | |
| action="store_true", | |
| default=False, | |
| help="Run tests marked 'slow' (skipped by default)", | |
| ) | |
| parser.addoption( | |
| "--run-gpu", | |
| action="store_true", | |
| default=False, | |
| help="Run tests marked 'requires_gpu' (skipped by default)", | |
| ) | |
| config.addinivalue_line( | |
| "markers", | |
| "requires_simpleware: marks tests that need a local Synopsys Simpleware " | |
| "Medical installation (skipped unless --run-simpleware is passed)", | |
| ) | |
| config.addinivalue_line( | |
| "markers", | |
| "slow: marks tests that take significant time to run " | |
| "(skipped unless --run-slow is passed)", | |
| ) | |
| config.addinivalue_line( | |
| "markers", | |
| "requires_gpu: marks tests that require GPU hardware " | |
| "(skipped unless --run-gpu is passed)", | |
| ) | |
| # Initialize test timing storage |
🤖 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/conftest.py` around lines 49 - 69, Add registrations for the missing
pytest markers inside the pytest_configure function: call
config.addinivalue_line for "slow" and for "requires_gpu" (similar to existing
registrations for "requires_simpleware", "experiment", and "tutorial") so pytest
stops warning about unregistered markers; update the marker descriptions to
briefly explain each marker's purpose and ensure the symbol names "slow" and
"requires_gpu" match those referenced in the auto-skip logic.
Comment on lines
+153
to
+165
| ### Test Markers (all opt-in via a matching `--run-*` flag) | ||
| - `@pytest.mark.slow` — Tests taking >30 seconds. Opt in: `--run-slow`. | ||
| - `@pytest.mark.requires_gpu` — Tests needing CUDA. Opt in: `--run-gpu`. | ||
| - `@pytest.mark.requires_simpleware` — Tests needing a licensed Synopsys | ||
| Simpleware Medical install. Opt in: `--run-simpleware`. (Combine with | ||
| `--run-gpu` and `--run-slow`.) | ||
| - `@pytest.mark.experiment` — End-to-end experiment notebooks (EXTREMELY | ||
| SLOW, never in CI). Opt in: `--run-experiments`. | ||
| - `@pytest.mark.tutorial` — Tutorial scripts run end-to-end. Opt in: | ||
| `--run-tutorials`. | ||
| - `@pytest.mark.integration` — Integration tests vs unit tests (filter-only). | ||
| - `@pytest.mark.timeout(seconds)` — Per-test timeout override. | ||
|
|
There was a problem hiding this comment.
Clarify marker wording: not all listed markers are opt-in buckets.
The heading says all markers map to --run-* flags, but integration and timeout are classification/configuration markers.
📝 Suggested wording fix
-### Test Markers (all opt-in via a matching `--run-*` flag)
+### Test Markers
@@
- `@pytest.mark.integration` — Integration tests vs unit tests (filter-only).
- `@pytest.mark.timeout(seconds)` — Per-test timeout override.
+
+Only `slow`, `requires_gpu`, `requires_simpleware`, `experiment`, and `tutorial`
+are opt-in buckets controlled by `--run-*` flags.🤖 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/README.md` around lines 153 - 165, Update the README heading and
entries to avoid implying every marker is an opt-in flag: change the heading
from "Test Markers (all opt-in via a matching `--run-*` flag)" to something like
"Test Markers (most are opt-in via a matching `--run-*` flag)"; keep the opt-in
language for `@pytest.mark.slow`, `@pytest.mark.requires_gpu`,
`@pytest.mark.requires_simpleware`, `@pytest.mark.experiment`, and
`@pytest.mark.tutorial` but explicitly mark `@pytest.mark.integration` as a
classification/filter-only marker and `@pytest.mark.timeout(seconds)` as a
per-test configuration marker (not opt-in), referencing those symbols so readers
understand they are not tied to `--run-*` flags.
Comment on lines
+96
to
+178
| def test_fit_workflow_default_segmentation_method_is_trimmed_branches() -> None: | ||
| """Default segmentation_method must match the KCL-Heart-Model fit contract.""" | ||
| default = ( | ||
| inspect.signature(WorkflowFitStatisticalModelToPatient.__init__) | ||
| .parameters["segmentation_method"] | ||
| .default | ||
| ) | ||
| assert default == "HeartSimplewareTrimmedBranches" | ||
|
|
||
|
|
||
| def test_fit_workflow_routes_default_to_image_to_vtk_with_trimmed_branches( | ||
| monkeypatch: Any, | ||
| ) -> None: | ||
| """When patient_models is omitted, the workflow must invoke | ||
| WorkflowConvertImageToVTK with segmentation_method='HeartSimplewareTrimmedBranches'.""" | ||
| image = itk.image_from_array(np.zeros((3, 3, 3), dtype=np.float32)) | ||
| template = pv.PolyData( | ||
| np.array( | ||
| [ | ||
| [0.0, 0.0, 0.0], | ||
| [1.0, 0.0, 0.0], | ||
| [0.0, 1.0, 0.0], | ||
| ], | ||
| dtype=np.float64, | ||
| ) | ||
| ) | ||
| heart_mesh = pv.PolyData( | ||
| np.array( | ||
| [ | ||
| [0.0, 0.0, 0.0], | ||
| [1.0, 0.0, 0.0], | ||
| [0.0, 1.0, 0.0], | ||
| [0.0, 0.0, 1.0], | ||
| ], | ||
| dtype=np.float64, | ||
| ) | ||
| ) | ||
|
|
||
| captured: dict[str, Any] = {} | ||
|
|
||
| class _FakeConvertImageToVTK: | ||
| def __init__(self, **kwargs: Any) -> None: | ||
| captured["init_kwargs"] = kwargs | ||
|
|
||
| def run_workflow(self, **kwargs: Any) -> dict[str, Any]: | ||
| captured["run_kwargs"] = kwargs | ||
| return {"meshes": {"heart": heart_mesh}} | ||
|
|
||
| monkeypatch.setattr( | ||
| "physiomotion4d.workflow_fit_statistical_model_to_patient." | ||
| "WorkflowConvertImageToVTK", | ||
| _FakeConvertImageToVTK, | ||
| ) | ||
|
|
||
| workflow = WorkflowFitStatisticalModelToPatient( | ||
| template_model=template, | ||
| patient_image=image, | ||
| ) | ||
|
|
||
| assert captured["init_kwargs"]["segmentation_method"] == ( | ||
| "HeartSimplewareTrimmedBranches" | ||
| ) | ||
| assert captured["run_kwargs"]["anatomy_groups"] == ["heart"] | ||
| assert captured["run_kwargs"]["contrast_enhanced_study"] is False | ||
| assert workflow.patient_models == [heart_mesh] | ||
|
|
||
|
|
||
| def test_image_to_vtk_segmenter_dispatch_for_trimmed_branches() -> None: | ||
| """WorkflowConvertImageToVTK('HeartSimplewareTrimmedBranches') must | ||
| instantiate SegmentHeartSimpleware with branch trimming enabled, while | ||
| 'HeartSimpleware' must leave it disabled.""" | ||
| trimmed = WorkflowConvertImageToVTK( | ||
| segmentation_method="HeartSimplewareTrimmedBranches" | ||
| )._create_segmenter() | ||
| assert isinstance(trimmed, SegmentHeartSimpleware) | ||
| assert trimmed._trim_branches is True | ||
|
|
||
| plain = WorkflowConvertImageToVTK( | ||
| segmentation_method="HeartSimpleware" | ||
| )._create_segmenter() | ||
| assert isinstance(plain, SegmentHeartSimpleware) | ||
| assert plain._trim_branches is False | ||
|
|
There was a problem hiding this comment.
Add shape/axis metadata to the new synthetic test docstrings.
These tests use synthetic images but the new docstrings do not state image shape and axis order.
✏️ Suggested docstring updates
def test_fit_workflow_default_segmentation_method_is_trimmed_branches() -> None:
- """Default segmentation_method must match the KCL-Heart-Model fit contract."""
+ """Default segmentation method for synthetic image shape (Z, Y, X) = (3, 3, 3), LPS world frame."""
@@
def test_fit_workflow_routes_default_to_image_to_vtk_with_trimmed_branches(
monkeypatch: Any,
) -> None:
- """When patient_models is omitted, the workflow must invoke
- WorkflowConvertImageToVTK with segmentation_method='HeartSimplewareTrimmedBranches'."""
+ """Route check on synthetic image shape (Z, Y, X) = (3, 3, 3), LPS world frame."""
@@
def test_image_to_vtk_segmenter_dispatch_for_trimmed_branches() -> None:
- """WorkflowConvertImageToVTK('HeartSimplewareTrimmedBranches') must
- instantiate SegmentHeartSimpleware with branch trimming enabled, while
- 'HeartSimpleware' must leave it disabled."""
+ """Segmenter dispatch check (synthetic context, no image I/O), LPS world frame."""As per coding guidelines tests/**/*.py: In test docstrings, state image shape and axis order (e.g., shape (X, Y, Z, T) = (64, 64, 32, 1), LPS world frame).
🤖 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_workflow_fit_statistical_model_to_patient.py` around lines 96 -
178, Update the three test docstrings
(test_fit_workflow_default_segmentation_method_is_trimmed_branches,
test_fit_workflow_routes_default_to_image_to_vtk_with_trimmed_branches,
test_image_to_vtk_segmenter_dispatch_for_trimmed_branches) to include the
synthetic image metadata: state the image shape and axis order (for example
"shape (X, Y, Z, T) = (3, 3, 3, 1), LPS world frame") and any relevant
world/frame assumptions so readers know the test image dimensions and axis
ordering; add this brief metadata sentence to each test's docstring directly
above the test body.
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.
Summary
This PR has grown from a single-feature commit into a small stack covering three related concerns. All three commits are on
simplewaretrimmedbranches.1.
HeartSimplewareTrimmedBranchessegmentation option (f35cec3)WorkflowConvertImageToVTK,WorkflowConvertImageToUSD, and theconvert-image-to-vtk/convert-image-to-usdCLIs that runsSegmentHeartSimplewarewithset_trim_branches(True), matching the KCL-Heart-Model template extent.HeartSimplewareTrimmedBranchesthe default forWorkflowFitStatisticalModelToPatientand switch theHeart-Statistical_Model_To_Patient/CHOP-Valveexperiment to it (both fit KCL-derived statistical models to a patient image).HeartSimplewareno longer implicitly trims branches in the convert workflows. Trimming is now opt-in via the new option name. Class docstrings flag this for external callers (see commit 3).SegmentHeartSimpleware._trim_maskto_trim_branchesso the attribute matches the publicset_trim_branches()API.2. Opt-in test marker overhaul (
20ffaef)--run-slow,--run-gpu,--run-simplewarepytest flags; collection-hook auto-skips the matching markers unless the flag is passed. Mirrors the pre-existing--run-experiments/--run-tutorialspattern. Defaultpytest tests/is now fully fast.requires_datamarker: 29 decorator lines removed across 17 test files. Tests that need downloadable data request the session fixtures (test_directories,download_test_data,test_images) and the data is fetched on first use.TestRegisterImagesICON,TestSegmentHeartSimpleware, andTestSegmentChestTotalSegmentatorwithrequires_gpuso they fall into the new--run-gpubucket.ci.ymlgpu-tests,test-slow.yml,nightly-health.yml) to pass every opt-in flag (--run-gpu --run-slow --run-simpleware --run-experiments --run-tutorials). Tests whose host requirements aren't met (e.g. a licensed Simpleware install) runtime-skip cleanly via their internal guards.test_workflow_fit_statistical_model_to_patient.pycovering the newHeartSimplewareTrimmedBranchesdefault and theWorkflowConvertImageToVTKdispatch path (closes the codecov/patch gap from the first commit).CLAUDE.md,AGENTS.md,README.md,tests/README.md,docs/testing.rst,docs/contributing.rst,docs/developer/{core,extending,segmentation}.rst,docs/api/segmentation/base.rst,.agents/agents/testing.md,.agents/skills/test-feature/SKILL.md. The/test-featureskill andAGENTS.mdtesting role now strongly prefer real (downloaded) test data with baseline comparison over synthetic data.3. Review follow-up (
1ca1547)WorkflowConvertImageToVTKandWorkflowConvertImageToUSDclass docstrings warning that theHeartSimplewareoption no longer implicitly trims branches.else:branch inWorkflowConvertImageToUSD.__init__with an explicitelif self.segmentation_method in (...)pluselse: raise ValueError(...), matching the dispatch shape inWorkflowConvertImageToVTK._create_segmenterso a future fourth option cannot silently behave likeHeartSimpleware.Out of scope (separate follow-up)
SEGMENTATION_METHODSis duplicated across the two workflow modules and the two CLI modules. Worth deduplicating but kept separate from this PR.Test plan
ruff check+ruff format --checkclean.py -m pytest tests/passes (84 passed / 130 skipped — slow/GPU/Simpleware/experiment/tutorial buckets correctly auto-skip; data-fixture tests run automatically).test_workflow_fit_statistical_model_to_patienttests pass; cover default introspection, monkeypatched routing, and_create_segmenterdispatch for both heart options.pytest --helpshows--run-slow,--run-gpu,--run-simplewarealongside the pre-existing--run-experiments/--run-tutorials.gpu-testsinvocation).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes
Documentation
Tests