ENH: accept registration/segmentation method instances in workflows#71
Merged
Conversation
Workflows and RegisterTimeSeriesImages now take pre-constructed RegisterImagesBase / SegmentAnatomyBase instances instead of string identifiers. Callers configure backend-specific options (iteration counts, mass preservation, weights path, contrast threshold) on the instance; each workflow defaults to the instance matching its previous string default, so default behavior is unchanged. New classes: - RegisterImagesChain: run an ordered list of registrars in sequence, feeding each stage's forward_transform to the next as initial_forward_transform. - RegisterImagesGreedyICON(RegisterImagesChain): named Greedy-then-ICON chain with .greedy/.icon stage accessors (replaces the "Greedy_ICON" string mode). - SegmentHeartSimplewareTrimmedBranches(SegmentHeartSimpleware): moves the ~140-line trim_branches post-processing out of the flag-gated base class into a Template-Method subclass (replaces the "HeartSimplewareTrimmedBranches" string and set_trim_branches()). RegisterImagesBase gains _delegate_to/_capture_delegate_result helpers so composite/delegating registrars can drive an inner registrar's registration_method() without re-preprocessing the fixed image or re-dilating an already-converted mask; each stage still computes its own intensity preprocessing (ICON vs. Greedy differ). Removed: SEGMENTATION_METHODS/REGISTRATION_METHODS string tuples and their validation, WorkflowConvertImageToUSD.number_of_registration_iterations, RegisterTimeSeriesImages.set_number_of_iterations_greedy/_ICON and its registrar_greedy/registrar_ICON fields, WorkflowReconstructHighres4DCT's per-method iteration setters, and SegmentHeartSimpleware.set_trim_branches(). Passing a string now raises TypeError pointing at the instance-based API. CLIs keep their string flags via a shared cli/_method_factories.py that maps names to configured instances; also fixes a pre-existing bug where --ICON-iterations was read as args.icon_iterations and silently ignored. Updates tutorials 01/02/06/08, docs (.rst) + README examples, experiments, and tests (new coverage for the chain, greedy-icon, trimmed-branches, and the three workflows' default/type-guard behavior). Also fixes 10 pre-existing mypy errors surfaced by strict checking, including a latent runtime crash in ImageTools.flip_image whose return annotation used `itk.Image | tuple[...]` (ITK SWIG types don't support `|`); switched to typing.Union with @overload signatures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR replaces string-selected segmentation and registration setup with configured backend instances across core APIs, workflows, CLIs, experiments, docs, tutorials, and tests. It also adds chained and Greedy-then-ICON registration backends, a trimmed-branches heart segmenter, and updates contributor guidance and CI workflow versions. ChangesBackend instance API and wiring
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR migrates PhysioMotion4D workflows and RegisterTimeSeriesImages from string-based backend selection to an instance-based API, so callers can configure segmentation/registration backends directly (while keeping prior defaults equivalent). It also introduces composite registration plumbing (registration chains) and a dedicated Simpleware “trim branches” segmenter subclass, and updates CLIs/docs/tutorials/tests accordingly.
Changes:
- Replaces string
segmentation_method/registration_methodparameters with pre-constructedSegmentAnatomyBase/RegisterImagesBaseinstances across workflows and time-series registration. - Adds composite registration (
RegisterImagesChain, plusRegisterImagesGreedyICONconvenience chain) and moves Simpleware branch-trimming intoSegmentHeartSimplewareTrimmedBranches. - Updates CLIs (string flags mapped via factories), tutorials, docs, and adds/updates tests for defaults, type-guards, and new composites.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_08_cardiac_fit_model.py | Updates tutorial to pass a configured RegisterImagesICON() instance instead of a string mode. |
| tutorials/tutorial_06_reconstruct_highres_4d_ct.py | Switches tutorial to RegisterImagesGreedyICON instance configuration. |
| tutorials/tutorial_02_ct_to_vtk.py | Uses an instance-based SegmentChestTotalSegmentator (with Windows multiprocessing guard). |
| tutorials/tutorial_01_heart_gated_ct_to_usd.py | Updates documentation and tutorial code to instance-based registration method usage. |
| tests/test_workflow_reconstruct_highres_4d_ct.py | New tests covering instance-based registration_method defaults and type-guard behavior. |
| tests/test_workflow_fit_statistical_model_to_patient.py | Updates tests for instance-based segmentation defaults and pass-through behavior. |
| tests/test_workflow_fine_tune_icon_registration.py | Adjusts test typing for updated workflow initialization contract. |
| tests/test_workflow_convert_image_to_vtk.py | New tests for instance-based segmentation_method defaults and type-guard behavior. |
| tests/test_workflow_convert_image_to_usd.py | Adds tests for default instances, type-guards, and “use supplied instance as-is” behavior. |
| tests/test_vtk_to_usd_library.py | Adds a mypy-only unreachable ignore to keep Windows static analysis happy. |
| tests/test_segment_heart_simpleware_trimmed_branches.py | New tests for trimmed-branches postprocessing and (gated) Simpleware regression behavior. |
| tests/test_register_time_series_images.py | Updates tests to construct/parameterize registrar instances directly. |
| tests/test_register_images_icon.py | Minor typing cleanup (cast) for strict type checking. |
| tests/test_register_images_greedy_icon.py | New wiring tests for the RegisterImagesGreedyICON chain class. |
| tests/test_register_images_chain.py | New plumbing tests for RegisterImagesChain. |
| tests/test_convert_vtk_to_usd.py | Minor variable typing cleanup in test helper. |
| src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py | Moves workflow to accept RegisterImagesBase instances (defaulting to RegisterImagesGreedyICON). |
| src/physiomotion4d/workflow_fit_statistical_model_to_patient.py | Converts segmentation_method to optional SegmentAnatomyBase instance with default routing. |
| src/physiomotion4d/workflow_fine_tune_icon_registration.py | Uses configured RegisterImagesICON instance for time-series registration path. |
| src/physiomotion4d/workflow_convert_image_to_vtk.py | Converts workflow to accept a SegmentAnatomyBase instance (defaulting to SegmentChestTotalSegmentator). |
| src/physiomotion4d/workflow_convert_image_to_usd.py | Converts workflow to accept segmenter/registrar instances and moves default configuration to init-time. |
| src/physiomotion4d/segment_heart_simpleware.py | Removes flag-gated branch trimming from base segmenter; points users to the subclass. |
| src/physiomotion4d/segment_heart_simpleware_trimmed_branches.py | New subclass implementing branch trimming via template-method override. |
| src/physiomotion4d/register_time_series_images.py | Refactors to delegate to a caller-supplied RegisterImagesBase backend instance. |
| src/physiomotion4d/register_images_greedy_icon.py | New named chain class (Greedy → ICON) with stage accessors. |
| src/physiomotion4d/register_images_chain.py | New composite registrar that sequences multiple registrars and propagates transforms. |
| src/physiomotion4d/register_images_base.py | Adds delegation helpers for composite/delegating registrars. |
| src/physiomotion4d/image_tools.py | Fixes type hints for flip_image using Union + @overload. |
| src/physiomotion4d/cli/reconstruct_highres_4d_ct.py | Uses CLI factories to build configured registrar instances; fixes ICON iterations flag bug. |
| src/physiomotion4d/cli/convert_image_to_vtk.py | Uses CLI factory for segmentation method instance construction. |
| src/physiomotion4d/cli/convert_image_to_usd.py | Uses CLI factories and configures constructed instances based on CLI flags. |
| src/physiomotion4d/cli/_method_factories.py | New shared mapping from CLI string flags to configured backend instances. |
| src/physiomotion4d/init.py | Exposes new public classes (RegisterImagesChain, RegisterImagesGreedyICON, trimmed segmenter). |
| README.md | Updates examples to instance-based backend configuration. |
| experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py | Updates experiment to build registrar instances explicitly. |
| experiments/LongitudinalRegistration/3-eval_icon.py | Updates experiment to build/configure registrar instances explicitly. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py | Updates experiment to pass trimmed-branches segmenter instance. |
| docs/quickstart.rst | Updates tutorial counts and instance-based code examples. |
| docs/index.rst | Adds cards/links for tutorials 8–10 variants. |
| docs/examples.rst | Updates examples to instance-based backend configuration. |
| docs/developer/workflows.rst | Updates workflow example to instance-based registration method. |
| docs/developer/registration_images.rst | Updates time-series docs and adds composite registrar documentation. |
| docs/api/workflows.rst | Updates API docs examples to pass backend instances. |
| docs/api/registration/time_series.rst | Updates time-series API docs to instance-based backend usage. |
| CLAUDE.md | Removes “call out breaking changes explicitly” line from the contributor checklist. |
| AGENTS.md | Removes “call out breaking changes explicitly” line from the contributor checklist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+392
to
+399
| other.modality = self.modality | ||
| other.mask_dilation_mm = self.mask_dilation_mm | ||
| other.fixed_image = self.fixed_image | ||
| if other.fixed_image_pre is None: | ||
| other.fixed_image_pre = other.preprocess( | ||
| other.fixed_image, modality=other.modality | ||
| ) | ||
| other.fixed_mask = self.fixed_mask |
| """ | ||
|
|
||
| import logging | ||
| from typing import Optional, Union |
| initial_forward_transform=current_initial, | ||
| ) | ||
| self._capture_delegate_result(registrar, result) | ||
| current_initial = result["forward_transform"] |
| self.seen_moving_image: Optional[itk.Image] = None | ||
| self.seen_moving_image_pre: Optional[itk.Image] = None | ||
| self.seen_moving_mask: Optional[itk.Image] = None | ||
| self.seen_initial_forward_transform: Optional[object] = None |
Comment on lines
+52
to
+70
| def registration_method( | ||
| self, | ||
| moving_image: itk.Image, | ||
| moving_mask: Optional[itk.Image] = None, | ||
| moving_labelmap: Optional[itk.Image] = None, | ||
| moving_image_pre: Optional[itk.Image] = None, | ||
| initial_forward_transform: Optional[object] = None, | ||
| ) -> dict[str, Union[object, float]]: | ||
| """Record the state visible at call time; return a sentinel result.""" | ||
| self.seen_fixed_image_pre = self.fixed_image_pre | ||
| self.seen_moving_image = self.moving_image | ||
| self.seen_moving_image_pre = moving_image_pre | ||
| self.seen_moving_mask = moving_mask | ||
| self.seen_initial_forward_transform = initial_forward_transform | ||
| return { | ||
| "forward_transform": f"forward_{self.name}", | ||
| "inverse_transform": f"inverse_{self.name}", | ||
| "loss": self.sentinel_value, | ||
| } |
Comment on lines
+84
to
+86
| assert stage2.seen_initial_forward_transform == "forward_stage1" | ||
| assert result["forward_transform"] == "forward_stage2" | ||
| assert result["loss"] == 2.0 |
Comment on lines
+78
to
+82
| registration_method (Optional[RegisterImagesBase]): Registration | ||
| backend instance. Defaults to a new :class:`RegisterImagesICON` | ||
| when None. A caller-supplied instance is mutated (fixed | ||
| image/mask/modality) during :meth:`process` - pass a fresh | ||
| instance per run unless intentionally reusing state. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/physiomotion4d/segment_heart_simpleware.py (1)
119-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the remaining
set_trim_branchescaller.The provided context still shows
experiments/LongitudinalRegistration/3-eval_icon.pycallingsegmenter.set_trim_branches(False). With this setter removed, that path will raiseAttributeError; for the old “no trim” behavior, remove the call, and useSegmentHeartSimplewareTrimmedBranchesonly where trimming is intended.🤖 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/segment_heart_simpleware.py` around lines 119 - 130, The remaining caller of the removed trim-branch setter still uses segmenter.set_trim_branches(False), which will now fail with AttributeError. Update the code that builds the SegmentHeartSimpleware instance so it no longer calls set_trim_branches, and keep the no-trim behavior by omitting that call while using SegmentHeartSimplewareTrimmedBranches only in the paths that actually need branch trimming.
🧹 Nitpick comments (4)
tests/test_segment_heart_simpleware_trimmed_branches.py (1)
57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert full ITK geometry, not just size.
The test says geometry is preserved, but only checks image size. Add spacing/origin/direction checks to catch
CopyInformationregressions. As per coding guidelines, follow the project’s data conventions for ITK images.Proposed test hardening
assert itk.size(trimmed) == itk.size(labelmap) + assert tuple(trimmed.GetSpacing()) == tuple(labelmap.GetSpacing()) + assert tuple(trimmed.GetOrigin()) == tuple(labelmap.GetOrigin()) + np.testing.assert_allclose( + itk.array_from_matrix(trimmed.GetDirection()), + itk.array_from_matrix(labelmap.GetDirection()), + ) trimmed_arr = itk.array_from_image(trimmed)🤖 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_segment_heart_simpleware_trimmed_branches.py` around lines 57 - 58, The trimmed-branches test currently verifies only image size, but it should assert full ITK geometry preservation to catch CopyInformation regressions. Update the assertions in the trimmed image test to compare spacing, origin, and direction between trimmed and labelmap, using the existing ITK image variables in the test around the current size check. Follow the project’s ITK data conventions when comparing geometry so the test fails if any metadata changes unexpectedly.Source: Coding guidelines
tests/test_register_images_chain.py (1)
89-109: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a test for the reused-instance / changed-fixed-image scenario.
Current tests only exercise a single
register()call per chain instance. GivenRegisterImagesChain/RegisterImagesGreedyICONinstances are intended to be reusable (per the PR's instance-based API design), a regression test asserting that each stage recomputesfixed_image_prewhen the chain's fixed image changes between calls would catch the caching issue flagged inregister_images_base.py.🤖 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_register_images_chain.py` around lines 89 - 109, Add a regression test for the reusable chain instance case in test_chain_propagates_fixed_and_moving_state_to_each_child or a nearby test helper. The current coverage only checks one register() call, so extend it to reuse the same RegisterImagesChain with a different fixed image between calls and assert each stage in _RecordingRegistrar recomputes fixed_image_pre from its own preprocess() rather than reusing stale cached state. Make sure the test exercises the RegisterImagesChain.register path and verifies the child stages see the updated fixed image preprocessing after set_fixed_image is called again.src/physiomotion4d/cli/convert_image_to_usd.py (1)
78-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winArgparse
choicesduplicate_method_factoriesconstants instead of importing them.These literal lists can drift from
SEGMENTATION_METHODS/REGISTRATION_METHODSin_method_factories.pyas backends are added/removed, silently locking users out of new options at the CLI parser (beforebuild_*is even reached).♻️ Proposed fix
-from ._method_factories import build_registration_method, build_segmentation_method +from ._method_factories import ( + build_registration_method, + build_segmentation_method, + SEGMENTATION_METHODS, +) ... parser.add_argument( "--segmentation-method", - choices=[ - "ChestTotalSegmentator", - "HeartSimpleware", - "HeartSimplewareTrimmedBranches", - ], + choices=list(SEGMENTATION_METHODS), default="ChestTotalSegmentator",🤖 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/cli/convert_image_to_usd.py` around lines 78 - 97, The argparse choices for the segmentation and registration flags are duplicated literals and can drift from the canonical backend lists. Update the CLI setup in convert_image_to_usd.py to import and use the shared SEGMENTATION_METHODS and REGISTRATION_METHODS from _method_factories.py for the "--segmentation-method" and "--registration-method" arguments, so the parser stays aligned with the available build_* backends.src/physiomotion4d/cli/_method_factories.py (1)
20-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate string→class dispatch into a single mapping to avoid drift.
The choice tuples (
SEGMENTATION_METHODS,REGISTRATION_METHODS) and the if/elif dispatch chains list the same strings independently; adding/removing a backend requires updating both in sync.♻️ Proposed refactor using a single-source-of-truth mapping
-#: Segmentation backend string choices exposed by CLI flags. -SEGMENTATION_METHODS: tuple[str, ...] = ( - "ChestTotalSegmentator", - "HeartSimpleware", - "HeartSimplewareTrimmedBranches", -) +_SEGMENTATION_FACTORIES: dict[str, type[SegmentAnatomyBase]] = { + "ChestTotalSegmentator": SegmentChestTotalSegmentator, + "HeartSimpleware": SegmentHeartSimpleware, + "HeartSimplewareTrimmedBranches": SegmentHeartSimplewareTrimmedBranches, +} + +#: Segmentation backend string choices exposed by CLI flags. +SEGMENTATION_METHODS: tuple[str, ...] = tuple(_SEGMENTATION_FACTORIES) -def build_segmentation_method(name: str) -> SegmentAnatomyBase: - ... - if name == "ChestTotalSegmentator": - return SegmentChestTotalSegmentator() - if name == "HeartSimpleware": - return SegmentHeartSimpleware() - if name == "HeartSimplewareTrimmedBranches": - return SegmentHeartSimplewareTrimmedBranches() - raise ValueError( - f"Unknown segmentation method: {name}. " - f"Must be one of: {', '.join(SEGMENTATION_METHODS)}." - ) +def build_segmentation_method(name: str) -> SegmentAnatomyBase: + ... + try: + return _SEGMENTATION_FACTORIES[name]() + except KeyError: + raise ValueError( + f"Unknown segmentation method: {name}. " + f"Must be one of: {', '.join(SEGMENTATION_METHODS)}." + ) from None(mirror the same pattern for
REGISTRATION_METHODS/build_registration_method)Also applies to: 30-51, 54-75
🤖 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/cli/_method_factories.py` around lines 20 - 27, Consolidate the backend string dispatch in _method_factories.py into a single source of truth so the CLI choices and factory resolution cannot drift. Replace the separate SEGMENTATION_METHODS/REGISTRATION_METHODS tuples plus the if/elif chains in build_segmentation_method and build_registration_method with one mapping per category from method name to class/construction logic, and derive the exposed choice tuples from those mappings.
🤖 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 `@experiments/LongitudinalRegistration/3-eval_icon.py`:
- Around line 38-65: The ICON and Greedy_ICON paths in _build_registrar are
passing icon_iterations into set_number_of_iterations without checking for None,
unlike the Greedy branch. Add the same None guard around
RegisterImagesICON.set_number_of_iterations and
greedy_icon.icon.set_number_of_iterations so the helper behaves safely when
icon_iterations is unset; use _build_registrar, RegisterImagesICON, and
RegisterImagesGreedyICON to locate the affected branches.
In `@src/physiomotion4d/cli/reconstruct_highres_4d_ct.py`:
- Around line 281-290: The ICON iteration selection in
reconstruct_highres_4d_ct.py is treating an explicit 0 as missing input, so
--ICON-iterations 0 gets replaced by the default. Update the logic around
args.ICON_iterations in the registration setup block to check for None
explicitly instead of relying on truthiness, while keeping the existing handling
in RegisterImagesGreedyICON and RegisterImagesICON.
In `@src/physiomotion4d/register_images_base.py`:
- Around line 358-404: The delegate setup in _delegate_to is reusing a stale
fixed_image_pre cache when other.fixed_image changes across reused registrar
instances. Update _delegate_to so the delegate’s cached fixed-image
preprocessing is invalidated or recomputed whenever other.fixed_image is
assigned from self.fixed_image, matching the cache reset behavior in
set_fixed_image(). Make sure RegisterImagesBase, _delegate_to, and
fixed_image_pre stay consistent so registration_method() never sees
preprocessing from a previous fixed image.
In `@src/physiomotion4d/segment_heart_simpleware_trimmed_branches.py`:
- Around line 27-33: The new __init__ method in
SegmentHeartSimplewareTrimmedBranches is missing the required return type
annotation, so update its signature to explicitly declare a None return. Keep
the constructor behavior the same and add the annotation on the __init__
definition alongside the existing log_level parameter and
super().__init__(log_level=log_level) call.
- Around line 187-189: The overlap cleanup in
segment_heart_simpleware_trimmed_branches.py is wiping out preserved vessel
labels by zeroing every voxel where labelmap_arr > 6 after the heart fill.
Update the np.where logic around heart_arr so the exclusion applies only to the
generated myocardium fill, while already-trimmed major-vessel labels 7-10 remain
intact. Use the heart_arr / lv_arr / labelmap_arr processing block as the target
area and keep the label handling consistent with the existing integer ITK-style
masks.
In `@src/physiomotion4d/workflow_convert_image_to_usd.py`:
- Around line 37-42: The class docstring and related method docs for the
image-to-USD workflow are inaccurate about when the caller-supplied registrar is
mutated. Update the docstring for the relevant public API in
`workflow_convert_image_to_usd.py` to state that `set_modality("ct")` and
`set_mask_dilation(5)` are applied unconditionally in `__init__`, while only the
fixed image/mask mutation happens during `process()`. Also clarify that these
two setters override any pre-configured values on the passed
`RegisterImagesBase` instance, unlike the backend-specific options that remain
caller-configurable before passing the object in.
In `@tests/test_segment_heart_simpleware_trimmed_branches.py`:
- Around line 68-70: The test function
`test_segment_trims_branches_relative_to_plain_segmenter` uses a bare `list`
type annotation for `test_images`, which is not strict-mypy friendly. Update the
parameter annotation to a parameterized collection type in this test signature
so the fixture typing remains explicit and compatible with strict type checking.
---
Outside diff comments:
In `@src/physiomotion4d/segment_heart_simpleware.py`:
- Around line 119-130: The remaining caller of the removed trim-branch setter
still uses segmenter.set_trim_branches(False), which will now fail with
AttributeError. Update the code that builds the SegmentHeartSimpleware instance
so it no longer calls set_trim_branches, and keep the no-trim behavior by
omitting that call while using SegmentHeartSimplewareTrimmedBranches only in the
paths that actually need branch trimming.
---
Nitpick comments:
In `@src/physiomotion4d/cli/_method_factories.py`:
- Around line 20-27: Consolidate the backend string dispatch in
_method_factories.py into a single source of truth so the CLI choices and
factory resolution cannot drift. Replace the separate
SEGMENTATION_METHODS/REGISTRATION_METHODS tuples plus the if/elif chains in
build_segmentation_method and build_registration_method with one mapping per
category from method name to class/construction logic, and derive the exposed
choice tuples from those mappings.
In `@src/physiomotion4d/cli/convert_image_to_usd.py`:
- Around line 78-97: The argparse choices for the segmentation and registration
flags are duplicated literals and can drift from the canonical backend lists.
Update the CLI setup in convert_image_to_usd.py to import and use the shared
SEGMENTATION_METHODS and REGISTRATION_METHODS from _method_factories.py for the
"--segmentation-method" and "--registration-method" arguments, so the parser
stays aligned with the available build_* backends.
In `@tests/test_register_images_chain.py`:
- Around line 89-109: Add a regression test for the reusable chain instance case
in test_chain_propagates_fixed_and_moving_state_to_each_child or a nearby test
helper. The current coverage only checks one register() call, so extend it to
reuse the same RegisterImagesChain with a different fixed image between calls
and assert each stage in _RecordingRegistrar recomputes fixed_image_pre from its
own preprocess() rather than reusing stale cached state. Make sure the test
exercises the RegisterImagesChain.register path and verifies the child stages
see the updated fixed image preprocessing after set_fixed_image is called again.
In `@tests/test_segment_heart_simpleware_trimmed_branches.py`:
- Around line 57-58: The trimmed-branches test currently verifies only image
size, but it should assert full ITK geometry preservation to catch
CopyInformation regressions. Update the assertions in the trimmed image test to
compare spacing, origin, and direction between trimmed and labelmap, using the
existing ITK image variables in the test around the current size check. Follow
the project’s ITK data conventions when comparing geometry so the test fails if
any metadata changes unexpectedly.
🪄 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: 5293c8e0-3671-408f-9959-cdeef853930b
📒 Files selected for processing (46)
AGENTS.mdCLAUDE.mdREADME.mddocs/api/registration/time_series.rstdocs/api/workflows.rstdocs/developer/registration_images.rstdocs/developer/workflows.rstdocs/examples.rstdocs/index.rstdocs/quickstart.rstexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.pyexperiments/LongitudinalRegistration/3-eval_icon.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct_class.pysrc/physiomotion4d/__init__.pysrc/physiomotion4d/cli/_method_factories.pysrc/physiomotion4d/cli/convert_image_to_usd.pysrc/physiomotion4d/cli/convert_image_to_vtk.pysrc/physiomotion4d/cli/reconstruct_highres_4d_ct.pysrc/physiomotion4d/image_tools.pysrc/physiomotion4d/register_images_base.pysrc/physiomotion4d/register_images_chain.pysrc/physiomotion4d/register_images_greedy_icon.pysrc/physiomotion4d/register_time_series_images.pysrc/physiomotion4d/segment_heart_simpleware.pysrc/physiomotion4d/segment_heart_simpleware_trimmed_branches.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_convert_image_to_vtk.pysrc/physiomotion4d/workflow_fine_tune_icon_registration.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.pysrc/physiomotion4d/workflow_reconstruct_highres_4d_ct.pytests/test_convert_vtk_to_usd.pytests/test_register_images_chain.pytests/test_register_images_greedy_icon.pytests/test_register_images_icon.pytests/test_register_time_series_images.pytests/test_segment_heart_simpleware_trimmed_branches.pytests/test_vtk_to_usd_library.pytests/test_workflow_convert_image_to_usd.pytests/test_workflow_convert_image_to_vtk.pytests/test_workflow_fine_tune_icon_registration.pytests/test_workflow_fit_statistical_model_to_patient.pytests/test_workflow_reconstruct_highres_4d_ct.pytutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_02_ct_to_vtk.pytutorials/tutorial_06_reconstruct_highres_4d_ct.pytutorials/tutorial_08_cardiac_fit_model.py
💤 Files with no reviewable changes (1)
- AGENTS.md
Comment on lines
+38
to
+65
| def _build_registrar( | ||
| reg_method: str, | ||
| greedy_iters, | ||
| icon_iterations, | ||
| weights_path, | ||
| ) -> RegisterImagesBase: | ||
| """Build a configured registrar instance for one of "Greedy", "ICON", or | ||
| "Greedy_ICON", matching this experiment's per-method config entries.""" | ||
| if reg_method == "Greedy": | ||
| greedy = RegisterImagesGreedy() | ||
| if greedy_iters is not None: | ||
| greedy.set_number_of_iterations(greedy_iters) | ||
| return greedy | ||
| if reg_method == "ICON": | ||
| icon = RegisterImagesICON() | ||
| icon.set_number_of_iterations(icon_iterations) | ||
| if weights_path is not None: | ||
| icon.set_weights_path(str(weights_path)) | ||
| return icon | ||
| if reg_method == "Greedy_ICON": | ||
| greedy_icon = RegisterImagesGreedyICON() | ||
| if greedy_iters is not None: | ||
| greedy_icon.greedy.set_number_of_iterations(greedy_iters) | ||
| greedy_icon.icon.set_number_of_iterations(icon_iterations) | ||
| if weights_path is not None: | ||
| greedy_icon.icon.set_weights_path(str(weights_path)) | ||
| return greedy_icon | ||
| raise ValueError(f"Unknown registration method: {reg_method}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing None-guard before set_number_of_iterations(icon_iterations).
The Greedy branch guards with if greedy_iters is not None, but the ICON branch (Line 53) and the Greedy_ICON branch's icon stage (Line 61) call set_number_of_iterations(icon_iterations) unconditionally. icon_iterations defaults to None in this script (used by all all_methods entries), so every ICON/Greedy_ICON run will pass None into set_number_of_iterations. The sibling helper in experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py guards all three branches with if iterations is not None:, confirming this asymmetry here is an oversight.
🐛 Proposed fix
if reg_method == "ICON":
icon = RegisterImagesICON()
- icon.set_number_of_iterations(icon_iterations)
+ if icon_iterations is not None:
+ icon.set_number_of_iterations(icon_iterations)
if weights_path is not None:
icon.set_weights_path(str(weights_path))
return icon
if reg_method == "Greedy_ICON":
greedy_icon = RegisterImagesGreedyICON()
if greedy_iters is not None:
greedy_icon.greedy.set_number_of_iterations(greedy_iters)
- greedy_icon.icon.set_number_of_iterations(icon_iterations)
+ if icon_iterations is not None:
+ greedy_icon.icon.set_number_of_iterations(icon_iterations)
if weights_path is not None:
greedy_icon.icon.set_weights_path(str(weights_path))
return greedy_iconSeparately: this _build_registrar duplicates similar logic in experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py and the CLI's _method_factories.py. Consolidating into one shared helper would have prevented this divergence.
📝 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
| def _build_registrar( | |
| reg_method: str, | |
| greedy_iters, | |
| icon_iterations, | |
| weights_path, | |
| ) -> RegisterImagesBase: | |
| """Build a configured registrar instance for one of "Greedy", "ICON", or | |
| "Greedy_ICON", matching this experiment's per-method config entries.""" | |
| if reg_method == "Greedy": | |
| greedy = RegisterImagesGreedy() | |
| if greedy_iters is not None: | |
| greedy.set_number_of_iterations(greedy_iters) | |
| return greedy | |
| if reg_method == "ICON": | |
| icon = RegisterImagesICON() | |
| icon.set_number_of_iterations(icon_iterations) | |
| if weights_path is not None: | |
| icon.set_weights_path(str(weights_path)) | |
| return icon | |
| if reg_method == "Greedy_ICON": | |
| greedy_icon = RegisterImagesGreedyICON() | |
| if greedy_iters is not None: | |
| greedy_icon.greedy.set_number_of_iterations(greedy_iters) | |
| greedy_icon.icon.set_number_of_iterations(icon_iterations) | |
| if weights_path is not None: | |
| greedy_icon.icon.set_weights_path(str(weights_path)) | |
| return greedy_icon | |
| raise ValueError(f"Unknown registration method: {reg_method}") | |
| def _build_registrar( | |
| reg_method: str, | |
| greedy_iters, | |
| icon_iterations, | |
| weights_path, | |
| ) -> RegisterImagesBase: | |
| """Build a configured registrar instance for one of "Greedy", "ICON", or | |
| "Greedy_ICON", matching this experiment's per-method config entries.""" | |
| if reg_method == "Greedy": | |
| greedy = RegisterImagesGreedy() | |
| if greedy_iters is not None: | |
| greedy.set_number_of_iterations(greedy_iters) | |
| return greedy | |
| if reg_method == "ICON": | |
| icon = RegisterImagesICON() | |
| if icon_iterations is not None: | |
| icon.set_number_of_iterations(icon_iterations) | |
| if weights_path is not None: | |
| icon.set_weights_path(str(weights_path)) | |
| return icon | |
| if reg_method == "Greedy_ICON": | |
| greedy_icon = RegisterImagesGreedyICON() | |
| if greedy_iters is not None: | |
| greedy_icon.greedy.set_number_of_iterations(greedy_iters) | |
| if icon_iterations is not None: | |
| greedy_icon.icon.set_number_of_iterations(icon_iterations) | |
| if weights_path is not None: | |
| greedy_icon.icon.set_weights_path(str(weights_path)) | |
| return greedy_icon | |
| raise ValueError(f"Unknown registration method: {reg_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 `@experiments/LongitudinalRegistration/3-eval_icon.py` around lines 38 - 65,
The ICON and Greedy_ICON paths in _build_registrar are passing icon_iterations
into set_number_of_iterations without checking for None, unlike the Greedy
branch. Add the same None guard around
RegisterImagesICON.set_number_of_iterations and
greedy_icon.icon.set_number_of_iterations so the helper behaves safely when
icon_iterations is unset; use _build_registrar, RegisterImagesICON, and
RegisterImagesGreedyICON to locate the affected branches.
Comment on lines
+281
to
+290
| # Set number of iterations based on registration method and CLI arguments | ||
| greedy_iterations = args.Greedy_iterations or [30, 15, 7, 3] | ||
| icon_iterations = args.ICON_iterations if args.ICON_iterations else 20 | ||
| if isinstance(registration_method, RegisterImagesGreedyICON): | ||
| registration_method.greedy.set_number_of_iterations(greedy_iterations) | ||
| registration_method.icon.set_number_of_iterations(icon_iterations) | ||
| elif isinstance(registration_method, RegisterImagesGreedy): | ||
| registration_method.set_number_of_iterations(greedy_iterations) | ||
| elif isinstance(registration_method, RegisterImagesICON): | ||
| registration_method.set_number_of_iterations(icon_iterations) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--ICON-iterations 0 is silently overridden to the default.
args.ICON_iterations if args.ICON_iterations else 20 treats an explicit 0 the same as "not provided", falling back to the default of 20 instead of honoring the user's 0. Use an explicit None check instead.
🐛 Proposed fix
- icon_iterations = args.ICON_iterations if args.ICON_iterations else 20
+ icon_iterations = (
+ args.ICON_iterations if args.ICON_iterations is not None else 20
+ )📝 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
| # Set number of iterations based on registration method and CLI arguments | |
| greedy_iterations = args.Greedy_iterations or [30, 15, 7, 3] | |
| icon_iterations = args.ICON_iterations if args.ICON_iterations else 20 | |
| if isinstance(registration_method, RegisterImagesGreedyICON): | |
| registration_method.greedy.set_number_of_iterations(greedy_iterations) | |
| registration_method.icon.set_number_of_iterations(icon_iterations) | |
| elif isinstance(registration_method, RegisterImagesGreedy): | |
| registration_method.set_number_of_iterations(greedy_iterations) | |
| elif isinstance(registration_method, RegisterImagesICON): | |
| registration_method.set_number_of_iterations(icon_iterations) | |
| # Set number of iterations based on registration method and CLI arguments | |
| greedy_iterations = args.Greedy_iterations or [30, 15, 7, 3] | |
| icon_iterations = ( | |
| args.ICON_iterations if args.ICON_iterations is not None else 20 | |
| ) | |
| if isinstance(registration_method, RegisterImagesGreedyICON): | |
| registration_method.greedy.set_number_of_iterations(greedy_iterations) | |
| registration_method.icon.set_number_of_iterations(icon_iterations) | |
| elif isinstance(registration_method, RegisterImagesGreedy): | |
| registration_method.set_number_of_iterations(greedy_iterations) | |
| elif isinstance(registration_method, RegisterImagesICON): | |
| registration_method.set_number_of_iterations(icon_iterations) |
🤖 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/cli/reconstruct_highres_4d_ct.py` around lines 281 - 290,
The ICON iteration selection in reconstruct_highres_4d_ct.py is treating an
explicit 0 as missing input, so --ICON-iterations 0 gets replaced by the
default. Update the logic around args.ICON_iterations in the registration setup
block to check for None explicitly instead of relying on truthiness, while
keeping the existing handling in RegisterImagesGreedyICON and
RegisterImagesICON.
Comment on lines
+187
to
+189
| heart_arr = np.where(heart_arr == 0, lv_arr, heart_arr) | ||
| # Eliminate overlap with other labels | ||
| heart_arr = np.where(labelmap_arr > 6, 0, heart_arr) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve trimmed major-vessel labels.
Line 189 clears every voxel whose original label was > 6, which removes inherited major-vessel labels 7-10 after they were clipped. Apply the guard only to the generated myocardium fill so existing trimmed vessel labels survive. Based on learnings, keep masks as ITK images with integer labels and keep anatomy group IDs consistent across segmenters.
Proposed fix
- heart_arr = np.where(heart_arr == 0, lv_arr, heart_arr)
- # Eliminate overlap with other labels
- heart_arr = np.where(labelmap_arr > 6, 0, heart_arr)
+ # Do not let gap-filled myocardium overwrite original vessel labels.
+ lv_arr = np.where(labelmap_arr > 6, 0, lv_arr)
+ heart_arr = np.where(heart_arr == 0, lv_arr, heart_arr)📝 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
| heart_arr = np.where(heart_arr == 0, lv_arr, heart_arr) | |
| # Eliminate overlap with other labels | |
| heart_arr = np.where(labelmap_arr > 6, 0, heart_arr) | |
| # Do not let gap-filled myocardium overwrite original vessel labels. | |
| lv_arr = np.where(labelmap_arr > 6, 0, lv_arr) | |
| heart_arr = np.where(heart_arr == 0, lv_arr, heart_arr) |
🤖 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/segment_heart_simpleware_trimmed_branches.py` around lines
187 - 189, The overlap cleanup in segment_heart_simpleware_trimmed_branches.py
is wiping out preserved vessel labels by zeroing every voxel where labelmap_arr
> 6 after the heart fill. Update the np.where logic around heart_arr so the
exclusion applies only to the generated myocardium fill, while already-trimmed
major-vessel labels 7-10 remain intact. Use the heart_arr / lv_arr /
labelmap_arr processing block as the target area and keep the label handling
consistent with the existing integer ITK-style masks.
Source: Learnings
Comment on lines
+37
to
+42
| ``segmentation_method`` and ``registration_method`` accept a | ||
| pre-configured :class:`SegmentAnatomyBase` / :class:`RegisterImagesBase` | ||
| instance. Configure backend-specific parameters (iteration counts, | ||
| trim_branches, mass preservation, etc.) on the instance before passing | ||
| it in. Defaults to :class:`SegmentChestTotalSegmentator` / | ||
| :class:`RegisterImagesICON` when omitted. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring says mutation happens "during process()", but modality/mask_dilation are set in __init__.
Line 81 states the caller-supplied registrar is mutated "during process", but set_modality("ct") and set_mask_dilation(5) (Lines 126-127) run unconditionally in __init__ — only the fixed image/mask mutation actually happens during process(). Also worth calling out explicitly that these two setters always override whatever the caller pre-configured on their own instance (unlike contrast_threshold/mass_preservation, which are default-only), since this is the one exception to the "configure the instance before passing it in" guidance in the class docstring.
As per coding guidelines, "Update docstrings for every changed public method, and keep claims factual."
📝 Proposed docstring fix
registration_method (Optional[RegisterImagesBase]): Registration
backend instance. Defaults to a new :class:`RegisterImagesICON`
- when None. A caller-supplied instance is mutated (fixed
- image/mask/modality) during :meth:`process` - pass a fresh
- instance per run unless intentionally reusing state.
+ when None. A caller-supplied instance always has its
+ modality/mask_dilation forced to "ct"/5mm in ``__init__``,
+ and its fixed image/mask overwritten during :meth:`process`
+ - pass a fresh instance per run unless intentionally reusing
+ state.Also applies to: 75-96, 119-128
🤖 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 37 - 42,
The class docstring and related method docs for the image-to-USD workflow are
inaccurate about when the caller-supplied registrar is mutated. Update the
docstring for the relevant public API in `workflow_convert_image_to_usd.py` to
state that `set_modality("ct")` and `set_mask_dilation(5)` are applied
unconditionally in `__init__`, while only the fixed image/mask mutation happens
during `process()`. Also clarify that these two setters override any
pre-configured values on the passed `RegisterImagesBase` instance, unlike the
backend-specific options that remain caller-configurable before passing the
object in.
Source: Coding guidelines
Comment on lines
+68
to
+70
| def test_segment_trims_branches_relative_to_plain_segmenter( | ||
| test_images: list, | ||
| ) -> None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid the bare list annotation.
Use a parameterized collection type so this test remains strict-mypy friendly.
Proposed fix
def test_segment_trims_branches_relative_to_plain_segmenter(
- test_images: list,
+ test_images: list[itk.Image],
) -> None:📝 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
| def test_segment_trims_branches_relative_to_plain_segmenter( | |
| test_images: list, | |
| ) -> None: | |
| def test_segment_trims_branches_relative_to_plain_segmenter( | |
| test_images: list[itk.Image], | |
| ) -> None: |
🤖 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_segment_heart_simpleware_trimmed_branches.py` around lines 68 -
70, The test function `test_segment_trims_branches_relative_to_plain_segmenter`
uses a bare `list` type annotation for `test_images`, which is not strict-mypy
friendly. Update the parameter annotation to a parameterized collection type in
this test signature so the fixture typing remains explicit and compatible with
strict type checking.
Source: Coding guidelines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 32.36% 34.68% +2.31%
==========================================
Files 53 57 +4
Lines 7257 7283 +26
==========================================
+ Hits 2349 2526 +177
+ Misses 4908 4757 -151
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/nightly-health.yml (1)
57-62: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPython 3.11 venv path now diverges from
ci.yml's identical GPU job.This line hardcodes a specific developer account's home directory (
C:\Users\saylward\...), whileci.yml'sgpu-testsjob — which targets the exact same runner labels[self-hosted, Windows, X64, gpu]— still usesC:\Program Files\Python311\python.exe(unchanged). If both jobs run on the same physical self-hosted runner, one of these two paths is now stale and that workflow will fail to findpython.exe. If they target different machines, the user-specific path is fragile and will break if the runner account changes or is reprovisioned.Please confirm which path is actually correct for the runner and update
ci.yml'sgpu-testsjob (or this line) to match, so both workflows stay consistent.🤖 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 @.github/workflows/nightly-health.yml around lines 57 - 62, The GPU venv setup in this workflow is using a hardcoded user-specific Python 3.11 path that no longer matches the identical GPU job in ci.yml, so update the Create venv in RUNNER_TEMP step to use the same correct runner-local python.exe path as the gpu-tests job, or change both jobs to a shared confirmed path. Keep the venv creation and GITHUB_PATH update logic in sync between the two workflows so the nightly-health and ci.yml GPU jobs use the same Python 3.11 location.
🤖 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/test_register_images_chain.py`:
- Around line 112-115: Update the test docstring in
test_chain_recomputes_fixed_image_pre_when_fixed_image_changes to include the
required image shape and axis-order statement, since tests/**/*.py docstrings
must always specify this metadata. Keep the existing behavior description, but
append a phrase like “shape (X, Y, Z, T) = (64, 64, 32, 1), LPS world frame” so
the docstring complies with the path instructions.
---
Outside diff comments:
In @.github/workflows/nightly-health.yml:
- Around line 57-62: The GPU venv setup in this workflow is using a hardcoded
user-specific Python 3.11 path that no longer matches the identical GPU job in
ci.yml, so update the Create venv in RUNNER_TEMP step to use the same correct
runner-local python.exe path as the gpu-tests job, or change both jobs to a
shared confirmed path. Keep the venv creation and GITHUB_PATH update logic in
sync between the two workflows so the nightly-health and ci.yml GPU jobs use the
same Python 3.11 location.
🪄 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: d8b59acf-61c8-4edf-9862-57fdc690854c
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/nightly-health.yml.github/workflows/test-slow.ymlsrc/physiomotion4d/cli/reconstruct_highres_4d_ct.pysrc/physiomotion4d/register_images_base.pysrc/physiomotion4d/register_images_chain.pytests/test_register_images_chain.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/test-slow.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/physiomotion4d/register_images_chain.py
- src/physiomotion4d/register_images_base.py
- src/physiomotion4d/cli/reconstruct_highres_4d_ct.py
Comment on lines
+112
to
+115
| def test_chain_recomputes_fixed_image_pre_when_fixed_image_changes() -> None: | ||
| """A reused chain must recompute each child's fixed_image_pre when the | ||
| chain's fixed image changes, not reuse a stale pre from the prior image; | ||
| but it must NOT recompute across frames that share the same fixed image.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test docstring is missing required image shape/axis-order statement.
As per path instructions, tests/**/*.py docstrings must state image shape and axis order, e.g. shape (X, Y, Z, T) = (64, 64, 32, 1), LPS world frame. This new test's docstring omits it.
📝 Proposed docstring addition
def test_chain_recomputes_fixed_image_pre_when_fixed_image_changes() -> None:
"""A reused chain must recompute each child's fixed_image_pre when the
chain's fixed image changes, not reuse a stale pre from the prior image;
- but it must NOT recompute across frames that share the same fixed image."""
+ but it must NOT recompute across frames that share the same fixed image.
+
+ Images: shape (X, Y, Z) synthetic constant-value ITK images, LPS world frame.
+ """📝 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
| def test_chain_recomputes_fixed_image_pre_when_fixed_image_changes() -> None: | |
| """A reused chain must recompute each child's fixed_image_pre when the | |
| chain's fixed image changes, not reuse a stale pre from the prior image; | |
| but it must NOT recompute across frames that share the same fixed image.""" | |
| def test_chain_recomputes_fixed_image_pre_when_fixed_image_changes() -> None: | |
| """A reused chain must recompute each child's fixed_image_pre when the | |
| chain's fixed image changes, not reuse a stale pre from the prior image; | |
| but it must NOT recompute across frames that share the same fixed image. | |
| Images: shape (X, Y, Z) synthetic constant-value ITK images, 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_register_images_chain.py` around lines 112 - 115, Update the test
docstring in test_chain_recomputes_fixed_image_pre_when_fixed_image_changes to
include the required image shape and axis-order statement, since tests/**/*.py
docstrings must always specify this metadata. Keep the existing behavior
description, but append a phrase like “shape (X, Y, Z, T) = (64, 64, 32, 1), LPS
world frame” so the docstring complies with the path instructions.
Source: Path instructions
This was referenced Jul 3, 2026
Closed
Merged
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.
Workflows and RegisterTimeSeriesImages now take pre-constructed RegisterImagesBase / SegmentAnatomyBase instances instead of string identifiers. Callers configure backend-specific options (iteration counts, mass preservation, weights path, contrast threshold) on the instance; each workflow defaults to the instance matching its previous string default, so default behavior is unchanged.
New classes:
RegisterImagesBase gains _delegate_to/_capture_delegate_result helpers so composite/delegating registrars can drive an inner registrar's registration_method() without re-preprocessing the fixed image or re-dilating an already-converted mask; each stage still computes its own intensity preprocessing (ICON vs. Greedy differ).
Removed: SEGMENTATION_METHODS/REGISTRATION_METHODS string tuples and their validation, WorkflowConvertImageToUSD.number_of_registration_iterations, RegisterTimeSeriesImages.set_number_of_iterations_greedy/_ICON and its registrar_greedy/registrar_ICON fields, WorkflowReconstructHighres4DCT's per-method iteration setters, and SegmentHeartSimpleware.set_trim_branches(). Passing a string now raises TypeError pointing at the instance-based API.
CLIs keep their string flags via a shared cli/_method_factories.py that maps names to configured instances; also fixes a pre-existing bug where --ICON-iterations was read as args.icon_iterations and silently ignored.
Updates tutorials 01/02/06/08, docs (.rst) + README examples, experiments, and tests (new coverage for the chain, greedy-icon, trimmed-branches, and the three workflows' default/type-guard behavior).
Also fixes 10 pre-existing mypy errors surfaced by strict checking, including a latent runtime crash in ImageTools.flip_image whose return annotation used
itk.Image | tuple[...](ITK SWIG types don't support|); switched to typing.Union with @overload signatures.Summary by CodeRabbit