I4h byod#52
Conversation
…inetune - Rename WorkflowConvertCTToVTK → WorkflowConvertImageToVTK and its CLI (physiomotion4d-convert-ct-to-vtk → -convert-image-to-vtk); the workflow is image-modality-agnostic, not CT-specific. - Expose --segmentation-method and --registration-method on the ImageToVTK and ImageToUSD CLIs. Backend identifiers: 'ChestTotalSegmentator' / 'HeartSimpleware' and 'ANTS' / 'ICON' (breaking: replaces lowercase total_segmentator / simpleware_heart / ants / icon). - Add segmentation_method to WorkflowConvertImageToUSD (previously hardcoded to TotalSegmentator). - Rename SegmentHeartSimpleware.set_trim_mask_to_essentials → set_trim_branches (and trim_mask_to_essentials → trim_branches), with docs noting that trimming reduces inter-subject vessel-extent variability for AI-Ready / Sim-Ready model fitting, consistent with the KCL Heart example data. - Add RegisterImagesICON.finetune() for cohort fine-tuning of UniGradICON weights with optional mask pairs, persistent weight updates, and checkpoint export. - Propagate new identifiers through tutorials, experiments, docs, and regenerate API_MAP.md. - Add torch to mypy ignore_missing_imports override list. - Refresh CLAUDE.md with role, priorities, and behavior guidelines.
WalkthroughThis PR renames CT-specific workflows to generic Image variants, standardizes segmentation/registration identifiers (PascalCase/UPPERCASE), adds ICON fine-tuning and tensor helpers, introduces runtime segmentation selection and optional-mask handling, and updates CLI, docs, packaging, tutorials, tests, and exports to match. ChangesCore API Refinements & Workflow Modernization
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 standardizes “CT-to-VTK” naming to “Image-to-VTK”, updates segmentation/registration method identifiers to the new canonical values (e.g., ChestTotalSegmentator, HeartSimpleware, ANTS, ICON), extends the Image→USD workflow/CLI to select segmentation backends, and adds an ICON network fine-tuning API.
Changes:
- Rename/replace
WorkflowConvertCTToVTK+ CLI entry point withWorkflowConvertImageToVTK(physiomotion4d-convert-image-to-vtk) and update tutorials/docs/tests accordingly. - Add
segmentation_methodselection toWorkflowConvertImageToUSDand thephysiomotion4d-convert-image-to-usdCLI; normalize registration method strings toANTS/ICON. - Add
RegisterImagesICON.finetune()and rename Simpleware trimming API toset_trim_branches()/trim_branches().
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_07_dirlab_pca_model.py | Switch tutorial to WorkflowConvertImageToVTK + updated segmentation method identifier. |
| tutorials/tutorial_02_ct_to_vtk.py | Update tutorial to new workflow class and save helpers. |
| tutorials/tutorial_01_heart_gated_ct_to_usd.py | Normalize registration method identifiers to ANTS/ICON in tutorial text and config. |
| tutorials/README.md | Update tutorials table to reflect WorkflowConvertImageToVTK. |
| tests/test_cli_smoke.py | Update smoke-import list for renamed CLI module. |
| src/physiomotion4d/workflow_fit_statistical_model_to_patient.py | Swap CT→VTK workflow dependency for Image→VTK and update default segmentation method identifier. |
| src/physiomotion4d/workflow_convert_image_to_vtk.py | Rename workflow class + segmentation method identifiers; update CLI name in docs/logging strings. |
| src/physiomotion4d/workflow_convert_image_to_usd.py | Add segmentation_method support and normalize registration method identifiers + validation. |
| src/physiomotion4d/segment_heart_simpleware.py | Rename trimming API (set_trim_branches, trim_branches) and expand docs. |
| src/physiomotion4d/register_images_icon.py | Refactor lazy net creation and add finetune() API for persistent weight updates + checkpoint save. |
| src/physiomotion4d/cli/convert_image_to_vtk.py | Update CLI text/choices/imports for WorkflowConvertImageToVTK + new method identifiers. |
| src/physiomotion4d/cli/convert_image_to_usd.py | Add --segmentation-method and normalize --registration-method to ANTS/ICON. |
| src/physiomotion4d/init.py | Export WorkflowConvertImageToVTK instead of removed WorkflowConvertCTToVTK. |
| README.md | Update quickstart and API examples to use ANTS/ICON. |
| pyproject.toml | Rename installed script to physiomotion4d-convert-image-to-vtk; add mypy ignore for torch.*. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py | Update segmentation method identifier to HeartSimpleware. |
| experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py | Update trimming call to trim_branches(). |
| docs/tutorials.rst | Update workflow class names in tutorial docs. |
| docs/troubleshooting.rst | Normalize registration method examples to ANTS. |
| docs/quickstart.rst | Normalize registration method examples to ANTS. |
| docs/examples.rst | Normalize registration method examples to ANTS. |
| docs/developer/workflows.rst | Update CLI/workflow mapping and registration method example strings. |
| docs/developer/architecture.rst | Update workflow naming from CT→VTK to Image→VTK. |
| docs/cli_scripts/overview.rst | Update CLI scripts list to physiomotion4d-convert-image-to-vtk. |
| docs/cli_scripts/heart_gated_ct.rst | Normalize registration method docs/examples to ANTS/ICON. |
| docs/cli_scripts/byod_tutorials.rst | Add BYOD guide for DICOM/VTK to USD workflows. |
| docs/cli_scripts/best_practices.rst | Normalize registration method example to ANTS. |
| docs/architecture.rst | Update primary workflows list to WorkflowConvertImageToVTK and CLI list. |
| docs/api/workflows.rst | Update module refs and examples to WorkflowConvertImageToVTK; add seg/registration args to Image→USD example. |
| docs/api/segmentation/totalsegmentator.rst | Update CLI name reference to physiomotion4d-convert-image-to-vtk. |
| docs/api/cli/index.rst | Replace convert_ct_to_vtk with convert_image_to_vtk in module index. |
| docs/api/cli/convert_image_to_vtk.rst | Add new API doc stub for the renamed CLI module. |
| docs/api/cli/convert_ct_to_vtk.rst | Remove obsolete CLI API doc stub. |
| docs/API_MAP.md | Regenerate API map to reflect renamed workflows and new finetune() API. |
| CLAUDE.md | Update contributor guidance content (plus a small typo). |
Comments suppressed due to low confidence (1)
src/physiomotion4d/workflow_convert_image_to_vtk.py:60
- The class docstring still says it segments a "CT image", but the workflow and CLI have been renamed to "Image" and the surrounding docs now describe it as operating on a generic 3D image. Please align this docstring with the new naming (either describe it as a generic 3D image workflow, or explicitly explain that the supported backends expect CT inputs).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| .. code-block:: bash | ||
|
|
||
| physiomotion4d-convert-heart-gated-ct-to-usd --help |
| wf = pm4d.WorkflowConvertHeartGatedCTToUSD() | ||
| wf.input_image = "patient_ct.nii.gz" | ||
| wf.output_file = "patient_heart.usd" |
| def finetune( | ||
| self, | ||
| image_pairs: Sequence[tuple[itk.Image, itk.Image]], | ||
| output_model_filename: str, | ||
| mask_pairs: Optional[Sequence[tuple[itk.Image, itk.Image]]] = None, |
| 5) testing | ||
|
|
||
| ## Behavior Guidelines | ||
| 1) Don't assume. Don't hide confuction. Surface tradeoffs. |
| patient_image: Optional[itk.Image] = None, | ||
| segmentation_method: str = "simpleware_heart", | ||
| segmentation_method: str = "HeartSimpleware", | ||
| log_level: int | str = logging.INFO, | ||
| ): | ||
| """Initialize the model-to-image-and-model registration pipeline. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 29.36% 29.98% +0.61%
==========================================
Files 49 49
Lines 6728 6806 +78
==========================================
+ Hits 1976 2041 +65
- Misses 4752 4765 +13
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/physiomotion4d/register_images_icon.py (1)
336-443: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd targeted tests for
finetune()before merge.This introduces a new public API (
finetune) with non-trivial behavior (validation, optimizer loop, checkpoint persistence), but the provided test updates don’t cover it. Please add focused tests for input validation, mask-pair length mismatch, and checkpoint output semantics.As per coding guidelines: "Update docstrings and tests for every changed public method."
🤖 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/register_images_icon.py` around lines 336 - 443, Add unit tests that exercise RegisterImagesICON.finetune's public behavior: 1) input validation tests asserting ValueError when image_pairs is empty and when epochs < 1; 2) mask-pair length mismatch test asserting ValueError when mask_pairs length != image_pairs; 3) functional test that runs finetune with a small synthetic image_pairs (and optional mask_pairs) mocking/using CPU so _ensure_net() creates the network, calling _image_to_resized_tensor/_mask_to_resized_tensor as in normal flow, then assert returned dict keys, that len(losses) == epochs * number_of_pairs, that a file was created at output_model_filename, that torch.load of that file yields a state_dict compatible with self.net.regis_net.state_dict(), and that self.weights_path was updated to the output path; use tmp_path fixtures for files and minimal epochs (1) to keep runtime small.
🧹 Nitpick comments (2)
src/physiomotion4d/workflow_fit_statistical_model_to_patient.py (1)
140-154: ⚡ Quick winAdd
segmentation_methodto the constructorArgsdocstring.Line 140 adds a public parameter, but it is not documented in the
Argsblock (Lines 145-154). Please add accepted values there to keep the API contract accurate.As per coding guidelines: "Update docstrings for every changed public method. Keep claims factual".
🤖 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` around lines 140 - 154, The constructor for the model-to-image-and-model registration pipeline now adds a public parameter segmentation_method but the Args docstring is missing it; update the Args block in the __init__ docstring to include a brief entry for segmentation_method describing accepted values (e.g., "HeartSimpleware" and any other supported strings), its purpose (which segmentation approach to use), and default value, and reference that if patient_image is None the code uses create_reference_image (contour_tools) to derive a reference—ensure the parameter name segmentation_method and its default "HeartSimpleware" are mentioned exactly as in the signature.src/physiomotion4d/register_images_icon.py (1)
313-317: ⚡ Quick winDocument axis order and tensor shapes in the new resize helpers.
Both helper docstrings describe array/tensor conversion but don’t explicitly state source axis order and resulting tensor shape. Add that explicitly to prevent X/Y/Z/T confusion at this boundary.
As per coding guidelines: "State axis order and shape explicitly in every docstring and comment that touches arrays."
Also applies to: 327-331
🤖 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/register_images_icon.py` around lines 313 - 317, Update the two new resize helper docstrings (the helper that "Convert an itk image to a torch tensor resized to the net's input grid" and the other helper mentioned nearby) to explicitly state the source ITK/numpy axis ordering and the exact resulting torch tensor shape and axis ordering (including where channel and time axes live, e.g. C, D, H, W or T, C, Z, Y, X), and note any axis reordering/transpose and dtype/contiguity performed so they exactly mirror icon_registration.itk_wrapper.register_pair's trilinear preprocessing; mention the conventions used for single-channel vs multi-channel inputs and any expected memory layout to avoid X/Y/Z/T confusion.
🤖 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 `@CLAUDE.md`:
- Line 19: Fix the typo in the CLAUDE.md guideline line that currently reads
"confuction" by replacing that token with the correct word "confusion" so the
sentence reads "Don't hide confusion" (identify and update the exact word
"confuction" in the file).
In `@docs/cli_scripts/byod_tutorials.rst`:
- Around line 37-38: Update the docs examples to use the standardized command
and class names instead of legacy identifiers: replace occurrences of the CLI
`physiomotion4d-convert-heart-gated-ct-to-usd` with
`physiomotion4d-convert-image-to-usd` and replace the class/workflow name
`WorkflowConvertHeartGatedCTToUSD` with `WorkflowConvertImageToUSD` in the
examples referenced (lines around the current occurrences). Ensure all affected
example blocks and help lines (the `--help` examples and any instantiation or
reference to the workflow class) are updated so copy/paste uses the new CLI and
class names consistently.
In `@src/physiomotion4d/workflow_convert_image_to_usd.py`:
- Around line 126-133: The HeartSimpleware branch can produce outputs missing
optional groups ('lung','bone','contrast') causing KeyError later in
_segment_and_register_frames; update the selection/handling so downstream mask
assembly is resilient: either ensure SegmentHeartSimpleware (or the assigned
self.segmenter) always returns those keys (with empty masks) or change
_segment_and_register_frames to use safe access (e.g., dict.get or membership
checks) when reading 'lung'/'bone'/'contrast' from the segmentation result
before indexing/iterating; reference SegmentHeartSimpleware,
SegmentChestTotalSegmentator, self.segmenter, and _segment_and_register_frames
when applying the fix.
---
Outside diff comments:
In `@src/physiomotion4d/register_images_icon.py`:
- Around line 336-443: Add unit tests that exercise
RegisterImagesICON.finetune's public behavior: 1) input validation tests
asserting ValueError when image_pairs is empty and when epochs < 1; 2) mask-pair
length mismatch test asserting ValueError when mask_pairs length != image_pairs;
3) functional test that runs finetune with a small synthetic image_pairs (and
optional mask_pairs) mocking/using CPU so _ensure_net() creates the network,
calling _image_to_resized_tensor/_mask_to_resized_tensor as in normal flow, then
assert returned dict keys, that len(losses) == epochs * number_of_pairs, that a
file was created at output_model_filename, that torch.load of that file yields a
state_dict compatible with self.net.regis_net.state_dict(), and that
self.weights_path was updated to the output path; use tmp_path fixtures for
files and minimal epochs (1) to keep runtime small.
---
Nitpick comments:
In `@src/physiomotion4d/register_images_icon.py`:
- Around line 313-317: Update the two new resize helper docstrings (the helper
that "Convert an itk image to a torch tensor resized to the net's input grid"
and the other helper mentioned nearby) to explicitly state the source ITK/numpy
axis ordering and the exact resulting torch tensor shape and axis ordering
(including where channel and time axes live, e.g. C, D, H, W or T, C, Z, Y, X),
and note any axis reordering/transpose and dtype/contiguity performed so they
exactly mirror icon_registration.itk_wrapper.register_pair's trilinear
preprocessing; mention the conventions used for single-channel vs multi-channel
inputs and any expected memory layout to avoid X/Y/Z/T confusion.
In `@src/physiomotion4d/workflow_fit_statistical_model_to_patient.py`:
- Around line 140-154: The constructor for the model-to-image-and-model
registration pipeline now adds a public parameter segmentation_method but the
Args docstring is missing it; update the Args block in the __init__ docstring to
include a brief entry for segmentation_method describing accepted values (e.g.,
"HeartSimpleware" and any other supported strings), its purpose (which
segmentation approach to use), and default value, and reference that if
patient_image is None the code uses create_reference_image (contour_tools) to
derive a reference—ensure the parameter name segmentation_method and its default
"HeartSimpleware" are mentioned exactly as in the signature.
🪄 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: f19bed35-3b33-447b-afee-0e421016e380
📒 Files selected for processing (35)
CLAUDE.mdREADME.mddocs/API_MAP.mddocs/api/cli/convert_ct_to_vtk.rstdocs/api/cli/convert_image_to_vtk.rstdocs/api/cli/index.rstdocs/api/segmentation/totalsegmentator.rstdocs/api/workflows.rstdocs/architecture.rstdocs/cli_scripts/best_practices.rstdocs/cli_scripts/byod_tutorials.rstdocs/cli_scripts/heart_gated_ct.rstdocs/cli_scripts/overview.rstdocs/developer/architecture.rstdocs/developer/workflows.rstdocs/examples.rstdocs/quickstart.rstdocs/troubleshooting.rstdocs/tutorials.rstexperiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.pypyproject.tomlsrc/physiomotion4d/__init__.pysrc/physiomotion4d/cli/convert_image_to_usd.pysrc/physiomotion4d/cli/convert_image_to_vtk.pysrc/physiomotion4d/register_images_icon.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.pytests/test_cli_smoke.pytutorials/README.mdtutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_02_ct_to_vtk.pytutorials/tutorial_07_dirlab_pca_model.py
💤 Files with no reviewable changes (1)
- docs/api/cli/convert_ct_to_vtk.rst
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/physiomotion4d/workflow_convert_image_to_usd.py (2)
49-63: ⚡ Quick winStrengthen type annotations on the updated initializer.
Since this public signature changed, please make it fully explicit for strict typing (
list[str]and-> None).Suggested patch
def __init__( self, - input_filenames: list, + input_filenames: list[str], contrast_enhanced: bool, output_directory: str, project_name: str, reference_image_filename: Optional[str] = None, number_of_registration_iterations: Optional[int] = 1, segmentation_method: str = "ChestTotalSegmentator", registration_method: str = "ICON", log_level: int | str = logging.INFO, save_registered_images: bool = True, save_registration_transforms: bool = True, save_labelmaps: bool = True, - ): + ) -> None:As per coding guidelines, use full type hints (
mypystrict mode enabled).🤖 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_convert_image_to_usd.py` around lines 49 - 63, The __init__ signature in class WorkflowConvertImageToUsd (method __init__) uses non-specific list and lacks an explicit return type; change input_filenames: list to input_filenames: list[str] and add an explicit -> None return annotation, and ensure Optional[...] and Union types remain consistent (e.g., number_of_registration_iterations: Optional[int] = 1, log_level: int | str = logging.INFO) so the initializer is fully typed for mypy strict mode; update the method signature accordingly and run type checks to confirm no other annotations are required.
284-303: ⚡ Quick winUse segmentation output geometry as the fallback-mask template.
Creating fallback masks from
self._fixed_imagecan drift from segmentation output geometry if a backend resamples internally. Building fromself._fixed_segmentation['labelmap']is safer for downstream mask arithmetic.Suggested patch
def _optional_mask(self, key: str) -> itk.Image: @@ assert self._fixed_segmentation is not None, "Fixed segmentation must be set" - assert self._fixed_image is not None, "Fixed image must be set" if key in self._fixed_segmentation: return self._fixed_segmentation[key] - zeros = np.zeros(itk.array_from_image(self._fixed_image).shape, dtype=np.uint8) - empty = itk.GetImageFromArray(zeros) - empty.CopyInformation(self._fixed_image) + labelmap = self._fixed_segmentation['labelmap'] + zeros = np.zeros(itk.array_from_image(labelmap).shape, dtype=np.uint8) + empty = itk.image_from_array(zeros) + empty.CopyInformation(labelmap) return empty🤖 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_convert_image_to_usd.py` around lines 284 - 303, The fallback mask builder _optional_mask currently creates zeros using self._fixed_image which can drift if the segmentation backend resampled; change it to prefer using the segmentation output geometry by using the image and array shape from self._fixed_segmentation['labelmap'] when present (create zeros with dtype=np.uint8 from itk.array_from_image(self._fixed_segmentation['labelmap']).shape and GetImageFromArray, then CopyInformation from that labelmap image); if 'labelmap' is not present, fall back to the existing behavior that uses self._fixed_image. Ensure assertions for self._fixed_segmentation/self._fixed_image remain.
🤖 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.
Nitpick comments:
In `@src/physiomotion4d/workflow_convert_image_to_usd.py`:
- Around line 49-63: The __init__ signature in class WorkflowConvertImageToUsd
(method __init__) uses non-specific list and lacks an explicit return type;
change input_filenames: list to input_filenames: list[str] and add an explicit
-> None return annotation, and ensure Optional[...] and Union types remain
consistent (e.g., number_of_registration_iterations: Optional[int] = 1,
log_level: int | str = logging.INFO) so the initializer is fully typed for mypy
strict mode; update the method signature accordingly and run type checks to
confirm no other annotations are required.
- Around line 284-303: The fallback mask builder _optional_mask currently
creates zeros using self._fixed_image which can drift if the segmentation
backend resampled; change it to prefer using the segmentation output geometry by
using the image and array shape from self._fixed_segmentation['labelmap'] when
present (create zeros with dtype=np.uint8 from
itk.array_from_image(self._fixed_segmentation['labelmap']).shape and
GetImageFromArray, then CopyInformation from that labelmap image); if 'labelmap'
is not present, fall back to the existing behavior that uses self._fixed_image.
Ensure assertions for self._fixed_segmentation/self._fixed_image remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89281ab7-30fb-4f31-943c-773c1eb0a495
📒 Files selected for processing (6)
CLAUDE.mddocs/API_MAP.mddocs/cli_scripts/byod_tutorials.rstsrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.py
✅ Files skipped from review due to trivial changes (2)
- docs/cli_scripts/byod_tutorials.rst
- docs/API_MAP.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/physiomotion4d/workflow_fit_statistical_model_to_patient.py
- src/physiomotion4d/register_images_icon.py
Summary by CodeRabbit
New Features
Updates