BUG:ACCESS_VIOLATION crashes and NumPy shape bug in 4D CT pipeline#68
Merged
Conversation
Four bugs fixed:
1. NumPy array shape wrong in create_distance_map (contour_tools.py)
ITK GetSize() returns (Nx, Ny, Nz) but NumPy needs (Nz, Ny, Nx)
ordering. np.zeros(size) was creating the wrong shape, causing an
IndexError when ix >= size[2].
2. TubeTK ResampleImage crashes ~80% on Windows (workflow_fit_statistical_model_to_patient.py)
ITK's lazy SWIG DLL loader for TubeTK is intermittently unstable on
Windows. Replace TubeTK.ResampleImage.SetMakeHighResIso() with a new
_make_isotropic_image() helper using standard ITK ResampleImageFilter
+ LinearInterpolateImageFunction. Equivalent output, no TubeTK DLL
needed for this path.
3. GPU libraries initialized at import time conflict with TubeTK DLL loading
Several modules imported CUDA-initializing libraries at module level,
which interfered with TubeTK's DLL initialization on Windows:
- register_images_icon.py: torch/icon_registration/unigradicon moved
into a _load_icon() lazy loader function
- segment_chest_total_segmentator.py: totalsegmentator import moved
inside segmentation_method() where it is used
- __init__.py: `import cupy` (which initializes CUDA at import time)
replaced with importlib.util.find_spec("cupy") existence check
- transform_tools.py: `import cupy` moved inside the one method
that uses it for GPU-accelerated norm computation
4. ITK lazy-load reference dropped before .New() (workflow_fit_statistical_model_to_patient.py)
ITK 6.0b2's __getattr__ returns a temporary that can be GC'd before
.New() completes. Fixed by storing the class reference before calling
.New() (now superseded by the TubeTK replacement in bug Project-MONAI#2).
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple Windows stability and correctness issues in the 4D CT / model-fitting pipeline by (1) fixing array axis ordering bugs, (2) removing unstable TubeTK resampling from a key workflow path, and (3) deferring GPU-library imports to runtime to avoid DLL/CUDA initialization conflicts.
Changes:
- Fix NumPy/ITK axis-ordering issues in distance-map generation and add diagnostics when meshes fall outside the reference image.
- Replace TubeTK isotropic resampling with a pure-ITK resampling helper, and propagate “mask→labelmap” terminology through the statistical-model fitting workflow/CLI/docs.
- Lazy-load CUDA/GPU-heavy dependencies (ICON/torch/unigradicon, TotalSegmentator, CuPy) to reduce import-time side effects and Windows crash risk.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_07_dirlab_pca_model.py | Updates workflow API call to new labelmap-to-labelmap registration toggle. |
| tests/test_workflow_fit_statistical_model_to_patient.py | Renames/updates helper test (but currently calls a non-existent workflow method). |
| src/physiomotion4d/workflow_fit_statistical_model_to_patient.py | TubeTK-free isotropic resampling; refactors mask-based stages to labelmap-based naming/behavior. |
| src/physiomotion4d/transform_tools.py | Moves optional CuPy import into the deformation-magnitude path to avoid import-time CUDA init. |
| src/physiomotion4d/segment_chest_total_segmentator.py | Defers TotalSegmentator import until the segmentation method executes. |
| src/physiomotion4d/register_models_distance_maps.py | Shifts terminology and implementation toward distance-map + binary registration masks; updates transform composition. |
| src/physiomotion4d/register_images_icon.py | Lazy-loads icon_registration/torch/unigradicon to avoid import-time GPU initialization. |
| src/physiomotion4d/contour_tools.py | Adds labelmap-from-meshes helper; fixes distance-map array shape ordering and adds logging for out-of-bounds meshes. |
| src/physiomotion4d/cli/fit_statistical_model_to_patient.py | CLI flags renamed to labelmap-to-*; output naming updated (l2l/l2i). |
| src/physiomotion4d/init.py | Replaces import cupy side effect with a find_spec availability check for warning. |
| pyproject.toml | Adds mypy override and ruff ignore for lazy-loaded ICON module annotations. |
| experiments/README.md | Updates terminology in experiment documentation (mask→labelmap). |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.py | Updates experiment script to new API names and l2l/l2i outputs. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py | Updates API call to new labelmap-to-labelmap toggle. |
| experiments/Heart-Create_Statistical_Model/3-registration_based_correspondence.py | Renames roi_dilation_mm usage to mask_dilation_mm in example code/comments. |
| docs/cli_scripts/fit_statistical_model_to_patient.rst | Updates CLI docs for labelmap-to-* flags and intermediate artifacts. |
| docs/architecture.rst | Updates architecture docs to reflect labelmap-based pipeline naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
| output = workflow._auto_generate_mask([model, model], dilate_mm=0.0) | ||
| output = workflow._auto_generate_labelmap([model, model], dilate_mm=0.0) |
Comment on lines
40
to
44
| Args: | ||
| mask_image (itk.image): The mask image to create contours from | ||
| labelmap_image (itk.image): The labelmap image to create contours from | ||
| output_file (str, optional): If provided, save the contours to this VTP | ||
| file | ||
|
|
Comment on lines
+97
to
100
| **Labelmap Configuration:** | ||
| Labelmaps are automatically generated from models if not provided by the user | ||
| via set_labelmaps(). Auto-generated labelmaps use labelmap_dilation_mm parameter. | ||
|
|
Comment on lines
+750
to
753
| patient_mask = self.contour_tools.create_mask_from_mesh( | ||
| self.patient_model_surface, | ||
| self.patient_image, | ||
| ) |
Comment on lines
295
to
299
| size = reference_image.GetLargestPossibleRegion().GetSize() | ||
| size = (size[2], size[1], size[0]) | ||
|
|
||
| tmp_arr = np.zeros(size, dtype=np.int32) | ||
| # NumPy convention is (z, y, x); ITK GetSize() returns (x, y, z) | ||
| tmp_arr = np.zeros((size[2], size[1], size[0]), dtype=np.int32) | ||
| itk_point = itk.Point[itk.D, 3]() |
Comment on lines
+255
to
+260
| def create_labelmap_from_meshes( | ||
| self, | ||
| meshes: list[pv.DataSet | pv.UnstructuredGrid], | ||
| reference_image: itk.Image, | ||
| ) -> itk.Image: | ||
| """ |
|
Warning Review limit reached
More reviews will be available in 42 minutes and 49 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR refactors the fit-statistical-model-to-patient pipeline from mask-based (m2m/m2i) to labelmap-based (l2l/l2i) deformable registration stages. ChangesLabelmap-Based Registration Pipeline Refactor
Sequence Diagram(s)sequenceDiagram
participant CLI as physiomotion4d-fit-statistical-model-to-patient
participant Workflow as WorkflowFitStatisticalModelToPatient
participant L2L as register_labelmap_to_labelmap
participant L2I as register_labelmap_to_image
participant RMD as RegisterModelsDistanceMaps
participant Greedy as GreedyRegistrar
participant ICON as RegisterImagesICON
CLI->>Workflow: set_use_labelmap_to_labelmap_registration(True)
CLI->>Workflow: set_use_labelmap_to_image_registration(True, label_ids, ...)
CLI->>Workflow: run_workflow()
Workflow->>L2L: register_labelmap_to_labelmap()
L2L->>RMD: register(template_labelmap → patient distance maps)
RMD->>Greedy: affine registration on distance maps with binary masks
Greedy-->>RMD: affine transform
RMD->>ICON: deformable refinement on pre-aligned distance map
ICON-->>RMD: deformable transforms
RMD-->>L2L: l2l_forward_transform, l2l_inverse_transform, l2l_template_labelmap
Workflow->>L2I: register_labelmap_to_image()
L2I->>L2I: propagate l2l_template_labelmap, remap labels to binary mask
L2I->>Greedy: register patient_image with moving_mask
Greedy-->>L2I: l2i affine transform
L2I->>ICON: optional ICON refinement
ICON-->>L2I: l2i deformable transforms
L2I-->>Workflow: registered_template_model_surface, registered_template_labelmap
Workflow-->>CLI: l2i_template_labelmap (or l2l fallback), l2l_template_model_surface
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
- Coverage 32.90% 32.46% -0.45%
==========================================
Files 53 53
Lines 7229 7244 +15
==========================================
- Hits 2379 2352 -27
- Misses 4850 4892 +42
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/physiomotion4d/register_models_distance_maps.py (2)
236-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
register()docstring to match the distance-map API.The method still says “mask-based”, describes ICON running on masks, and documents a
moving_modelreturn key even though the implementation returnsregistered_model.Proposed docstring cleanup
- """Perform mask-based registration of moving model to fixed model. + """Perform distance-map-based registration of moving model to fixed model. @@ **Deformable transform type:** 1. Greedy affine registration - 2. ICON deformable registration on the affine-pre-aligned masks + 2. ICON deformable registration on affine-pre-aligned distance maps @@ - - 'moving_model': Aligned moving model (PyVista PolyData) + - 'registered_model': Aligned moving model (PyVista PolyData)🤖 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_models_distance_maps.py` around lines 236 - 260, The docstring for the register() method contains outdated information that no longer matches the implementation. Update the summary line to change "mask-based registration" to "distance-map-based registration", update the description of the Deformable transform type to indicate that ICON deformable registration runs on distance maps instead of masks, and correct the Returns section to document that the dictionary contains a "registered_model" key instead of "moving_model" to accurately reflect what the method actually returns.
125-148:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
mask_dilation_mmin the public constructor.Direct users of
RegisterModelsDistanceMapscan pass a negative dilation and bypass workflow-level validation, which can shrink/fail registration masks instead of dilating them.Proposed guard
self.moving_model = moving_model self.fixed_model = fixed_model self.reference_image = reference_image + if mask_dilation_mm < 0: + raise ValueError("mask_dilation_mm must be non-negative") self.mask_dilation_mm = mask_dilation_mm🤖 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_models_distance_maps.py` around lines 125 - 148, Add validation for the mask_dilation_mm parameter in the __init__ method of the RegisterModelsDistanceMaps class to ensure it is not negative. After the super().__init__() call and before assigning self.mask_dilation_mm, check if mask_dilation_mm is less than zero and raise a ValueError with a descriptive message if it is. This prevents users from passing negative dilation values that would shrink registration masks instead of dilating them, ensuring the parameter behaves as documented.
🧹 Nitpick comments (1)
experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.py (1)
191-191: ⚡ Quick winReplace touched
These updated runtime-status lines still use
print(...); switch them to logging to keep output handling consistent.Suggested patch
+import logging @@ +logger = logging.getLogger(__name__) @@ - print("Starting deformable labelmap-to-labelmap registration...") + logger.info("Starting deformable labelmap-to-labelmap registration...") @@ - print(f"L2L inverse transform time: {time.time() - start_time} seconds", flush=True) + logger.info("L2L inverse transform time: %.6f seconds", time.time() - start_time) @@ - print(f"L2I inverse transform time: {time.time() - start_time} seconds", flush=True) + logger.info("L2I inverse transform time: %.6f seconds", time.time() - start_time)As per coding guidelines, "Use logging module instead of print statements in Python code."
Also applies to: 244-249
🤖 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/Heart-Statistical_Model_To_Patient/heart_model_to_patient.py` at line 191, Replace all print statements used for status output with logger method calls to maintain consistent output handling. Specifically, replace the print statement containing "Starting deformable labelmap-to-labelmap registration..." and the additional print statements in the range around lines 244-249 with appropriate logger.info() calls. Ensure that a logger instance is properly initialized in the module if not already present, and use logger.info() for informational status messages rather than print() to comply with logging guidelines.Source: Coding guidelines
🤖 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/README.md`:
- Around line 152-154: The README has inconsistent terminology for describing
the registration workflow. The section around lines 152-154 now uses
"labelmap-based" terminology, but a later section describing the same workflow
still refers to it as "mask-based." Search through the README for all references
to "mask-based" that describe the registration pipeline workflow and update them
to use "labelmap-based" terminology instead to maintain consistency with the
updated section and provide clear, non-conflicting guidance throughout the
document.
In `@pyproject.toml`:
- Around line 273-278: The tool.mypy.overrides section for the
physiomotion4d.register_images_icon module uses blanket suppression with
ignore_errors = true which can hide real regressions. Replace ignore_errors =
true with targeted disable_error_code entries for only the specific mypy
diagnostics that are actually needed (such as string forward-reference
annotation issues). Additionally, use TYPE_CHECKING imports for torch symbols in
the register_images_icon module itself rather than relying on file-wide
suppression, and apply the same targeted approach to the F821 suppression
mentioned at lines 407-408.
In `@src/physiomotion4d/cli/fit_statistical_model_to_patient.py`:
- Around line 243-246: The if condition around the
set_use_labelmap_to_labelmap_registration call only executes when
args.use_labelmap_to_labelmap is True, which means when the flag is False (from
--no-labelmap-to-labelmap), the method is never called and the workflow retains
its default value. Remove the if statement and unconditionally call
workflow.set_use_labelmap_to_labelmap_registration with
args.use_labelmap_to_labelmap so that both True and False values from the CLI
flag are properly wired to the workflow.
- Around line 289-299: The itk.imwrite calls used to save the labelmap files
(both for workflow.l2i_template_labelmap and workflow.l2l_template_labelmap) are
missing the compression parameter. Add compression=True as a keyword argument to
both itk.imwrite calls to enable compression when writing the output labelmap
files, following the coding guidelines for image storage.
In `@src/physiomotion4d/contour_tools.py`:
- Around line 40-43: The docstring for the extract_contours() function documents
an output_file parameter in the Args section that no longer exists in the
function signature. Remove the two lines in the Args block that document the
output_file parameter (the parameter name and its description about saving to
VTP file), keeping only the labelmap_image argument documentation.
- Around line 255-274: The create_labelmap_from_meshes method currently derives
label values from mesh ordering using i + 1, and uses a hardcoded np.uint8 dtype
which limits valid labels to 255. Modify the method signature to accept an
explicit anatomy_ids parameter (a list of label IDs for each mesh) instead of
deriving labels from index order, validate that the anatomy_ids list has the
correct length and unique values, dynamically determine the appropriate integer
dtype based on the maximum label value in anatomy_ids, and use the explicit
anatomy_ids when assigning labels in the loop instead of i + 1. Also add
Optional to the typing imports.
In `@src/physiomotion4d/transform_tools.py`:
- Around line 365-373: The exception handler currently only catches ImportError
and OSError, but CuPy v13.x supports lazy loading where the import succeeds even
without CUDA available. When GPU operations like cp.array() and cp.linalg.norm()
are executed without CUDA runtime, they raise CUDARuntimeError which bypasses
the fallback and crashes. Restructure the code by keeping the import cupy
statement in its own try-except block catching ImportError, then wrap the GPU
operations (cp.array for new_pnts_cp and pnts_cp, and cp.linalg.norm for the
DeformationMagnitude calculation) in a separate try-except block that catches
cupy_backends.cuda.api.runtime.CUDARuntimeError along with OSError, ensuring the
NumPy fallback is properly invoked when CUDA runtime is unavailable or
misconfigured.
In `@src/physiomotion4d/workflow_fit_statistical_model_to_patient.py`:
- Around line 44-47: The function _image_has_isotropic_spacing currently checks
all spacing dimensions including temporal spacing for 4D images, which can cause
incorrect behavior during resampling. Modify the function to only check the
first three spatial dimensions (X, Y, Z) by indexing into the spacing array with
[:3] before comparing with np.allclose, ensuring temporal spacing is excluded
from the isotropic check. Additionally, apply the same spatial-axes-only
restriction to the related resampling code around lines 56-62 where new_spacing
and new_size are constructed, limiting them to the first 3 dimensions while
preserving the temporal dimension unchanged.
- Around line 718-723: The ValueError raised when self.l2l_template_labelmap is
None unconditionally depends on L2L output, but labelmap-to-image registration
can be enabled independently of L2L. Instead of always raising an error, use the
latest propagated labelmap or surface available from any source (L2L, ICP, PCA,
or direct input), or validate earlier that the incompatible option combination
(L2I enabled without L2L or another valid labelmap source) is rejected before
registration starts. Apply the same fix to the similar check that appears around
lines 779-786.
- Around line 791-796: In the transform_image call within this code block,
replace the first argument `self.template_labelmap` with the L2L-propagated
labelmap variable (the labelmap that has already been transformed through the
L2L pipeline in patient-image space). The l2i_forward_transform was computed
based on the propagated labelmap, so applying it directly to the original
template_labelmap bypasses the ICP/PCA/L2L transforms and produces incorrect
registration. Identify the correct propagated labelmap variable in your codebase
and use it instead of self.template_labelmap in the
transform_tools.transform_image() call.
- Around line 725-742: The template_labelmap_arr remapping logic leaves unlisted
labels unchanged, which can unexpectedly influence Greedy/ICON processing.
Instead of chaining multiple np.where operations starting with the
template_labelmap_arr values, build the binary L2I labelmap directly by
initializing a zero array and then setting it to 1 only where
template_labelmap_arr contains values from template_labelmap_organ_extra_ids.
This ensures any labels not explicitly in template_labelmap_organ_extra_ids are
properly zeroed out rather than remaining unchanged.
---
Outside diff comments:
In `@src/physiomotion4d/register_models_distance_maps.py`:
- Around line 236-260: The docstring for the register() method contains outdated
information that no longer matches the implementation. Update the summary line
to change "mask-based registration" to "distance-map-based registration", update
the description of the Deformable transform type to indicate that ICON
deformable registration runs on distance maps instead of masks, and correct the
Returns section to document that the dictionary contains a "registered_model"
key instead of "moving_model" to accurately reflect what the method actually
returns.
- Around line 125-148: Add validation for the mask_dilation_mm parameter in the
__init__ method of the RegisterModelsDistanceMaps class to ensure it is not
negative. After the super().__init__() call and before assigning
self.mask_dilation_mm, check if mask_dilation_mm is less than zero and raise a
ValueError with a descriptive message if it is. This prevents users from passing
negative dilation values that would shrink registration masks instead of
dilating them, ensuring the parameter behaves as documented.
---
Nitpick comments:
In `@experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.py`:
- Line 191: Replace all print statements used for status output with logger
method calls to maintain consistent output handling. Specifically, replace the
print statement containing "Starting deformable labelmap-to-labelmap
registration..." and the additional print statements in the range around lines
244-249 with appropriate logger.info() calls. Ensure that a logger instance is
properly initialized in the module if not already present, and use logger.info()
for informational status messages rather than print() to comply with logging
guidelines.
🪄 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: 33b29c5e-c193-4760-b209-c33fee10f7c8
📒 Files selected for processing (17)
docs/architecture.rstdocs/cli_scripts/fit_statistical_model_to_patient.rstexperiments/Heart-Create_Statistical_Model/3-registration_based_correspondence.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.pyexperiments/README.mdpyproject.tomlsrc/physiomotion4d/__init__.pysrc/physiomotion4d/cli/fit_statistical_model_to_patient.pysrc/physiomotion4d/contour_tools.pysrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/register_models_distance_maps.pysrc/physiomotion4d/segment_chest_total_segmentator.pysrc/physiomotion4d/transform_tools.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.pytests/test_workflow_fit_statistical_model_to_patient.pytutorials/tutorial_07_dirlab_pca_model.py
💤 Files with no reviewable changes (1)
- tests/test_workflow_fit_statistical_model_to_patient.py
Comment on lines
+152
to
+154
| - Labelmap-based deformable registration for anatomical fitting | ||
| - PCA (Principal Component Analysis) shape modeling for shape constraints | ||
| - Three-stage registration pipeline (ICP → Mask-to-Mask → Mask-to-Image) | ||
| - Three-stage registration pipeline (ICP → Labelmap-to-Labelmap → Labelmap-to-Image) |
There was a problem hiding this comment.
Synchronize the remaining registration terminology in this README.
You updated this section to labelmap-based wording, but Line 250 still describes the same workflow as “mask-based,” which leaves conflicting guidance in one document.
🤖 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/README.md` around lines 152 - 154, The README has inconsistent
terminology for describing the registration workflow. The section around lines
152-154 now uses "labelmap-based" terminology, but a later section describing
the same workflow still refers to it as "mask-based." Search through the README
for all references to "mask-based" that describe the registration pipeline
workflow and update them to use "labelmap-based" terminology instead to maintain
consistency with the updated section and provide clear, non-conflicting guidance
throughout the document.
Comment on lines
+273
to
+278
| [[tool.mypy.overrides]] | ||
| # torch/icon_registration/unigradicon are lazy-loaded at runtime; string | ||
| # forward-reference annotations like "torch.Size" are intentional. | ||
| module = ["physiomotion4d.register_images_icon"] | ||
| ignore_errors = true | ||
|
|
There was a problem hiding this comment.
Avoid blanket type/lint suppression for register_images_icon.
Lines 273-278 (ignore_errors = true) disable all mypy checks for this module, and Lines 407-408 suppress F821 file-wide. Together, this can hide real regressions in a critical registration path. Prefer targeted suppressions (disable_error_code scoped to specific diagnostics) and TYPE_CHECKING imports for torch symbols.
As per coding guidelines, "Use full type hints with mypy strict mode (disallow_untyped_defs = true)."
Also applies to: 407-408
🤖 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 `@pyproject.toml` around lines 273 - 278, The tool.mypy.overrides section for
the physiomotion4d.register_images_icon module uses blanket suppression with
ignore_errors = true which can hide real regressions. Replace ignore_errors =
true with targeted disable_error_code entries for only the specific mypy
diagnostics that are actually needed (such as string forward-reference
annotation issues). Additionally, use TYPE_CHECKING imports for torch symbols in
the register_images_icon module itself rather than relying on file-wide
suppression, and apply the same targeted approach to the F821 suppression
mentioned at lines 407-408.
Source: Coding guidelines
Comment on lines
44
to
47
| def _image_has_isotropic_spacing(image: itk.Image) -> bool: | ||
| """Return whether all image spacing values match within numeric tolerance.""" | ||
| spacing = np.asarray(image.GetSpacing(), dtype=np.float64) | ||
| return bool(np.allclose(spacing, spacing[0])) |
There was a problem hiding this comment.
Keep isotropic resampling limited to spatial axes.
For a 4D image, spacing.min() can select the temporal spacing, while new_spacing/new_size are hard-coded to 3 entries. That can either fail at ITK resampling or resample the spatial grid using the time spacing. Only compare/resample X/Y/Z and preserve T unchanged, or reject non-3D images explicitly. As per coding guidelines, “4D time series data must have shape (X, Y, Z, T) and never silently squeeze or permute axes.”
Suggested direction
def _image_has_isotropic_spacing(image: itk.Image) -> bool:
"""Return whether all image spacing values match within numeric tolerance."""
spacing = np.asarray(image.GetSpacing(), dtype=np.float64)
- return bool(np.allclose(spacing, spacing[0]))
+ spatial_spacing = spacing[:3]
+ return bool(np.allclose(spatial_spacing, spatial_spacing[0]))
@@
spacing = np.asarray(image.GetSpacing(), dtype=np.float64)
size = np.asarray(image.GetLargestPossibleRegion().GetSize(), dtype=np.int64)
+ if spacing.size not in (3, 4):
+ raise ValueError("Only 3D images or 4D time series images are supported.")
- min_spacing = float(spacing.min())
- new_spacing = [min_spacing] * 3
+ min_spacing = float(spacing[:3].min())
+ new_spacing = spacing.tolist()
+ new_spacing[:3] = [min_spacing] * 3
# Ceiling to avoid clipping the image boundary.
- new_size = [int(np.ceil(size[i] * spacing[i] / min_spacing)) for i in range(3)]
+ new_size = size.tolist()
+ for axis in range(3):
+ new_size[axis] = int(np.ceil(size[axis] * spacing[axis] / min_spacing))Also applies to: 56-62
🤖 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
44 - 47, The function _image_has_isotropic_spacing currently checks all spacing
dimensions including temporal spacing for 4D images, which can cause incorrect
behavior during resampling. Modify the function to only check the first three
spatial dimensions (X, Y, Z) by indexing into the spacing array with [:3] before
comparing with np.allclose, ensuring temporal spacing is excluded from the
isotropic check. Additionally, apply the same spatial-axes-only restriction to
the related resampling code around lines 56-62 where new_spacing and new_size
are constructed, limiting them to the first 3 dimensions while preserving the
temporal dimension unchanged.
Source: Coding guidelines
Comment on lines
+718
to
723
| if self.l2l_template_labelmap is None: | ||
| raise ValueError( | ||
| "Mask-to-image registration requires a labelmap to have been set " | ||
| "(via set_use_mask_to_image_registration(True, ...)) before running " | ||
| "earlier stages so the labelmap is propagated through ICP/PCA/M2M." | ||
| "Labelmap-to-image registration requires a labelmap to have been set " | ||
| "(via set_use_labelmap_to_image_registration(True, ...)) before running " | ||
| "earlier stages so the labelmap is propagated through ICP/PCA/L2L." | ||
| ) |
There was a problem hiding this comment.
Do not make L2I depend unconditionally on L2L output.
--labelmap-to-image can be enabled while L2L is disabled, but this path always raises because self.l2l_template_labelmap is required. Use the latest propagated labelmap/surface available, or reject the incompatible option combination before registration starts.
Suggested direction
- if self.l2l_template_labelmap is None:
+ source_labelmap = self.l2l_template_labelmap
+ source_model_surface = self.l2l_template_model_surface
+ if source_labelmap is None:
+ source_labelmap = self.pca_template_labelmap
+ source_model_surface = self.pca_template_model_surface
+ if source_labelmap is None:
+ source_labelmap = self.icp_template_labelmap
+ source_model_surface = self.icp_template_model_surface
+ if source_labelmap is None:
raise ValueError(
"Labelmap-to-image registration requires a labelmap to have been set "
"(via set_use_labelmap_to_image_registration(True, ...)) before running "
"earlier stages so the labelmap is propagated through ICP/PCA/L2L."
)
@@
- assert self.l2l_template_model_surface is not None, (
- "L2L template model surface must be set"
- )
+ assert source_model_surface is not None, (
+ "Template model surface must be propagated before L2I registration"
+ )
self.l2i_template_model_surface = cast(
pv.PolyData,
self._transform_model_dataset(
- self.l2l_template_model_surface,
+ source_model_surface,
self.l2i_inverse_transform,Also applies to: 779-786
🤖 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
718 - 723, The ValueError raised when self.l2l_template_labelmap is None
unconditionally depends on L2L output, but labelmap-to-image registration can be
enabled independently of L2L. Instead of always raising an error, use the latest
propagated labelmap or surface available from any source (L2L, ICP, PCA, or
direct input), or validate earlier that the incompatible option combination (L2I
enabled without L2L or another valid labelmap source) is rejected before
registration starts. Apply the same fix to the similar check that appears around
lines 779-786.
Comment on lines
+725
to
742
| template_labelmap_arr = itk.GetArrayFromImage( | ||
| self.l2l_template_labelmap | ||
| ).astype(np.uint16) | ||
| template_labelmap_arr = np.where( | ||
| np.isin(template_labelmap_arr, self.template_labelmap_background_ids), | ||
| 0, | ||
| labelmap_arr, | ||
| template_labelmap_arr, | ||
| ) | ||
| labelmap_arr = np.where( | ||
| np.isin(labelmap_arr, self.template_labelmap_organ_mesh_ids), | ||
| template_labelmap_arr = np.where( | ||
| np.isin(template_labelmap_arr, self.template_labelmap_organ_mesh_ids), | ||
| 0, | ||
| labelmap_arr, | ||
| template_labelmap_arr, | ||
| ) | ||
| labelmap_arr = np.where( | ||
| np.isin(labelmap_arr, self.template_labelmap_organ_extra_ids), | ||
| template_labelmap_arr = np.where( | ||
| np.isin(template_labelmap_arr, self.template_labelmap_organ_extra_ids), | ||
| 1, | ||
| labelmap_arr, | ||
| template_labelmap_arr, | ||
| ) |
There was a problem hiding this comment.
Zero out labels that are not part of the L2I moving structure.
The current remap leaves any unlisted labels unchanged, so they remain nonzero in the moving image/mask and can influence Greedy/ICON unexpectedly. Build the binary L2I labelmap directly from template_labelmap_organ_extra_ids.
Suggested fix
- template_labelmap_arr = itk.GetArrayFromImage(
- self.l2l_template_labelmap
- ).astype(np.uint16)
- template_labelmap_arr = np.where(
- np.isin(template_labelmap_arr, self.template_labelmap_background_ids),
- 0,
- template_labelmap_arr,
- )
- template_labelmap_arr = np.where(
- np.isin(template_labelmap_arr, self.template_labelmap_organ_mesh_ids),
- 0,
- template_labelmap_arr,
- )
- template_labelmap_arr = np.where(
- np.isin(template_labelmap_arr, self.template_labelmap_organ_extra_ids),
- 1,
- template_labelmap_arr,
- )
+ source_labelmap_arr = itk.GetArrayFromImage(source_labelmap)
+ template_labelmap_arr = np.where(
+ np.isin(source_labelmap_arr, self.template_labelmap_organ_extra_ids),
+ 1,
+ 0,
+ ).astype(np.uint8)🤖 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
725 - 742, The template_labelmap_arr remapping logic leaves unlisted labels
unchanged, which can unexpectedly influence Greedy/ICON processing. Instead of
chaining multiple np.where operations starting with the template_labelmap_arr
values, build the binary L2I labelmap directly by initializing a zero array and
then setting it to 1 only where template_labelmap_arr contains values from
template_labelmap_organ_extra_ids. This ensures any labels not explicitly in
template_labelmap_organ_extra_ids are properly zeroed out rather than remaining
unchanged.
Comment on lines
+791
to
796
| self.l2i_template_labelmap = self.transform_tools.transform_image( | ||
| self.template_labelmap, | ||
| self.m2i_forward_transform, | ||
| self.l2i_forward_transform, | ||
| self.patient_image, | ||
| interpolation_method="nearest", | ||
| ) |
There was a problem hiding this comment.
Transform the propagated labelmap, not the original template labelmap.
self.l2i_forward_transform is computed from the L2L-propagated labelmap in patient-image space. Applying it directly to self.template_labelmap skips the ICP/PCA/L2L transforms and can produce a misregistered or empty final labelmap.
Suggested fix
self.l2i_template_labelmap = self.transform_tools.transform_image(
- self.template_labelmap,
+ source_labelmap,
self.l2i_forward_transform,
self.patient_image,
interpolation_method="nearest",🤖 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
791 - 796, In the transform_image call within this code block, replace the first
argument `self.template_labelmap` with the L2L-propagated labelmap variable (the
labelmap that has already been transformed through the L2L pipeline in
patient-image space). The l2i_forward_transform was computed based on the
propagated labelmap, so applying it directly to the original template_labelmap
bypasses the ICP/PCA/L2L transforms and produces incorrect registration.
Identify the correct propagated labelmap variable in your codebase and use it
instead of self.template_labelmap in the transform_tools.transform_image() call.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py (1)
61-61: 💤 Low valueClarify intent: uncomment or remove the commented call.
This commented-out line leaves the registration mode ambiguous. If the script intentionally relies on the default
use_l2l_registration=Truebehavior, consider removing the comment entirely to reduce confusion. If l2l registration should be disabled for this experiment, add a brief comment explaining why (e.g.,# L2L disabled: using PCA-only alignment for CHOPValve data).The sibling script
heart_model_to_patient.pyexplicitly invokes both l2l and l2i stages, so the differing behavior here should be documented.🤖 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/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py` at line 61, The commented-out call to registrar.set_use_labelmap_to_labelmap_registration(True) creates ambiguity about whether the script intentionally relies on default registration behavior or if L2L registration is being deliberately disabled. Either remove the commented line entirely if the script intentionally uses the default behavior, or if L2L registration should remain disabled for this CHOPValve experiment, add a clarifying comment above or on the same line explaining why (such as noting that the script uses PCA-only alignment instead). Since the sibling script heart_model_to_patient.py explicitly invokes both l2l and l2i stages, documenting the different behavior in this script will improve code clarity and maintainability.
🤖 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
`@experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.py`:
- Line 61: The commented-out call to
registrar.set_use_labelmap_to_labelmap_registration(True) creates ambiguity
about whether the script intentionally relies on default registration behavior
or if L2L registration is being deliberately disabled. Either remove the
commented line entirely if the script intentionally uses the default behavior,
or if L2L registration should remain disabled for this CHOPValve experiment, add
a clarifying comment above or on the same line explaining why (such as noting
that the script uses PCA-only alignment instead). Since the sibling script
heart_model_to_patient.py explicitly invokes both l2l and l2i stages,
documenting the different behavior in this script will improve code clarity and
maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39ca57ab-3629-4a6a-be72-87e6d2b323d4
📒 Files selected for processing (3)
experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.pysrc/physiomotion4d/cli/fit_statistical_model_to_patient.pysrc/physiomotion4d/contour_tools.py
💤 Files with no reviewable changes (1)
- src/physiomotion4d/contour_tools.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/physiomotion4d/cli/fit_statistical_model_to_patient.py
Comment on lines
356
to
360
| self.transform_tools.combine_displacement_field_transforms( | ||
| forward_transform_Greedy, | ||
| forward_transform_ICON, | ||
| forward_transform_Greedy, | ||
| reference_image=self.reference_image, | ||
| mode="compose", |
Comment on lines
364
to
368
| self.transform_tools.combine_displacement_field_transforms( | ||
| inverse_transform_ICON, | ||
| inverse_transform_Greedy, | ||
| inverse_transform_ICON, | ||
| reference_image=self.reference_image, | ||
| mode="compose", |
Comment on lines
+750
to
757
| patient_mask = self.contour_tools.create_mask_from_mesh( | ||
| self.patient_model_surface, | ||
| self.patient_image, | ||
| ) | ||
| patient_roi = self._auto_generate_roi_mask(patient_mask) | ||
|
|
||
| self.registrar_Greedy.set_fixed_image(self.patient_image) | ||
| self.registrar_Greedy.set_fixed_mask(patient_roi) | ||
| self.registrar_Greedy.set_fixed_mask(patient_mask) | ||
|
|
Comment on lines
+97
to
+99
| **Labelmap Configuration:** | ||
| Labelmaps are automatically generated from models if not provided by the user | ||
| via set_labelmaps(). Auto-generated labelmaps use labelmap_dilation_mm parameter. |
|
|
||
| Key Features: | ||
| - Automatic mask generation if not provided by user | ||
| - Automatic labelmap generation if not provided by user |
Comment on lines
19
to
21
| def test_transform_model_applies_staged_transform() -> None: | ||
| """Transform helper updates mesh points with image shape (Z, Y, X) = (3, 3, 3).""" | ||
| image = itk.image_from_array(np.zeros((3, 3, 3), dtype=np.float32)) |
Comment on lines
293
to
+296
| size = reference_image.GetLargestPossibleRegion().GetSize() | ||
| size = (size[2], size[1], size[0]) | ||
|
|
||
| tmp_arr = np.zeros(size, dtype=np.int32) | ||
| # NumPy convention is (z, y, x); ITK GetSize() returns (x, y, z) | ||
| tmp_arr = np.zeros((size[2], size[1], size[0]), dtype=np.int32) |
Comment on lines
+273
to
+277
| [[tool.mypy.overrides]] | ||
| # torch/icon_registration/unigradicon are lazy-loaded at runtime; string | ||
| # forward-reference annotations like "torch.Size" are intentional. | ||
| module = ["physiomotion4d.register_images_icon"] | ||
| ignore_errors = true |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/physiomotion4d/register_models_distance_maps.py:237
- The
register()method docstring still says “mask-based registration”, but the implementation now registers distance maps and uses binary masks only as ROI constraints. Updating the wording will keep docs consistent with the new behavior.
def register(
self,
transform_type: str = "Deformable",
icon_iterations: int = 50,
) -> dict:
"""Perform mask-based registration of moving model to fixed model.
Comment on lines
+365
to
+368
| try: | ||
| import cupy as cp # noqa: PLC0415 | ||
| except ImportError: | ||
| cp = None |
Comment on lines
+750
to
753
| patient_mask = self.contour_tools.create_mask_from_mesh( | ||
| self.patient_model_surface, | ||
| self.patient_image, | ||
| ) |
Comment on lines
+97
to
+99
| **Labelmap Configuration:** | ||
| Labelmaps are automatically generated from models if not provided by the user | ||
| via set_labelmaps(). Auto-generated labelmaps use labelmap_dilation_mm parameter. |
Comment on lines
+300
to
+307
| def set_labelmap_dilation_mm(self, labelmap_dilation_mm: float) -> None: | ||
| """Set dilation amount for auto-generated labelmaps. | ||
|
|
||
| Uses self.roi_dilation_mm for dilation amount. | ||
|
|
||
| Note: | ||
| Requires masks to exist (auto-generated or user-provided). | ||
| Args: | ||
| labelmap_dilation_mm: Dilation amount in millimeters applied when | ||
| auto-generating multi-label labelmaps from models. Default: 0mm | ||
| """ | ||
| self.log_info( | ||
| f"Auto-generating ROI masks (dilation: {self.roi_dilation_mm}mm)..." | ||
| ) | ||
|
|
||
| if dilate_mm is None: | ||
| dilate_mm = self.roi_dilation_mm | ||
|
|
||
| # Generate model ROI mask | ||
| roi = None | ||
| if dilate_mm > 0: | ||
| roi = self.labelmap_tools.convert_labelmap_to_mask( | ||
| mask, dilation_in_mm=dilate_mm | ||
| ) | ||
| else: | ||
| roi = mask | ||
|
|
||
| self.log_info("ROI masks auto-generated successfully.") | ||
| return roi | ||
| self.labelmap_dilation_mm = labelmap_dilation_mm |
Comment on lines
+261
to
+268
| labelmap_arr = np.zeros( | ||
| ( | ||
| reference_image.GetLargestPossibleRegion().GetSize()[2], | ||
| reference_image.GetLargestPossibleRegion().GetSize()[1], | ||
| reference_image.GetLargestPossibleRegion().GetSize()[0], | ||
| ), | ||
| dtype=np.uint8, | ||
| ) |
Comment on lines
293
to
297
| size = reference_image.GetLargestPossibleRegion().GetSize() | ||
| size = (size[2], size[1], size[0]) | ||
|
|
||
| tmp_arr = np.zeros(size, dtype=np.int32) | ||
| # NumPy convention is (z, y, x); ITK GetSize() returns (x, y, z) | ||
| tmp_arr = np.zeros((size[2], size[1], size[0]), dtype=np.int32) | ||
| itk_point = itk.Point[itk.D, 3]() |
| @@ -140,17 +143,24 @@ def create_distance_map( | |||
| """ | |||
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.
Four bugs fixed:
NumPy array shape wrong in create_distance_map (contour_tools.py) ITK GetSize() returns (Nx, Ny, Nz) but NumPy needs (Nz, Ny, Nx) ordering. np.zeros(size) was creating the wrong shape, causing an IndexError when ix >= size[2].
TubeTK ResampleImage crashes ~80% on Windows (workflow_fit_statistical_model_to_patient.py) ITK's lazy SWIG DLL loader for TubeTK is intermittently unstable on Windows. Replace TubeTK.ResampleImage.SetMakeHighResIso() with a new _make_isotropic_image() helper using standard ITK ResampleImageFilter
GPU libraries initialized at import time conflict with TubeTK DLL loading Several modules imported CUDA-initializing libraries at module level, which interfered with TubeTK's DLL initialization on Windows:
import cupy(which initializes CUDA at import time) replaced with importlib.util.find_spec("cupy") existence checkimport cupymoved inside the one method that uses it for GPU-accelerated norm computationITK lazy-load reference dropped before .New() (workflow_fit_statistical_model_to_patient.py) ITK 6.0b2's getattr returns a temporary that can be GC'd before .New() completes. Fixed by storing the class reference before calling .New() (now superseded by the TubeTK replacement in bug ENH: Use logging in project classes. BUG: Fixed tests. #2).
Summary by CodeRabbit
Release Notes
Documentation
Improvements
mask_dilation_mm(replacing ROI/mask-to-mask terminology) and adjusted registered labelmap export to match the enabled stage.Tests