BUG:ACCESS_VIOLATION crashes and NumPy shape bug in 4D CT pipeline#68
Open
aylward wants to merge 2 commits into
Open
BUG:ACCESS_VIOLATION crashes and NumPy shape bug in 4D CT pipeline#68aylward wants to merge 2 commits into
aylward wants to merge 2 commits into
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: | ||
| """ |
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 + 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 → 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 warning)
✅ 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.55% -0.36%
==========================================
Files 53 53
Lines 7229 7225 -4
==========================================
- Hits 2379 2352 -27
- Misses 4850 4873 +23
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
+243
to
+246
| if args.use_labelmap_to_labelmap: | ||
| workflow.set_use_labelmap_to_labelmap_registration( | ||
| args.use_labelmap_to_labelmap | ||
| ) |
There was a problem hiding this comment.
Always wire the L2L flag, including the disabled case.
When --no-labelmap-to-labelmap is passed, args.use_labelmap_to_labelmap is False, so this if block is skipped and the workflow keeps its default True. The CLI flag therefore has no effect.
Suggested fix
- if args.use_labelmap_to_labelmap:
- workflow.set_use_labelmap_to_labelmap_registration(
- args.use_labelmap_to_labelmap
- )
+ workflow.set_use_labelmap_to_labelmap_registration(
+ args.use_labelmap_to_labelmap
+ )🤖 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/fit_statistical_model_to_patient.py` around lines 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.
Comment on lines
+289
to
+299
| if workflow.l2i_template_labelmap is not None: | ||
| output_labelmap_file = os.path.join( | ||
| args.output_dir, f"{args.output_prefix}_labelmap.nii.gz" | ||
| ) | ||
| itk.imwrite(workflow.m2i_template_labelmap, output_labelmap_file) | ||
| itk.imwrite(workflow.l2i_template_labelmap, output_labelmap_file) | ||
| print(f" Registered labelmap: {output_labelmap_file}") | ||
| elif workflow.m2m_template_labelmap is not None: | ||
| elif workflow.l2l_template_labelmap is not None: | ||
| output_labelmap_file = os.path.join( | ||
| args.output_dir, f"{args.output_prefix}_labelmap.nii.gz" | ||
| ) | ||
| itk.imwrite(workflow.m2m_template_labelmap, output_labelmap_file) | ||
| itk.imwrite(workflow.l2l_template_labelmap, output_labelmap_file) |
There was a problem hiding this comment.
Write labelmaps with ITK compression enabled.
The new registered labelmap exports omit compression=True. As per coding guidelines, images must be “stored using itk.imwrite with compression=True.”
Suggested fix
- itk.imwrite(workflow.l2i_template_labelmap, output_labelmap_file)
+ itk.imwrite(
+ workflow.l2i_template_labelmap,
+ output_labelmap_file,
+ compression=True,
+ )
@@
- itk.imwrite(workflow.l2l_template_labelmap, output_labelmap_file)
+ itk.imwrite(
+ workflow.l2l_template_labelmap,
+ output_labelmap_file,
+ compression=True,
+ )🤖 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/fit_statistical_model_to_patient.py` around lines 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.
Source: Coding guidelines
Comment on lines
40
to
43
| 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 |
There was a problem hiding this comment.
Remove the stale output_file argument from the docstring.
extract_contours() no longer accepts output_file, so the Args block documents a non-existent parameter.
Proposed docstring cleanup
Args:
labelmap_image (itk.image): The labelmap image to create contours from
- output_file (str, optional): If provided, save the contours to this VTP
- file📝 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
| 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 | |
| Args: | |
| labelmap_image (itk.image): The labelmap image to create contours from |
🤖 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/contour_tools.py` around lines 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.
Comment on lines
+365
to
+373
| try: | ||
| import cupy as cp # noqa: PLC0415 | ||
|
|
||
| new_pnts_cp = cp.array(new_pnts) | ||
| pnts_cp = cp.array(pnts) | ||
| new_mesh.point_data["DeformationMagnitude"] = cp.linalg.norm( | ||
| new_pnts_cp - pnts_cp, axis=1 | ||
| ).get() | ||
| else: | ||
| except (ImportError, OSError): |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For CuPy v13.x, what exception type is raised when CUDA runtime/driver is unavailable during array creation (e.g., cp.array)?
💡 Result:
In CuPy v13.x, when CUDA runtime or driver is unavailable during array creation (such as calling cp.array), the exception type raised is cupy_backends.cuda.api.runtime.CUDARuntimeError [1][2][3]. This error typically occurs when CuPy attempts to interact with the CUDA runtime or driver to allocate memory or initialize a device, but finds the environment inadequately configured—such as when no CUDA-capable device is detected (cudaErrorNoDevice) or the driver version is insufficient for the installed CUDA runtime (cudaErrorInsufficientDriver) [1][2][4]. While CuPy v13 introduced lazy loading of CUDA libraries to allow import cupy to succeed even in environments without CUDA [5], executing operations that require the GPU (like cp.array) will trigger a check against the underlying CUDA driver/runtime, resulting in a CUDARuntimeError if that communication fails [2][3]. Note that in some edge cases or specific compilation paths, such as when using RawKernel or RawModule, CuPy may also raise cupy.cuda.compiler.CompileException if it fails to locate the necessary CUDA toolkit components [6][7][8].
Citations:
- 1: cupy-cuda11x==13.2.0 fails to detect GPU cupy/cupy#8395
- 2:
CUDARuntimeErrorraised on first CuPy call in session when no GPU is present; second call returns expectedFalsecupy/cupy#9091 - 3: CUDA-capable device detection takes over 10 seconds cupy/cupy#8152
- 4: CUDA driver version is insufficient for CUDA runtime version cupy/cupy#9252
- 5: https://medium.com/cupy-team/announcing-cupy-v13-66979ee7fab0
- 6: https://docs.cupy.dev/en/v13.6.0/install.html
- 7: https://docs.cupy.dev/en/v13.1.0/install.html
- 8: [FEA]: CuPy fails to auto-detect CUDA when CUDA_PATH is unset in containers NVIDIA/cuda-python#1707
🏁 Script executed:
cat -n src/physiomotion4d/transform_tools.py | sed -n '360,380p'Repository: Project-MONAI/physiomotion4d
Length of output: 939
🏁 Script executed:
python3 << 'PY'
# Check what CuPy version would be used and verify import paths
import sys
# Try to determine the correct import path for CUDARuntimeError
# based on CuPy's public API structure
# Check common import paths
import_paths = [
"cupy.cuda.runtime.CUDARuntimeError",
"cupy_backends.cuda.api.runtime.CUDARuntimeError",
"cupy.cuda.CUDARuntimeError",
]
print("Common CuPy exception import paths to check:")
for path in import_paths:
print(f" - {path}")
# The web search result mentioned cupy_backends.cuda.api.runtime.CUDARuntimeError
# but the proposed fix uses cupy.cuda.runtime.CUDARuntimeError
# This should be verified in documentation or the actual library structure
PYRepository: Project-MONAI/physiomotion4d
Length of output: 239
🏁 Script executed:
rg "CUDARuntimeError|cupy.*import" src/physiomotion4d/ -A 2 -B 2Repository: Project-MONAI/physiomotion4d
Length of output: 54
🏁 Script executed:
# Check CuPy documentation or any hints in the codebase about exception handling
rg "cupy" src/physiomotion4d/transform_tools.py -B 3 -A 3Repository: Project-MONAI/physiomotion4d
Length of output: 271
Handle CuPy runtime failures, not just import failures.
On Line 373, the exception handler only catches ImportError and OSError. CuPy v13.x supports lazy loading, so import cupy succeeds even without CUDA available. However, operations like cp.array() and cp.linalg.norm() will raise CUDARuntimeError if the CUDA runtime is unavailable or misconfigured, bypassing the NumPy fallback and crashing the deformation magnitude computation.
Separate the import attempt from the operation attempt, catching cupy_backends.cuda.api.runtime.CUDARuntimeError for the GPU operations:
Proposed fix
if with_deformation_magnitude:
try:
import cupy as cp # noqa: PLC0415
+ from cupy_backends.cuda.api.runtime import CUDARuntimeError # noqa: PLC0415
+ except (ImportError, OSError):
+ new_mesh.point_data["DeformationMagnitude"] = np.linalg.norm(
+ np.asarray(new_pnts) - np.asarray(pnts), axis=1
+ )
+ else:
+ try:
new_pnts_cp = cp.array(new_pnts)
pnts_cp = cp.array(pnts)
new_mesh.point_data["DeformationMagnitude"] = cp.linalg.norm(
new_pnts_cp - pnts_cp, axis=1
).get()
- except (ImportError, OSError):
+ except CUDARuntimeError:
new_mesh.point_data["DeformationMagnitude"] = np.linalg.norm(
np.asarray(new_pnts) - np.asarray(pnts), axis=1
)🤖 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/transform_tools.py` around lines 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.
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.
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