Skip to content

I4h byod#52

Merged
aylward merged 4 commits into
Project-MONAI:mainfrom
aylward:i4h_byod
May 18, 2026
Merged

I4h byod#52
aylward merged 4 commits into
Project-MONAI:mainfrom
aylward:i4h_byod

Conversation

@aylward
Copy link
Copy Markdown
Collaborator

@aylward aylward commented May 17, 2026

Summary by CodeRabbit

  • New Features

    • Fine-tuning support for ICON registration models on custom image pairs.
    • Selectable segmentation backend via CLI/examples (ChestTotalSegmentator, HeartSimpleware).
  • Updates

    • Generalized CT→VTK workflow to a generic Image→VTK workflow and replaced CLI command accordingly.
    • Standardized method identifiers to uppercase (ANTS, ICON, ChestTotalSegmentator, HeartSimpleware).
    • Added a “Bring Your Own Data” DICOM & VTK to USD tutorial and refreshed docs/examples.

aylward and others added 3 commits May 17, 2026 10:14
…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.
Copilot AI review requested due to automatic review settings May 17, 2026 18:47
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Walkthrough

This 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.

Changes

Core API Refinements & Workflow Modernization

Layer / File(s) Summary
Workflow & VTK CLI transition
src/physiomotion4d/workflow_convert_image_to_vtk.py, src/physiomotion4d/cli/convert_image_to_vtk.py, pyproject.toml
Replaced CT→VTK usage with Image→VTK: new WorkflowConvertImageToVTK, updated segmentation backend identifiers/defaults (ChestTotalSegmentator/HeartSimpleware), CLI updates, and save helper calls.
Workflow ConvertImageToUSD updates
src/physiomotion4d/workflow_convert_image_to_usd.py
Add segmentation_method arg, validate segmentation/registration identifiers against SEGMENTATION_METHODS/REGISTRATION_METHODS, instantiate chosen segmenter/registrar, and add _optional_mask to tolerate missing anatomy groups.
ICON registration fine-tuning
src/physiomotion4d/register_images_icon.py, docs/API_MAP.md
Add DEFAULT_FINETUNE_LEARNING_RATE, lazy _ensure_net(), _image_to_resized_tensor(), _mask_to_resized_tensor(), and public finetune() to train and persist ICON model weights from image pairs.
SegmentHeartSimpleware rename
src/physiomotion4d/segment_heart_simpleware.py, experiments
Rename set_trim_mask_to_essentialsset_trim_branches and trim_mask_to_essentialstrim_branches; update call sites and experiments/tutorials to use new APIs.
CLI argument & package wiring
src/physiomotion4d/cli/convert_image_to_usd.py, src/physiomotion4d/cli/convert_image_to_vtk.py, src/physiomotion4d/__init__.py, pyproject.toml
Normalize --registration-method values to ANTS/ICON, add --segmentation-method choices, export WorkflowConvertImageToVTK in package __all__, and update installed script entrypoint.
Docs, tutorials, tests, and examples
docs/*, CLAUDE.md, README.md, tutorials/*, tests/test_cli_smoke.py, docs/cli_scripts/byod_tutorials.rst
Comprehensive documentation refresh to reference Image→VTK workflows, uppercase registration method examples, API_MAP updates (including finetune()), new BYOD tutorial, and test/quickstart/example normalization.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

"I nibble code at break of dawn,
I hop through docs until they're drawn,
I fine-tune ICON, name things neat,
PascalCase and UPPERCASE meet,
A rabbit cheers: your workflows run!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'I4h byod' is vague and uses non-descriptive abbreviations that do not clearly convey the main changes to the codebase. Use a descriptive title that summarizes the primary change, such as 'Rename CT-to-VTK workflow to Image-to-VTK and standardize registration method casing' or 'Refactor workflow naming and add image-to-VTK conversion capability'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with WorkflowConvertImageToVTK (physiomotion4d-convert-image-to-vtk) and update tutorials/docs/tests accordingly.
  • Add segmentation_method selection to WorkflowConvertImageToUSD and the physiomotion4d-convert-image-to-usd CLI; normalize registration method strings to ANTS/ICON.
  • Add RegisterImagesICON.finetune() and rename Simpleware trimming API to set_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.

Comment thread docs/cli_scripts/byod_tutorials.rst Outdated

.. code-block:: bash

physiomotion4d-convert-heart-gated-ct-to-usd --help
Comment thread docs/cli_scripts/byod_tutorials.rst Outdated
Comment on lines +69 to +71
wf = pm4d.WorkflowConvertHeartGatedCTToUSD()
wf.input_image = "patient_ct.nii.gz"
wf.output_file = "patient_heart.usd"
Comment on lines +336 to +340
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,
Comment thread CLAUDE.md Outdated
5) testing

## Behavior Guidelines
1) Don't assume. Don't hide confuction. Surface tradeoffs.
Comment on lines 139 to 143
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
Copy link
Copy Markdown

codecov Bot commented May 17, 2026

Codecov Report

❌ Patch coverage is 19.82759% with 93 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.98%. Comparing base (a76ad0b) to head (5af4a84).

Files with missing lines Patch % Lines
src/physiomotion4d/register_images_icon.py 18.33% 49 Missing ⚠️
...rc/physiomotion4d/workflow_convert_image_to_usd.py 14.70% 29 Missing ⚠️
src/physiomotion4d/cli/convert_image_to_vtk.py 14.28% 6 Missing ⚠️
...rc/physiomotion4d/workflow_convert_image_to_vtk.py 16.66% 5 Missing ⚠️
src/physiomotion4d/segment_heart_simpleware.py 50.00% 2 Missing ⚠️
...ion4d/workflow_fit_statistical_model_to_patient.py 33.33% 2 Missing ⚠️
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     
Flag Coverage Δ
integration-tests 29.98% <19.82%> (?)
unittests 29.26% <19.82%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Add 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 win

Add segmentation_method to the constructor Args docstring.

Line 140 adds a public parameter, but it is not documented in the Args block (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 win

Document 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

📥 Commits

Reviewing files that changed from the base of the PR and between a76ad0b and 74f527d.

📒 Files selected for processing (35)
  • CLAUDE.md
  • README.md
  • docs/API_MAP.md
  • docs/api/cli/convert_ct_to_vtk.rst
  • docs/api/cli/convert_image_to_vtk.rst
  • docs/api/cli/index.rst
  • docs/api/segmentation/totalsegmentator.rst
  • docs/api/workflows.rst
  • docs/architecture.rst
  • docs/cli_scripts/best_practices.rst
  • docs/cli_scripts/byod_tutorials.rst
  • docs/cli_scripts/heart_gated_ct.rst
  • docs/cli_scripts/overview.rst
  • docs/developer/architecture.rst
  • docs/developer/workflows.rst
  • docs/examples.rst
  • docs/quickstart.rst
  • docs/troubleshooting.rst
  • docs/tutorials.rst
  • experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py
  • experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py
  • pyproject.toml
  • src/physiomotion4d/__init__.py
  • src/physiomotion4d/cli/convert_image_to_usd.py
  • src/physiomotion4d/cli/convert_image_to_vtk.py
  • src/physiomotion4d/register_images_icon.py
  • src/physiomotion4d/segment_heart_simpleware.py
  • src/physiomotion4d/workflow_convert_image_to_usd.py
  • src/physiomotion4d/workflow_convert_image_to_vtk.py
  • src/physiomotion4d/workflow_fit_statistical_model_to_patient.py
  • tests/test_cli_smoke.py
  • tutorials/README.md
  • tutorials/tutorial_01_heart_gated_ct_to_usd.py
  • tutorials/tutorial_02_ct_to_vtk.py
  • tutorials/tutorial_07_dirlab_pca_model.py
💤 Files with no reviewable changes (1)
  • docs/api/cli/convert_ct_to_vtk.rst

Comment thread CLAUDE.md Outdated
Comment thread docs/cli_scripts/byod_tutorials.rst Outdated
Comment thread src/physiomotion4d/workflow_convert_image_to_usd.py
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/physiomotion4d/workflow_convert_image_to_usd.py (2)

49-63: ⚡ Quick win

Strengthen 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 (mypy strict 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 win

Use segmentation output geometry as the fallback-mask template.

Creating fallback masks from self._fixed_image can drift from segmentation output geometry if a backend resamples internally. Building from self._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

📥 Commits

Reviewing files that changed from the base of the PR and between 74f527d and 5af4a84.

📒 Files selected for processing (6)
  • CLAUDE.md
  • docs/API_MAP.md
  • docs/cli_scripts/byod_tutorials.rst
  • src/physiomotion4d/register_images_icon.py
  • src/physiomotion4d/workflow_convert_image_to_usd.py
  • src/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

@aylward aylward merged commit db1c416 into Project-MONAI:main May 18, 2026
11 of 12 checks passed
@aylward aylward deleted the i4h_byod branch May 18, 2026 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants