ENH: Simplify heart models for inter-patient consistency#31
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances the heart modeling workflows to support using full UnstructuredGrid models in addition to surfaces, improving inter-patient consistency in statistical shape modeling. The changes add a new pca_uses_surface parameter to control whether PCA registration operates on surfaces or full meshes, refactor the Simpleware segmentation trimming functionality, and update type annotations throughout to support both PolyData and UnstructuredGrid mesh types.
Changes:
- Added support for operating on full UnstructuredGrid models alongside surface-only (PolyData) approaches in the PCA registration workflow
- Refactored Simpleware heart segmentation to extract the mask trimming logic into a separate method with enhanced morphological operations
- Updated type annotations across transform and contour tools to accept both PolyData and UnstructuredGrid types
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow_fit_statistical_model_to_patient.py | Added pca_uses_surface parameter, new icp_template_model and pca_template_model attributes to store full UnstructuredGrid models, and logic to handle both surface and volume mesh registration modes |
| workflow_create_statistical_model.py | Added reduce_reference_to_surface parameter to optionally skip surface extraction and work with full meshes |
| transform_tools.py | Updated transform_pvcontour type signatures to accept and return both PolyData and UnstructuredGrid |
| segment_heart_simpleware.py | Renamed set_trim_mesh_to_essentials to set_trim_mask_to_essentials, extracted trimming logic into new trim_mask_to_essentials method with complex morphological operations |
| contour_tools.py | Updated type signatures to support UnstructuredGrid in addition to PolyData, added logic to extract surfaces from UnstructuredGrid when needed |
Comments suppressed due to low confidence (2)
src/physiomotion4d/workflow_fit_statistical_model_to_patient.py:497
- The docstring for the
register_model_to_model_pcamethod is incomplete and doesn't document the newly addedpca_uses_surfaceparameter. The docstring should include an Args section documenting what this parameter does and when to use True vs False.
def register_model_to_model_pca(self, pca_uses_surface: bool = True) -> dict:
"""Perform PCA-based registration after ICP alignment.
Uses RegisterModelsPCA class for intensity-based PCA registration.
This method requires PCA data to be set via set_pca_data().
Returns:
dict: Dictionary containing:
- 'forward_point_transform': Rigid transform from PCA registration
- 'pca_coefficients': PCA shape coefficients
- 'registered_template_model_surface': PCA-registered model surface
Raises:
ValueError: If PCA data has not been set
"""
src/physiomotion4d/workflow_fit_statistical_model_to_patient.py:882
- The docstring for the
run_workflowmethod should document the newly addedpca_uses_surfaceparameter on line 859. Currently, the Args section only documents the other three parameters but omits this new one.
"""Execute the complete multi-stage registration workflow.
Runs registration stages in sequence:
1. ICP alignment (RegisterModelsICP)
2. PCA registration (PCA data was provided)
3. Mask-to-mask deformable registration (RegisterModelsDistanceMaps)
4. Optional mask-to-image refinement (Icon); requires template labelmap and IDs
set via set_use_mask_to_image_registration(True, ...).
Args:
use_mask_to_image_registration: Whether to include mask-to-image
registration stage. Default: False. When True, template labelmap and
label IDs must have been set via set_use_mask_to_image_registration(True, ...).
use_mask_to_mask_registration: Whether to include mask-to-mask registration
stage. Default: True
use_icon_registration_refinement: Whether to include icon registration
refinement stage. Default: False
Returns:
dict with registered_template_model and registered_template_model_surface
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| itk.imwrite(tfm_x_img, "pca_forward_point_transform_x.nii.gz") | ||
| itk.imwrite(tfm_y_img, "pca_forward_point_transform_y.nii.gz") | ||
| itk.imwrite(tfm_z_img, "pca_forward_point_transform_z.nii.gz") |
There was a problem hiding this comment.
Debug code writes files to the current working directory without specifying a path, which could clutter the working directory. Consider writing to a dedicated debug output directory or using tempfile for temporary debug outputs. If these files are intended to be kept, they should be written to a configurable output directory.
| self.pca_coefficients: Optional[np.ndarray] = None | ||
| self.pca_template_model: Optional[pv.UnstructuredGrid] = None | ||
| self.pca_template_model_surface: Optional[pv.PolyData] = None | ||
| self.pca_template_labelmap: Optional[itk.Image] = None |
There was a problem hiding this comment.
The attribute pca_uses_surface is set in the set_use_pca_registration method but is not initialized in __init__. If register_model_to_model_pca is called before set_use_pca_registration, it will raise an AttributeError when trying to access self.pca_uses_surface on line 527. Initialize this attribute in __init__ with a default value (e.g., self.pca_uses_surface = True).
| self.pca_template_labelmap: Optional[itk.Image] = None | |
| self.pca_template_labelmap: Optional[itk.Image] = None | |
| self.pca_uses_surface: bool = True |
No description provided.