ENH: Registration fixes - #118
Merged
Merged
Conversation
…tive
RegisterModelsPCA previously maximized mean intensity sampled from the fixed
distance map through ITK's LinearInterpolateImageFunction, with the optimizer
estimating gradients by finite differences. The objective is now stated and
minimized directly:
mean distance(model -> target)
+ w * mean distance(target -> model) # symmetric term
+ lambda * sum(b_i^2) # Mahalanobis shape prior
Because the PCA deformation is linear in the coefficients b, the gradient is
analytic and handed to the optimizer instead of being estimated, which removes
one objective evaluation per coefficient per step. The post-PCA transform is
folded into the mode directions so the gradient stays exact; when that
transform is not affine its Jacobian is not constant, so the analytic gradient
is disabled and finite differences are used with a logged warning.
- Add symmetric_weight (default 0.5) so partial target coverage is penalized,
and pca_prior_weight (default 0.0, disabled) for the Mahalanobis prior
- Sample the distance map and its gradient with scipy.ndimage.map_coordinates,
and build the target-to-model term with a scipy.spatial.cKDTree
- Replace the cached ITK interpolator and _create_itk_points with
_prepare_sampling, which builds the arrays the objective is made of once
- Add ContourTools.sample_mesh_faces for face-density-aware point sampling and
a negative_inside option on the signed distance map
- Log transform fidelity after computing the PCA transforms
Tests cover the pieces that were previously unverified: the analytic gradient
against finite differences, recovery of known coefficients, the symmetric term
penalizing partial coverage, the prior shrinking coefficients, eigenvector
scaling by standard deviation, deformation happening in the template frame,
and a transform round trip.
…eding Greedy reports its affine in RAS while ITK is LPS, but the 4x4 was copied straight into an itk.AffineTransform, negating x and y. Recovering a known (6, -4, 3) mm shift returned (+5.96, -4.01, -2.92) instead of (-6, +4, -3), and warping by the result scored below the unregistered pair (foreground NCC 0.21 vs 0.32). Rigid, Affine and Deformable were all affected; the displacement field was already LPS and is left alone. Existing Greedy tests only asserted the transforms were non-None, so the sign error survived. - Change basis in RegisterImagesGreedy._matrix_to_itk_affine - Add known-shift accuracy tests for Greedy, ANTs and ICON (a KnownShiftCase helper in conftest); ANTs and ICON were audited and are correct - Replace per-backend initial_forward_transform handling, which pre-warped in ANTs, pre-warped only the image in ICON, and double-applied in Greedy, with one RegisterImagesBase.register_from() - Add TransformTools.invert_transform, preferring the analytic inverse over a displacement field that is only defined on the reference grid - Remove prior_weight from RegisterTimeSeriesImages, the reconstruction workflow and its CLI flag - Offer explicit Rigid/Similarity/Affine modes in RegisterModelsICP, with bounding-box scale estimation before ICP - Add ImageTools.pad_image; drop icon_iterations from RegisterModelsDistanceMaps.register() - Fix PhaseSampleDataset caching (0 now means unbounded, as documented) and include Case8Deploy in tutorial 09's case discovery - Regenerate registration_time_series_images baselines
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR updates registration APIs and model-fitting logic, expands VTK-to-USD surface metadata handling, adds a lung ICON fine-tuning tutorial, renames PCA options, removes temporal prior weighting, and replaces automated experiment tests with tutorial-focused testing guidance. ChangesRegistration and model fitting
Surface conversion and tutorials
Testing and experiment execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR fixes several registration correctness issues (notably Greedy’s RAS↔LPS affine basis mismatch) and standardizes how registrations are seeded across backends by introducing a single RegisterImagesBase.register_from() composition path. It also expands registration validation with “known shift” accuracy tests, improves PCA/ICP model-registration behavior, and removes prior_weight-based time-series smoothing in favor of independent per-frame registration.
Changes:
- Fix Greedy affine conversion by changing basis from Greedy’s RAS convention into the project-wide ITK LPS convention, and add known-shift accuracy tests for Greedy/ANTs/ICON.
- Introduce
RegisterImagesBase.register_from()to pre-warp + refine + compose consistently across backends; update chain/backends/tests/docs accordingly. - Enhance model registration tooling (PCA objective/gradient, ICP modes including Similarity, distance-map registration knobs), and remove
prior_weightfrom time-series workflows/CLI/tests with regenerated baselines.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_09_lung_train_physicsnemo_mgn.py | Clarifies dataset splitting/validation behavior; logs skipped case reasons; includes Case8Deploy discovery. |
| tutorials/tutorial_07_lung_fit_statistical_model_to_patient.py | Makes PCA mode count explicit, adjusts mask dilation, and saves an additional PCA-registered surface artifact. |
| tutorials/tutorial_06_lung_create_statistical_model.py | Renames PCA mode variable and changes how many modes are visualized/saved. |
| tests/test_register_time_series_images.py | Removes prior_weight usage and updates a test scenario/name and output artifacts. |
| tests/test_register_models_pca.py | Adds extensive PCA registrar unit tests (mode validation, deformation math, gradients, priors, symmetric term, transform fidelity). |
| tests/test_register_images_icon.py | Adds known-shift recovery test; updates seeding test to use register_from(). |
| tests/test_register_images_greedy.py | Adds known-shift recovery test across transform types; regression guard for RAS/LPS sign errors. |
| tests/test_register_images_chain.py | Updates chain behavior assertions to validate pre-warp + composed transform behavior numerically. |
| tests/test_register_images_ants.py | Adds known-shift recovery test; migrates initial transform tests to register_from(). |
| tests/conftest.py | Adds KnownShiftCase helper + session fixture for deterministic translation-based accuracy tests. |
| tests/baselines/registration_time_series_images/transform_application_time_series_0.mha | Updates baseline pointer after time-series registration behavior change. |
| tests/baselines/registration_time_series_images/prior_time_series_registered_0.mha | Removes obsolete baseline (prior-weight path removed). |
| tests/baselines/registration_time_series_images/prior_forward_transform_0.hdf | Removes obsolete baseline (prior-weight path removed). |
| tests/baselines/registration_time_series_images/middle_frame_forward_transform_0.hdf | Adds new baseline artifact for “middle reference frame” test. |
| tests/baselines/registration_time_series_images/basic_time_series_registered_0.mha | Updates baseline pointer after registration behavior change. |
| tests/baselines/registration_time_series_images/basic_forward_transform_0.hdf | Updates baseline pointer after registration behavior change. |
| src/physiotwin4d/workflow_reconstruct_highres_4d_ct.py | Removes prior_weight parameter/plumbing and associated logging. |
| src/physiotwin4d/workflow_fit_statistical_model_to_patient.py | Fixes PCA frame handling (post-PCA transform usage) and refines labelmap/model transform composition; pads patient image for distance-map registration. |
| src/physiotwin4d/transform_tools.py | Adds generic invert_transform() (analytic when possible; fallback to field inversion) and tightens displacement-field inversion controls. |
| src/physiotwin4d/train_physicsnemo_mgn.py | Adds processor gradient-checkpoint segment configuration to trade compute for GPU memory. |
| src/physiotwin4d/register_time_series_images.py | Removes prior-based smoothing/selection logic; registers frames independently; updates docs/notes. |
| src/physiotwin4d/register_models_pca.py | Overhauls PCA registration objective (distance-map minimization), adds symmetric term + priors, analytic gradient, and improves transform/field fidelity reporting. |
| src/physiotwin4d/register_models_icp.py | Adds explicit Rigid/Similarity/Affine pipelines with bounding-box scaling and refactors ICP staging. |
| src/physiotwin4d/register_models_distance_maps.py | Adds distance normalization knob and changes mask creation behavior; removes ICON iteration parameter from the API. |
| src/physiotwin4d/register_images_icon.py | Removes backend-specific initial-transform pre-warp/composition (now centralized in register_from()). |
| src/physiotwin4d/register_images_greedy.py | Fixes RAS→LPS affine conversion; removes backend-specific initial-transform handling; clarifies deformable seeding. |
| src/physiotwin4d/register_images_chain.py | Updates chaining semantics to pre-warp and refine via register_from() composition rather than passing an “initial transform” into backends. |
| src/physiotwin4d/register_images_base.py | Removes initial_forward_transform from the backend method contracts and introduces register_from() + shared pre-warp/composition helpers. |
| src/physiotwin4d/register_images_ants.py | Removes backend-specific initial-transform handling; updates examples to register_from(). |
| src/physiotwin4d/physicsnemo_tools.py | Fixes caching semantics so _cache_max_samples == 0 means unbounded (as documented). |
| src/physiotwin4d/image_tools.py | Adds ImageTools.pad_image() with correct origin handling and flexible per-axis padding specification. |
| src/physiotwin4d/contour_tools.py | Adds face sampling for mesh rasterization; updates distance-map creation to optionally use face samples for smoother maps. |
| src/physiotwin4d/cli/reconstruct_highres_4d_ct.py | Removes --prior-weight CLI flag and associated validation/plumbing. |
| experiments/Heart-Create_Statistical_Model/README.md | Updates guidance for adjusting ICON iterations now that icon_iterations param was removed. |
| docs/developer/registration_images.rst | Documents seeding via register_from() and updates chain description accordingly. |
Suppressed comments (1)
src/physiotwin4d/register_models_distance_maps.py:258
- Same issue as the fixed-side debug writes: moving_mask_image can be None when mask_dilation_mm <= 0, so itk.imwrite(self.moving_mask_image, ...) will raise. Debug file writes should be gated/optional and avoid writing None images.
itk.imwrite(
self.moving_mask_image, "debug_moving_mask_image.nii.gz", compression=True
)
itk.imwrite(
self.moving_distance_map_image,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
+ Coverage 36.60% 42.18% +5.58%
==========================================
Files 72 72
Lines 8510 8742 +232
==========================================
+ Hits 3115 3688 +573
+ Misses 5395 5054 -341
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:
|
1. Per-structure USD export ENH: Name USD prims per structure and pick materials from those names save_combined_surfaces now tags each cell with a SegmentationLabelIds array so structure identity survives the merge, and ConvertVTKToUSD splits on that array in addition to boundary_labels. ConvertVTKToUSD gains object_names for the static-merge layout; WorkflowConvertVTKToUSD derives them from each mesh's SegmentationLabelNames. anatomy_type now defaults to None, which resolves a material per prim from the prim name, falling back to the object's AnatomyGroup and then to "other" -- so ventricle_left, myocardium, and the great vessels each get their own look instead of one shared heart material. Adds USDAnatomyTools.resolve_anatomy_type so callers can test a name before applying it rather than catching ValueError. 2. Composite transform flattening (bug) BUG: Splice nested composites when chaining registration transforms itk.HDF5TransformIO refuses to write a CompositeTransform holding another CompositeTransform, which every multi-stage registration produced: RegisterImagesGreedy returns an affine+warp composite, and composing a residual onto it nested that composite. _add_transform_flattened splices sub-transforms in at the position their composite occupied, leaving the mapping unchanged since CompositeTransform applies its queue back to front either way. 3. Retire experiment test harness ENH: Drop the experiment test harness; tutorials are the e2e suite Removes tests/test_experiments.py, the "experiment" marker, and --run-experiments. Experiment scripts are exploratory: they assume interactive display, full-resolution parameters, and data layouts that only exist on the author's machine. The test-mode branches in those scripts go away with the harness that drove them, and experiments/ is omitted from coverage. tests/test_tutorials.py --run-tutorials is now the only end-to-end suite; CI comments and READMEs point there. 4. PCA parameter rename (breaking) ENH: Rename PCA count parameters to number_of_pca_components pca_number_of_components, pca_number_of_modes, and --pca-components / --pca-number-of-modes all become number_of_pca_components, so the workflows, the CLIs, and the docs use one name. Breaking change to both the Python and the CLI interface. 5. Distance-map finetuning tutorial ENH: Add tutorial 2 finetuning uniGradICON on lung distance maps RegisterModelsDistanceMaps feeds ICON rasterized signed squared distance maps, not CT intensities, so stock uniGradICON is out of distribution for that stage. The new tutorial finetunes on exactly that representation using DIR-Lab 4D CT lung segmentations, holding Case 1 out for landmark TRE and Dice evaluation. RegisterModelsDistanceMaps.set_icon_weights_path and WorkflowFitStatisticalModelToPatient .set_labelmap_to_labelmap_icon_weights_path plumb the resulting checkpoint into the labelmap-to-labelmap stage; the labelmap-to-image stage keeps stock weights since it registers the image itself.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tutorials/tutorial_06_lung_create_statistical_model.py:171
mode_countis currently set tonumber_of_pca_componentswithout checking how many PCA components/eigenvalues were actually produced. If the workflow reduced components due to limited sample count, the loop will raise IndexError when indexingeigenvalues[mode_idx]/components[mode_idx]. Cap the loop by the available lengths.
src/physiotwin4d/register_models_distance_maps.py:233- These debug
itk.imwrite(...)calls will crash whenmask_dilation_mm <= 0becauseself.fixed_mask_imageis None, and they also write large files to the current working directory unconditionally. Gate them behind debug logging and only write the mask if it exists.
src/physiotwin4d/register_models_distance_maps.py:274 - Same issue as the fixed-mask debug write above:
self.moving_mask_imagecan be None (whenmask_dilation_mm <= 0), but it is still passed toitk.imwrite, which will error. Also consider keeping these writes debug-only to avoid unexpected I/O during normal library use.
tests/test_workflow_convert_vtk_to_usd.py:20 - Type hints in this repo use Optional[T] rather than T | None. Using
str | Nonehere violates the project typing convention (and strict mypy config), and can be fixed by switching to Optional[str] and importing Optional.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/physiotwin4d/register_models_distance_maps.py (1)
126-139: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
distance_squared_maxin the__init__Args.The new constructor parameter appears in the class Attributes list at line 89 but not in the
__init__Args block. State its meaning and the unit, since callers derive it from the dilation radius, for example(1.25 * mask_dilation_mm) ** 2inworkflow_fit_statistical_model_to_patient.py.📝 Proposed docstring addition
reference_image: ITK image providing coordinate frame (origin, spacing, direction) for mask generation. Typically the patient CT/MRI image. + distance_squared_max: Squared distance in mm^2 used to normalize the + signed distance maps to [-1, 1]. Default: 50.0 mask_dilation_mm: Dilation amount in millimeters for binary registration - mask generation. Default: 20mm + mask generation. Pass 0 or a negative value to skip mask + generation entirely. Default: 20mm🤖 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/physiotwin4d/register_models_distance_maps.py` around lines 126 - 139, Update the constructor docstring’s Args section for __init__ to document distance_squared_max, describing it as the maximum squared distance threshold and specifying that its unit is squared millimeters.Source: Coding guidelines
src/physiotwin4d/register_time_series_images.py (1)
26-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale prior-propagation claims.
Prior-transform initialization is gone. Three statements still describe it:
- Line 28-29: "It can propagate information from prior registrations to initialize subsequent ones."
- Line 36-37: "This bidirectional approach helps maintain temporal coherence in the registration results."
- Lines 171-173: "the method can optionally use the transform from the previous image to initialize the registration, which can improve convergence and temporal coherence."
The Note at lines 208-209 now states the opposite. Align the class docstring and the method docstring with the independent-per-frame behavior.
📝 Proposed docstring fix
This class extends RegisterImagesBase to provide sequential registration of multiple images (time series) to a fixed image, using a - caller-supplied registration backend. It can propagate information from - prior registrations to initialize subsequent ones. + caller-supplied registration backend. Every frame is registered + independently of the others. The registration proceeds in two passes from a reference frame: 1. Forward pass: from reference_frame to the end of the series 2. Backward pass: from reference_frame-1 to the beginning - - This bidirectional approach helps maintain temporal coherence in the - registration results.This method registers an ordered sequence of images to a common fixed frame. Registration proceeds bidirectionally from a reference frame: forward to the end and backward to the beginning. - For each image after the reference image, the method can optionally use - the transform from the previous image to initialize the registration, - which can improve convergence and temporal coherence. + Each image is registered independently of the others.Also applies to: 171-173
🤖 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/physiotwin4d/register_time_series_images.py` around lines 26 - 45, Update the class docstring and the affected method docstring in the time-series registration implementation to remove claims about propagating or reusing prior transforms, initialization benefits, and temporal-coherence guarantees. Keep the documentation aligned with the current independent-per-frame behavior and the existing note that states prior-transform initialization is not used.Source: Coding guidelines
src/physiotwin4d/register_images_ants.py (1)
537-555: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale initial-transform documentation.
registration_methodno longer accepts or composes an initial transform. The Note block still states that this method pre-warps the moving image with the initial transform and composes it with the registration result. Line 555 also claims that initial transforms are converted from ITK to ANTs format automatically, but the call now passesinitial_transform=["identity"]. Point the reader toRegisterImagesBase.register_frominstead.📝 Proposed docstring fix
Note: For SyN registration, the transformations are approximately inverse consistent. The forward and inverse transforms are stored separately by ANTs. - IMPORTANT: the initial transform is applied by pre-warping the - moving image onto the fixed grid (the same approach as - RegisterImagesICON) rather than via ants.registration's - initial_transform argument, which mishandles matrix (affine/ - translation) initials. This method composes the initial transform - with the registration result, so the returned transforms include - both the initial alignment and the registration refinement. + To seed the registration from a known alignment, call + :meth:`RegisterImagesBase.register_from`. This method always runs + ANTs from identity. Implementation details: - Uses ANTs registration with configurable transform types - Supports multi-resolution optimization - Handles masked and unmasked registration - Returns ITK-compatible displacement field transforms - - Initial transforms are converted from ITK to ANTs format automatically🤖 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/physiotwin4d/register_images_ants.py` around lines 537 - 555, Remove the stale initial-transform claims from the docstring for registration_method, including the Note text about pre-warping/composition and the implementation-detail bullet about ITK-to-ANTs conversion. Replace them with a brief reference directing readers to RegisterImagesBase.register_from for initial-transform handling, while preserving the remaining registration behavior documentation.Source: Coding guidelines
🟡 Minor comments (13)
.agents/agents/testing.md-32-32 (1)
32-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap the modified Markdown lines to 88 characters or fewer.
.agents/agents/testing.md#L32-L32: move the long inline CI explanation to a separate wrapped comment line.CLAUDE.md#L134-L134: wrap the fixture-chain description onto a continuation line.statistics.md#L155-L155: wrap the tutorial marker description onto a continuation line.🤖 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 @.agents/agents/testing.md at line 32, Wrap the modified Markdown lines to 88 characters or fewer: in .agents/agents/testing.md lines 32-32, move the inline CI explanation to a separate wrapped comment line; in CLAUDE.md lines 134-134, place the fixture-chain description on a continuation line; and in statistics.md lines 155-155, place the tutorial marker description on a continuation line.Source: Coding guidelines
tests/conftest.py-615-620 (1)
615-620: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the coordinate-frame reference from this docstring.
Describe the expected displacement and sign-check behavior without naming the
RAS/LPS convention. The repository rule prohibits restating fixed
coordinate-frame conventions in docstrings.As per coding guidelines, "Do not restate fixed ITK shape, axis-order, or LPS
conventions in docstrings, comments, or test docstrings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 615 - 620, Update the docstring near the moving/fixed resampling explanation to describe the expected displacement and sign-check behavior without mentioning RAS/LPS or any fixed coordinate-frame convention. Preserve the explanation that the transform uses the negated shift and validates absolute accuracy.Source: Coding guidelines
tests/README.md-143-143 (1)
143-143: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap this line to 88 characters or fewer.
Line 143 exceeds the repository line-length limit for Markdown files.
As per coding guidelines, "keep lines at or below 88 characters."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/README.md` at line 143, Wrap the timing-report sentence in tests/README.md so each Markdown line is no longer than 88 characters, preserving the existing wording and meaning.Source: Coding guidelines
experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py-61-67 (1)
61-67: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winExit early when the file dialog is canceled.
If
filedialog.askopenfilename()returns an empty path,input_image_pathis not selected. Add a guard before the lateritk.imread(input_image_path)call so the script exits with a clear message.🤖 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-Simpleware_Segmentation/simpleware_heart_segmentation.py` around lines 61 - 67, After the file-selection flow around filedialog.askopenfilename, validate that input_image_path is non-empty before the later itk.imread call; print a clear cancellation message and exit early when no file is selected, while preserving normal processing for valid paths.tutorials/tutorial_02_lung_distancemap_finetune_icon.py-1-2 (1)
1-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new distance-map tutorial.
docs/tutorials.rstlists onlytutorial_02_lung_finetune_icon.pyfor Tutorial 2 and still states that the repository has 15 runnable scripts. Add this tutorial to the index, or explicitly identify it as an advanced variant. Otherwise users cannot discover it.🤖 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 `@tutorials/tutorial_02_lung_distancemap_finetune_icon.py` around lines 1 - 2, Update the Tutorial 2 documentation in docs/tutorials.rst to include tutorials/tutorial_02_lung_distancemap_finetune_icon.py, clearly labeling it as the distance-map or advanced variant if appropriate, and revise the runnable-script count to match the added tutorial.tests/test_contour_tools.py-360-386 (1)
360-386: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a baseline comparison for the merged
.vtpoutput.The tests write surfaces but only assert array membership and cell counts. Add deterministic baselines under
tests/baselines/forcombined.vtpandcombined_group.vtp, then compare withTestToolsfor the full merged output. If exact comparison is not available for.vtp, compare the relevant arrays across multiple deterministic inputs to keep the regression stronger.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_contour_tools.py` around lines 360 - 386, Add deterministic baseline fixtures under tests/baselines for combined.vtp and combined_group.vtp, and update the corresponding tests around save_combined_surfaces to compare the full merged output using TestTools. If TestTools lacks exact .vtp comparison support, compare the relevant output arrays across multiple deterministic surface inputs instead of only checking label membership and cell counts.Source: Coding guidelines
tutorials/README.md-31-31 (1)
31-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the added Markdown entry within the line limit.
Line 31 exceeds 88 characters. Shorten the table content or move the longer description below the table.
As per coding guidelines, “Use double quotes for strings and docstrings, and keep lines at or below 88 characters.”
🤖 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 `@tutorials/README.md` at line 31, Shorten the Markdown table entry for tutorial_02_lung_distancemap_finetune_icon.py so the entire line is no longer than 88 characters, while preserving the tutorial link and essential description.Source: Coding guidelines
src/physiotwin4d/convert_vtk_to_usd.py-957-960 (1)
957-960: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the workflow logging helper.
Replace
self.logger.warning(...)withself.log_warning(...). This keeps logging behavior inPhysioTwin4DBase.As per coding guidelines, “All classes must inherit from
PhysioTwin4DBase; useself.log_info()orself.log_debug()for logging and never useprint().”🤖 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/physiotwin4d/convert_vtk_to_usd.py` around lines 957 - 960, Update the warning call in the segmentation-label handling flow to use the inherited PhysioTwin4DBase helper self.log_warning(...) instead of self.logger.warning(...), preserving the existing message and behavior.Source: Coding guidelines
tests/test_workflow_convert_vtk_to_usd.py-172-173 (1)
172-173: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the direct test-file runner.
Run this test through the project test command. The direct
pytest.main(...)path bypasses the documented suite invocation.As per coding guidelines, “Use
py -m pytest tests/ -vfor fast tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workflow_convert_vtk_to_usd.py` around lines 172 - 173, Remove the __main__ block that directly invokes pytest.main in the test file, so the test is run only through the documented project command, py -m pytest tests/ -v.Source: Coding guidelines
tests/test_workflow_convert_vtk_to_usd.py-16-25 (1)
16-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
Optional[str]for the nullable parameter.The repository guideline requires nullable Python annotations to use
Optional[X]instead ofstr | None.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workflow_convert_vtk_to_usd.py` around lines 16 - 25, Update the nullable group parameter in _labeled_sphere to use Optional[str] instead of str | None, and add or reuse the required Optional import while preserving the existing default value and behavior.Source: Coding guidelines
src/physiotwin4d/register_models_pca.py-466-490 (1)
466-490: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProbe the transform inside the model's own extent.
_affine_of_transformprobes the origin, the three unit basis vectors, and one point near the unit cube. ADisplacementFieldTransformreturns the identity outside its field support. If the model sits far from the origin, all five probes fall outside that support, the linearity check passes, and the transform is accepted as the identity affine._prepare_samplingthen folds an identity matrix into the modes and_apply_post_pca_transformreturns the points unchanged, so the post-PCA deformation is silently dropped instead of triggering the documented finite-difference fallback.Probe points drawn from
pca_template_model.boundsinstead of the unit cube keeps the check inside the region the transform is actually applied to.🤖 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/physiotwin4d/register_models_pca.py` around lines 466 - 490, Update _affine_of_transform to derive its probe points from pca_template_model.bounds rather than the origin, unit basis vectors, and unit-cube probe. Ensure the offset, matrix, and nonlinearity check use points within the model’s extent, while preserving affine-transform detection and returning None for non-affine transforms so _prepare_sampling uses the documented finite-difference fallback.tests/test_register_models_pca.py-295-329 (1)
295-329: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that the forward field actually displaces the points.
registered_model_pca_deformationisstd[0] * mode, andmodeis normalized over all points, so each per-point displacement is far below 1 mm. An identity forward transform therefore also satisfiesfield_rms < 1.5andround_trip_rms < 1.0. The test passes whether or notcompute_pca_transformsproduced a usable field.Add a lower-bound check on the displacement, or scale the deformation so the tolerances discriminate.
💚 Proposed strengthening
assert field_rms < 1.5 assert round_trip_rms < 1.0 + # Guard against a no-op field: the forward transform must move the points. + assert np.linalg.norm(mapped - template_points, axis=1).max() > 0.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_register_models_pca.py` around lines 295 - 329, Strengthen test_pca_transforms_round_trip so it verifies that the forward transform produces a meaningful displacement, not just accurate round-tripping. Add a lower-bound assertion for the measured displacement, or scale the registered_model_pca_deformation before computing expected, while retaining the existing upper-bound and round-trip checks.src/physiotwin4d/register_models_pca.py-110-144 (1)
110-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
symmetric_weightagainst its documented range.The docstring states
symmetric_weightis a "Weight in [0, 1]", but the constructor does not check it._objective_and_gradientcomputes(1.0 - weight) * forward_distance + weight * reverse_distance. A value above 1.0 makes the model-to-target term negative, so the optimizer is rewarded for moving the model away from the target. The constructor already validates the eigenvector shapes and mode counts, so add the same guard here.🛡️ Proposed validation
if self.pca_eigenvectors.shape[0] != self.pca_std_deviations.shape[0]: raise ValueError( f"Mode count mismatch: {self.pca_eigenvectors.shape[0]} eigenvectors " f"but {self.pca_std_deviations.shape[0]} standard deviations" ) + if not 0.0 <= symmetric_weight <= 1.0: + raise ValueError( + f"symmetric_weight must be in [0, 1], got {symmetric_weight}" + )🤖 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/physiotwin4d/register_models_pca.py` around lines 110 - 144, Validate symmetric_weight in the constructor before storing or using it, requiring it to be within the documented inclusive range [0, 1]. Raise the constructor’s established validation error type for out-of-range values, consistent with the existing eigenvector and mode-count checks, while preserving valid behavior in _objective_and_gradient.
🧹 Nitpick comments (8)
tests/test_contour_tools.py (1)
344-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the concrete mesh return type.
_annotated_sphere()returns apv.PolyData. ReplaceAnywithpv.PolyDataso strict mypy preserves the helper contract. Verify this against the installed PyVista stubs.As per coding guidelines, “Use full type hints compatible with strict mypy.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_contour_tools.py` around lines 344 - 349, Update the _annotated_sphere helper’s return annotation from Any to the concrete pv.PolyData type, matching the object returned by pv.Sphere and the installed PyVista stubs while preserving its existing behavior.Source: Coding guidelines
tests/test_workflow_convert_vtk_to_usd.py (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the pytest test class or document an exemption.
TestAnatomyAppearancedoes not inherit fromPhysioTwin4DBase. Do not add that base class directly, because its initializer can stop pytest from collecting the class. Convert these methods to module-level tests, or add an explicit test-class exemption to the guideline.As per coding guidelines, “All classes must inherit from
PhysioTwin4DBase.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workflow_convert_vtk_to_usd.py` around lines 36 - 37, Remove or refactor the TestAnatomyAppearance class so the tests become module-level functions, avoiding a non-PhysioTwin4DBase test class; alternatively, add the project’s explicit exemption for this class without making it inherit from PhysioTwin4DBase.Source: Coding guidelines
src/physiotwin4d/workflow_fit_statistical_model_to_patient.py (2)
302-315: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the checkpoint path when it is set.
set_labelmap_to_labelmap_icon_weights_pathstores the path without checking it. The path is first used inregister_labelmap_to_labelmap, which is stage 3.RegisterModelsDistanceMaps.set_icon_weights_pathraisesFileNotFoundErrorthere. A typo in the path therefore fails only after ICP and PCA registration have completed. Check the path here so the error surfaces before any work starts.♻️ Proposed fail-fast check
Args: weights_path: Path to an existing uniGradICON checkpoint. + + Raises: + FileNotFoundError: If weights_path does not exist. """ + if not Path(weights_path).exists(): + raise FileNotFoundError(f"ICON weights not found: {weights_path}") self.l2l_icon_weights_path = weights_path🤖 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/physiotwin4d/workflow_fit_statistical_model_to_patient.py` around lines 302 - 315, Update set_labelmap_to_labelmap_icon_weights_path to validate that weights_path exists before assigning it to l2l_icon_weights_path, raising FileNotFoundError consistently with RegisterModelsDistanceMaps.set_icon_weights_path. Preserve the existing assignment for valid checkpoint paths so invalid paths fail when configured, before registration work begins.
695-710: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDerive the padding margin from spacing rather than hard-coding 50 voxels.
pad_voxels=[50, 50, 50]fixes the margin in voxels, so the physical margin scales with the image resolution: 50 mm at 1 mm spacing, 25 mm at 0.5 mm spacing. The margin needs to cover the template surface that falls outside the patient image, which is a physical distance, not a voxel count.The cost is also material. Padding a 256-voxel axis by 50 per side grows it to 356, so the distance map and the ICON deformable stage run on roughly 2.7 times the voxels.
ImageTools.pad_imageacceptspad_portionfor exactly this case, andmask_dilation_mmalready expresses the relevant physical scale.Consider deriving the margin from
self.mask_dilation_mmand the image spacing, or exposing it through a setter so callers can tune it.🤖 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/physiotwin4d/workflow_fit_statistical_model_to_patient.py` around lines 695 - 710, Update the padding setup in the workflow around ImageTools().pad_image to derive the margin from the patient image spacing and the physical scale represented by self.mask_dilation_mm, rather than using the fixed pad_voxels=[50, 50, 50]. Prefer the existing pad_portion interface when appropriate, or expose a tunable padding configuration while ensuring the resulting padding covers the intended physical margin on each axis.src/physiotwin4d/register_models_pca.py (1)
999-1034: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSubsample or gate the fidelity check.
_log_transform_fidelityruns a Python loop over every template point and callsTransformPointtwice per point.compute_pca_transformscalls it unconditionally. For a large template this adds hundreds of thousands of interpreter-level ITK calls purely to produce two log lines. Subsample the points, or run the check only when the log level is DEBUG.♻️ Proposed change
- self._log_transform_fidelity(template_points) + # Cap the fidelity check: the RMS of a few thousand points is + # representative and keeps this off the hot path for large templates. + step = max(1, len(template_points) // 2000) + self._log_transform_fidelity(template_points[::step])🤖 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/physiotwin4d/register_models_pca.py` around lines 999 - 1034, Reduce the cost of _log_transform_fidelity, which currently performs two ITK TransformPoint calls for every template point and is invoked unconditionally by compute_pca_transforms. Either gate the fidelity calculation behind DEBUG-level logging or subsample template_points before the loop, while preserving the existing RMS log outputs for the points that are checked.src/physiotwin4d/image_tools.py (1)
306-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
ImageTools.pad_image.
pad_imageexposes three invalid-inputValueErrorcases and an origin-preservation guarantee, buttests/test_image_tools.pyhas no coverage forpad_image,_per_axis_values, or the origin re-anchoring behavior.🤖 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/physiotwin4d/image_tools.py` around lines 306 - 413, Add focused tests in the image-tools test suite for pad_image and _per_axis_values: cover mutually exclusive or missing padding arguments, negative values, and sequences with incorrect dimensionality raising ValueError. Also verify padding preserves the input voxels’ physical positions by checking the returned image origin and expected padded dimensions for representative voxel and portion inputs.Source: Coding guidelines
src/physiotwin4d/register_images_greedy.py (1)
366-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
kwargs_affexplicitly for strict mypy.mypy infers the value type of
kwargs_afffrom the initial literal, which holds SimpleITK images. Line 375 then assignsNonetokwargs_aff["aff_init"]. Strict mypy rejects that assignment._registration_method_affine_or_rigidalready declareskwargs: dict[str, Any]at line 320; use the same annotation here.♻️ Proposed annotation
- kwargs_aff = { + kwargs_aff: dict[str, Any] = { "fixed": fixed_sitk, "moving": moving_sitk, }🤖 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/physiotwin4d/register_images_greedy.py` around lines 366 - 375, Annotate the kwargs_aff dictionary declaration in the affine registration setup with dict[str, Any], matching the existing _registration_method_affine_or_rigid kwargs annotation, so the later aff_init assignment of None passes strict mypy.Source: Coding guidelines
src/physiotwin4d/register_time_series_images.py (1)
306-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the two-pass loop.
No state carries between frames now, so the forward pass and the backward pass produce the same result as a single pass over every index other than
reference_frame. The two-pass structure only adds complexity. Keep it if you plan to restore prior propagation; otherwise a single loop is clearer.♻️ Proposed simplification
- # Register forward and backward from reference frame - for step, start_idx, end_idx in [ - (1, reference_frame + 1, num_images), # Forward pass - (-1, reference_frame - 1, -1), # Backward pass - ]: - for img_idx in range(start_idx, end_idx, step): - moving_image = moving_images[img_idx] - moving_mask = ( - moving_masks[img_idx] if moving_masks is not None else None - ) - moving_labelmap = ( - moving_labelmaps[img_idx] if moving_labelmaps is not None else None - ) - - result = self.registrar.register( - moving_image=moving_image, - moving_mask=moving_mask, - moving_labelmap=moving_labelmap, - ) - - forward_transforms[img_idx] = result["forward_transform"] - inverse_transforms[img_idx] = result["inverse_transform"] - losses[img_idx] = cast(float, result["loss"]) + # Every frame is registered independently, so the order does not matter. + for img_idx in range(num_images): + if img_idx == reference_frame: + continue + result = self.registrar.register( + moving_image=moving_images[img_idx], + moving_mask=( + moving_masks[img_idx] if moving_masks is not None else None + ), + moving_labelmap=( + moving_labelmaps[img_idx] if moving_labelmaps is not None else None + ), + ) + + forward_transforms[img_idx] = result["forward_transform"] + inverse_transforms[img_idx] = result["inverse_transform"] + losses[img_idx] = cast(float, result["loss"])🤖 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/physiotwin4d/register_time_series_images.py` around lines 306 - 328, Replace the separate forward and backward iteration in the registration flow with one loop over every image index except reference_frame, while preserving the existing moving-image, mask, labelmap, registration, and result-assignment logic in the loop.
🤖 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 `@src/physiotwin4d/contour_tools.py`:
- Around line 355-365: Update the face-sampling flow around the samples
collection and create_distance_map() so generated triangle samples are processed
and rasterized in bounded batches rather than appended to samples for the entire
mesh. Ensure each batch is released after rasterization, and preserve the
existing barycentric sampling behavior and final distance-map results without
retaining all face coordinates simultaneously.
In `@src/physiotwin4d/convert_vtk_to_usd.py`:
- Around line 168-177: Update the initializer validation for object_names to
enforce valid USD prim-name path components and reject duplicate names before
assigning self.object_names. Preserve the existing length validation, and make
the resulting ValueError clearly mention that names must be unique and comply
with USD prim-name rules.
In `@src/physiotwin4d/register_images_base.py`:
- Around line 396-411: Update register_from around the self.register(...) call
to restore self.moving_image to the original moving_image after seeded
registration, matching the composed transform’s input. Also reset
self.moving_image_registered so get_registered_image() cannot reuse the cache
produced for the pre-warped image.
In `@src/physiotwin4d/register_models_distance_maps.py`:
- Around line 230-237: Remove the unconditional fixed-image debug writes around
register_models_distance_maps.py lines 230-237, or guard them with
self.log_level == logging.DEBUG and self.fixed_mask_image is not None. Apply the
same removal or guard to the moving-image writes at lines 271-278, using
self.moving_mask_image, so disabled mask dilation cannot pass None to
itk.imwrite or write files by default.
In `@src/physiotwin4d/workflow_convert_vtk_to_usd.py`:
- Around line 213-234: Update the object_groups construction in the static-merge
naming flow so anatomy-group annotations remain mapped when object_names is
None. Use the same effective positional names generated from annotations (the
project-name/index fallback) as the map keys, while preserving custom
object_names when available, allowing positional prim names to resolve their
anatomy groups instead of defaulting to “other”.
- Around line 139-150: Update _read_object_annotations() to convert supported
raw vtk.vtkDataSet objects with pv.wrap() before checking field_data
annotations. Preserve the existing non-dataset fallback, then read
SegmentationLabelNames and AnatomyGroup from the wrapped dataset so automatic
names and anatomy groups are retained.
In `@src/physiotwin4d/workflow_fit_statistical_model_to_patient.py`:
- Around line 611-625: Update compute_pca_transforms() and its
create_deformation_field() input so the deformation-field grid covers the
un-aligned pca_template_model.points as well as the patient image, or use an
axis-aligned template-frame grid before ICP composition. Preserve PCA
deformation for use_surface and template-labelmap resampling even when the
un-aligned template lies outside the patient-image bounds.
In `@tutorials/tutorial_06_lung_create_statistical_model.py`:
- Line 170: Update mode_count before the mode loop to use the minimum of
number_of_pca_components, len(components), and len(eigenvalues), ensuring the
loop only indexes available PCA components and eigenvalues.
---
Outside diff comments:
In `@src/physiotwin4d/register_images_ants.py`:
- Around line 537-555: Remove the stale initial-transform claims from the
docstring for registration_method, including the Note text about
pre-warping/composition and the implementation-detail bullet about ITK-to-ANTs
conversion. Replace them with a brief reference directing readers to
RegisterImagesBase.register_from for initial-transform handling, while
preserving the remaining registration behavior documentation.
In `@src/physiotwin4d/register_models_distance_maps.py`:
- Around line 126-139: Update the constructor docstring’s Args section for
__init__ to document distance_squared_max, describing it as the maximum squared
distance threshold and specifying that its unit is squared millimeters.
In `@src/physiotwin4d/register_time_series_images.py`:
- Around line 26-45: Update the class docstring and the affected method
docstring in the time-series registration implementation to remove claims about
propagating or reusing prior transforms, initialization benefits, and
temporal-coherence guarantees. Keep the documentation aligned with the current
independent-per-frame behavior and the existing note that states prior-transform
initialization is not used.
---
Minor comments:
In @.agents/agents/testing.md:
- Line 32: Wrap the modified Markdown lines to 88 characters or fewer: in
.agents/agents/testing.md lines 32-32, move the inline CI explanation to a
separate wrapped comment line; in CLAUDE.md lines 134-134, place the
fixture-chain description on a continuation line; and in statistics.md lines
155-155, place the tutorial marker description on a continuation line.
In `@experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py`:
- Around line 61-67: After the file-selection flow around
filedialog.askopenfilename, validate that input_image_path is non-empty before
the later itk.imread call; print a clear cancellation message and exit early
when no file is selected, while preserving normal processing for valid paths.
In `@src/physiotwin4d/convert_vtk_to_usd.py`:
- Around line 957-960: Update the warning call in the segmentation-label
handling flow to use the inherited PhysioTwin4DBase helper self.log_warning(...)
instead of self.logger.warning(...), preserving the existing message and
behavior.
In `@src/physiotwin4d/register_models_pca.py`:
- Around line 466-490: Update _affine_of_transform to derive its probe points
from pca_template_model.bounds rather than the origin, unit basis vectors, and
unit-cube probe. Ensure the offset, matrix, and nonlinearity check use points
within the model’s extent, while preserving affine-transform detection and
returning None for non-affine transforms so _prepare_sampling uses the
documented finite-difference fallback.
- Around line 110-144: Validate symmetric_weight in the constructor before
storing or using it, requiring it to be within the documented inclusive range
[0, 1]. Raise the constructor’s established validation error type for
out-of-range values, consistent with the existing eigenvector and mode-count
checks, while preserving valid behavior in _objective_and_gradient.
In `@tests/conftest.py`:
- Around line 615-620: Update the docstring near the moving/fixed resampling
explanation to describe the expected displacement and sign-check behavior
without mentioning RAS/LPS or any fixed coordinate-frame convention. Preserve
the explanation that the transform uses the negated shift and validates absolute
accuracy.
In `@tests/README.md`:
- Line 143: Wrap the timing-report sentence in tests/README.md so each Markdown
line is no longer than 88 characters, preserving the existing wording and
meaning.
In `@tests/test_contour_tools.py`:
- Around line 360-386: Add deterministic baseline fixtures under tests/baselines
for combined.vtp and combined_group.vtp, and update the corresponding tests
around save_combined_surfaces to compare the full merged output using TestTools.
If TestTools lacks exact .vtp comparison support, compare the relevant output
arrays across multiple deterministic surface inputs instead of only checking
label membership and cell counts.
In `@tests/test_register_models_pca.py`:
- Around line 295-329: Strengthen test_pca_transforms_round_trip so it verifies
that the forward transform produces a meaningful displacement, not just accurate
round-tripping. Add a lower-bound assertion for the measured displacement, or
scale the registered_model_pca_deformation before computing expected, while
retaining the existing upper-bound and round-trip checks.
In `@tests/test_workflow_convert_vtk_to_usd.py`:
- Around line 172-173: Remove the __main__ block that directly invokes
pytest.main in the test file, so the test is run only through the documented
project command, py -m pytest tests/ -v.
- Around line 16-25: Update the nullable group parameter in _labeled_sphere to
use Optional[str] instead of str | None, and add or reuse the required Optional
import while preserving the existing default value and behavior.
In `@tutorials/README.md`:
- Line 31: Shorten the Markdown table entry for
tutorial_02_lung_distancemap_finetune_icon.py so the entire line is no longer
than 88 characters, while preserving the tutorial link and essential
description.
In `@tutorials/tutorial_02_lung_distancemap_finetune_icon.py`:
- Around line 1-2: Update the Tutorial 2 documentation in docs/tutorials.rst to
include tutorials/tutorial_02_lung_distancemap_finetune_icon.py, clearly
labeling it as the distance-map or advanced variant if appropriate, and revise
the runnable-script count to match the added tutorial.
---
Nitpick comments:
In `@src/physiotwin4d/image_tools.py`:
- Around line 306-413: Add focused tests in the image-tools test suite for
pad_image and _per_axis_values: cover mutually exclusive or missing padding
arguments, negative values, and sequences with incorrect dimensionality raising
ValueError. Also verify padding preserves the input voxels’ physical positions
by checking the returned image origin and expected padded dimensions for
representative voxel and portion inputs.
In `@src/physiotwin4d/register_images_greedy.py`:
- Around line 366-375: Annotate the kwargs_aff dictionary declaration in the
affine registration setup with dict[str, Any], matching the existing
_registration_method_affine_or_rigid kwargs annotation, so the later aff_init
assignment of None passes strict mypy.
In `@src/physiotwin4d/register_models_pca.py`:
- Around line 999-1034: Reduce the cost of _log_transform_fidelity, which
currently performs two ITK TransformPoint calls for every template point and is
invoked unconditionally by compute_pca_transforms. Either gate the fidelity
calculation behind DEBUG-level logging or subsample template_points before the
loop, while preserving the existing RMS log outputs for the points that are
checked.
In `@src/physiotwin4d/register_time_series_images.py`:
- Around line 306-328: Replace the separate forward and backward iteration in
the registration flow with one loop over every image index except
reference_frame, while preserving the existing moving-image, mask, labelmap,
registration, and result-assignment logic in the loop.
In `@src/physiotwin4d/workflow_fit_statistical_model_to_patient.py`:
- Around line 302-315: Update set_labelmap_to_labelmap_icon_weights_path to
validate that weights_path exists before assigning it to l2l_icon_weights_path,
raising FileNotFoundError consistently with
RegisterModelsDistanceMaps.set_icon_weights_path. Preserve the existing
assignment for valid checkpoint paths so invalid paths fail when configured,
before registration work begins.
- Around line 695-710: Update the padding setup in the workflow around
ImageTools().pad_image to derive the margin from the patient image spacing and
the physical scale represented by self.mask_dilation_mm, rather than using the
fixed pad_voxels=[50, 50, 50]. Prefer the existing pad_portion interface when
appropriate, or expose a tunable padding configuration while ensuring the
resulting padding covers the intended physical margin on each axis.
In `@tests/test_contour_tools.py`:
- Around line 344-349: Update the _annotated_sphere helper’s return annotation
from Any to the concrete pv.PolyData type, matching the object returned by
pv.Sphere and the installed PyVista stubs while preserving its existing
behavior.
In `@tests/test_workflow_convert_vtk_to_usd.py`:
- Around line 36-37: Remove or refactor the TestAnatomyAppearance class so the
tests become module-level functions, avoiding a non-PhysioTwin4DBase test class;
alternatively, add the project’s explicit exemption for this class without
making it inherit from PhysioTwin4DBase.
🪄 Autofix
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 Plus
Run ID: 11412ee5-e4d4-4f99-84d2-a32ae9bacc59
📒 Files selected for processing (89)
.agents/agents/testing.md.github/workflows/README.md.github/workflows/ci.yml.github/workflows/nightly-health.ymlAGENTS.mdCLAUDE.mddocs/cli_scripts/create_statistical_model.rstdocs/cli_scripts/fit_statistical_model_to_patient.rstdocs/cli_scripts/vtk_to_usd.rstdocs/contributing.rstdocs/developer/core.rstdocs/developer/registration_images.rstdocs/testing.rstdocs/tutorials.rstexperiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.pyexperiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.pyexperiments/Heart-Create_Statistical_Model/1-input_meshes_to_input_surfaces.pyexperiments/Heart-Create_Statistical_Model/2-input_surfaces_to_surfaces_aligned.pyexperiments/Heart-Create_Statistical_Model/3-registration_based_correspondence.pyexperiments/Heart-Create_Statistical_Model/4-surfaces_aligned_correspond_to_pca_inputs.pyexperiments/Heart-Create_Statistical_Model/5-compute_pca_model.pyexperiments/Heart-Create_Statistical_Model/README.mdexperiments/Heart-GatedCT_To_USD/1-register_images.pyexperiments/Heart-GatedCT_To_USD/2-generate_segmentation.pyexperiments/Heart-GatedCT_To_USD/3-transform_dynamic_and_static_contours.pyexperiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_model_icp_itk.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_model_registration_pca.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient-CHOPValve.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.pyexperiments/README.mdexperiments/Reconstruct4DCT/reconstruct_4d_ct.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct_class.pypyproject.tomlsrc/physiotwin4d/cli/convert_vtk_to_usd.pysrc/physiotwin4d/cli/create_statistical_model.pysrc/physiotwin4d/cli/fit_statistical_model_to_patient.pysrc/physiotwin4d/cli/reconstruct_highres_4d_ct.pysrc/physiotwin4d/contour_tools.pysrc/physiotwin4d/convert_vtk_to_usd.pysrc/physiotwin4d/image_tools.pysrc/physiotwin4d/physicsnemo_tools.pysrc/physiotwin4d/register_images_ants.pysrc/physiotwin4d/register_images_base.pysrc/physiotwin4d/register_images_chain.pysrc/physiotwin4d/register_images_greedy.pysrc/physiotwin4d/register_images_icon.pysrc/physiotwin4d/register_models_distance_maps.pysrc/physiotwin4d/register_models_icp.pysrc/physiotwin4d/register_models_pca.pysrc/physiotwin4d/register_time_series_images.pysrc/physiotwin4d/train_physicsnemo_mgn.pysrc/physiotwin4d/transform_tools.pysrc/physiotwin4d/usd_anatomy_tools.pysrc/physiotwin4d/workflow_convert_vtk_to_usd.pysrc/physiotwin4d/workflow_create_statistical_model.pysrc/physiotwin4d/workflow_fit_statistical_model_to_patient.pysrc/physiotwin4d/workflow_reconstruct_highres_4d_ct.pystatistics.mdtests/README.mdtests/baselines/registration_time_series_images/basic_forward_transform_0.hdftests/baselines/registration_time_series_images/basic_time_series_registered_0.mhatests/baselines/registration_time_series_images/middle_frame_forward_transform_0.hdftests/baselines/registration_time_series_images/prior_forward_transform_0.hdftests/baselines/registration_time_series_images/prior_time_series_registered_0.mhatests/baselines/registration_time_series_images/transform_application_time_series_0.mhatests/conftest.pytests/test_contour_tools.pytests/test_convert_vtk_to_usd.pytests/test_experiments.pytests/test_register_images_ants.pytests/test_register_images_chain.pytests/test_register_images_greedy.pytests/test_register_images_icon.pytests/test_register_models_pca.pytests/test_register_time_series_images.pytests/test_tutorials.pytests/test_workflow_convert_vtk_to_usd.pytutorials/README.mdtutorials/tutorial_02_lung_distancemap_finetune_icon.pytutorials/tutorial_02_lung_finetune_icon.pytutorials/tutorial_04_heart_ct_to_vtk.pytutorials/tutorial_04_lung_ct_to_vtk.pytutorials/tutorial_05_heart_vtk_to_usd.pytutorials/tutorial_06_heart_create_statistical_model.pytutorials/tutorial_06_lung_create_statistical_model.pytutorials/tutorial_07_lung_fit_statistical_model_to_patient.pytutorials/tutorial_08_lung_fit_model_to_4d_patients.pytutorials/tutorial_09_lung_train_physicsnemo_mgn.py
💤 Files with no reviewable changes (6)
- tests/baselines/registration_time_series_images/prior_time_series_registered_0.mha
- tests/baselines/registration_time_series_images/prior_forward_transform_0.hdf
- tests/test_experiments.py
- src/physiotwin4d/register_images_icon.py
- src/physiotwin4d/workflow_reconstruct_highres_4d_ct.py
- src/physiotwin4d/cli/reconstruct_highres_4d_ct.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/physiotwin4d/register_models_pca.py`:
- Around line 219-224: Validate pca_prior_weight in the initializer before
assigning it, ensuring the value is finite and non-negative; raise ValueError
for invalid inputs. Use the existing pca_prior_weight parameter and validation
pattern near symmetric_weight, while preserving valid values unchanged.
In `@src/physiotwin4d/workflow_fit_statistical_model_to_patient.py`:
- Around line 608-622: Update the three itk.imwrite calls in the log_level ==
logging.DEBUG block to pass compression=True while preserving their existing
images and output filenames.
- Around line 319-321: Update the weights_path validation in the setter
containing self.l2l_icon_weights_path to use Path(weights_path).is_file()
instead of exists(), so directories are rejected and only valid checkpoint files
are accepted before assignment.
🪄 Autofix
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 Plus
Run ID: 90532f65-0703-4f4a-9baf-4284796afe58
📒 Files selected for processing (23)
.agents/agents/testing.mdCLAUDE.mddocs/tutorials.rstexperiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct.pysrc/physiotwin4d/convert_vtk_to_usd.pysrc/physiotwin4d/register_images_ants.pysrc/physiotwin4d/register_images_base.pysrc/physiotwin4d/register_images_greedy.pysrc/physiotwin4d/register_models_distance_maps.pysrc/physiotwin4d/register_models_pca.pysrc/physiotwin4d/register_time_series_images.pysrc/physiotwin4d/transform_tools.pysrc/physiotwin4d/workflow_convert_vtk_to_usd.pysrc/physiotwin4d/workflow_fit_statistical_model_to_patient.pystatistics.mdtests/README.mdtests/conftest.pytests/test_contour_tools.pytests/test_register_models_pca.pytests/test_workflow_convert_vtk_to_usd.pytutorials/README.mdtutorials/tutorial_06_lung_create_statistical_model.py
🚧 Files skipped from review as they are similar to previous changes (15)
- tutorials/tutorial_06_lung_create_statistical_model.py
- statistics.md
- docs/tutorials.rst
- tests/test_contour_tools.py
- .agents/agents/testing.md
- src/physiotwin4d/convert_vtk_to_usd.py
- src/physiotwin4d/transform_tools.py
- tests/README.md
- CLAUDE.md
- experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py
- tutorials/README.md
- src/physiotwin4d/register_images_base.py
- src/physiotwin4d/workflow_convert_vtk_to_usd.py
- src/physiotwin4d/register_time_series_images.py
- tests/test_register_models_pca.py
Comment on lines
+219
to
+224
| self.pca_prior_weight = pca_prior_weight | ||
| if not 0.0 <= symmetric_weight <= 1.0: | ||
| raise ValueError( | ||
| f"symmetric_weight must be in [0, 1]; got {symmetric_weight}" | ||
| ) | ||
| self.symmetric_weight = symmetric_weight |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid PCA prior weights.
A negative pca_prior_weight rewards large coefficient magnitudes instead of
penalizing them. A non-finite value makes the optimization objective non-finite.
Validate that the value is finite and non-negative.
Proposed fix
+ if not np.isfinite(pca_prior_weight) or pca_prior_weight < 0.0:
+ raise ValueError(
+ "pca_prior_weight must be finite and greater than or equal to 0"
+ )
self.pca_prior_weight = pca_prior_weight📝 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
| self.pca_prior_weight = pca_prior_weight | |
| if not 0.0 <= symmetric_weight <= 1.0: | |
| raise ValueError( | |
| f"symmetric_weight must be in [0, 1]; got {symmetric_weight}" | |
| ) | |
| self.symmetric_weight = symmetric_weight | |
| if not np.isfinite(pca_prior_weight) or pca_prior_weight < 0.0: | |
| raise ValueError( | |
| "pca_prior_weight must be finite and greater than or equal to 0" | |
| ) | |
| self.pca_prior_weight = pca_prior_weight | |
| if not 0.0 <= symmetric_weight <= 1.0: | |
| raise ValueError( | |
| f"symmetric_weight must be in [0, 1]; got {symmetric_weight}" | |
| ) | |
| self.symmetric_weight = symmetric_weight |
🤖 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/physiotwin4d/register_models_pca.py` around lines 219 - 224, Validate
pca_prior_weight in the initializer before assigning it, ensuring the value is
finite and non-negative; raise ValueError for invalid inputs. Use the existing
pca_prior_weight parameter and validation pattern near symmetric_weight, while
preserving valid values unchanged.
Comment on lines
+319
to
+321
| if not Path(weights_path).exists(): | ||
| raise FileNotFoundError(f"ICON weights not found: {weights_path}") | ||
| self.l2l_icon_weights_path = weights_path |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a checkpoint file.
Path.exists() accepts directories. A directory then passes this setter and fails
later when ICON loads the checkpoint. Use Path(weights_path).is_file().
Proposed fix
- if not Path(weights_path).exists():
+ if not Path(weights_path).is_file():
raise FileNotFoundError(f"ICON weights not found: {weights_path}")📝 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
| if not Path(weights_path).exists(): | |
| raise FileNotFoundError(f"ICON weights not found: {weights_path}") | |
| self.l2l_icon_weights_path = weights_path | |
| if not Path(weights_path).is_file(): | |
| raise FileNotFoundError(f"ICON weights not found: {weights_path}") | |
| self.l2l_icon_weights_path = weights_path |
🤖 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/physiotwin4d/workflow_fit_statistical_model_to_patient.py` around lines
319 - 321, Update the weights_path validation in the setter containing
self.l2l_icon_weights_path to use Path(weights_path).is_file() instead of
exists(), so directories are rejected and only valid checkpoint files are
accepted before assignment.
Comment on lines
608
to
622
| if self.log_level == logging.DEBUG: | ||
| tfm_arr = itk.GetArrayFromImage( | ||
| self.pca_forward_point_transform.GetDisplacementField() | ||
| ) | ||
| tfm_field = self.pca_forward_point_transform.GetDisplacementField() | ||
| tfm_arr = itk.GetArrayFromImage(tfm_field) | ||
| tfm_x_arr = tfm_arr[:, :, :, 0] | ||
| tfm_y_arr = tfm_arr[:, :, :, 1] | ||
| tfm_z_arr = tfm_arr[:, :, :, 2] | ||
| tfm_x_img = itk.GetImageFromArray(tfm_x_arr) | ||
| tfm_y_img = itk.GetImageFromArray(tfm_y_arr) | ||
| tfm_z_img = itk.GetImageFromArray(tfm_z_arr) | ||
| tfm_x_img.CopyInformation(self.patient_image) | ||
| tfm_y_img.CopyInformation(self.patient_image) | ||
| tfm_z_img.CopyInformation(self.patient_image) | ||
| tfm_x_img.CopyInformation(tfm_field) | ||
| tfm_y_img.CopyInformation(tfm_field) | ||
| tfm_z_img.CopyInformation(tfm_field) | ||
| 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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Enable compression for debug ITK writes.
Pass compression=True to each itk.imwrite call in this block.
As per coding guidelines, “Persist ITK images with
itk.imwrite(..., compression=True).”
Proposed fix
- 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")
+ itk.imwrite(
+ tfm_x_img, "pca_forward_point_transform_x.nii.gz", compression=True
+ )
+ itk.imwrite(
+ tfm_y_img, "pca_forward_point_transform_y.nii.gz", compression=True
+ )
+ itk.imwrite(
+ tfm_z_img, "pca_forward_point_transform_z.nii.gz", compression=True
+ )📝 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
| if self.log_level == logging.DEBUG: | |
| tfm_arr = itk.GetArrayFromImage( | |
| self.pca_forward_point_transform.GetDisplacementField() | |
| ) | |
| tfm_field = self.pca_forward_point_transform.GetDisplacementField() | |
| tfm_arr = itk.GetArrayFromImage(tfm_field) | |
| tfm_x_arr = tfm_arr[:, :, :, 0] | |
| tfm_y_arr = tfm_arr[:, :, :, 1] | |
| tfm_z_arr = tfm_arr[:, :, :, 2] | |
| tfm_x_img = itk.GetImageFromArray(tfm_x_arr) | |
| tfm_y_img = itk.GetImageFromArray(tfm_y_arr) | |
| tfm_z_img = itk.GetImageFromArray(tfm_z_arr) | |
| tfm_x_img.CopyInformation(self.patient_image) | |
| tfm_y_img.CopyInformation(self.patient_image) | |
| tfm_z_img.CopyInformation(self.patient_image) | |
| tfm_x_img.CopyInformation(tfm_field) | |
| tfm_y_img.CopyInformation(tfm_field) | |
| tfm_z_img.CopyInformation(tfm_field) | |
| 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") | |
| if self.log_level == logging.DEBUG: | |
| tfm_field = self.pca_forward_point_transform.GetDisplacementField() | |
| tfm_arr = itk.GetArrayFromImage(tfm_field) | |
| tfm_x_arr = tfm_arr[:, :, :, 0] | |
| tfm_y_arr = tfm_arr[:, :, :, 1] | |
| tfm_z_arr = tfm_arr[:, :, :, 2] | |
| tfm_x_img = itk.GetImageFromArray(tfm_x_arr) | |
| tfm_y_img = itk.GetImageFromArray(tfm_y_arr) | |
| tfm_z_img = itk.GetImageFromArray(tfm_z_arr) | |
| tfm_x_img.CopyInformation(tfm_field) | |
| tfm_y_img.CopyInformation(tfm_field) | |
| tfm_z_img.CopyInformation(tfm_field) | |
| itk.imwrite( | |
| tfm_x_img, "pca_forward_point_transform_x.nii.gz", compression=True | |
| ) | |
| itk.imwrite( | |
| tfm_y_img, "pca_forward_point_transform_y.nii.gz", compression=True | |
| ) | |
| itk.imwrite( | |
| tfm_z_img, "pca_forward_point_transform_z.nii.gz", 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/physiotwin4d/workflow_fit_statistical_model_to_patient.py` around lines
608 - 622, Update the three itk.imwrite calls in the log_level == logging.DEBUG
block to pass compression=True while preserving their existing images and output
filenames.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/physiotwin4d/convert_vtk_to_usd.py:974
- When a mesh has a per-cell label array but none of its IDs match mask_ids (e.g., a merged mesh tagged with SegmentationLabelIds==0 for every cell), _split_by_labels() later returns an empty dict and _convert_with_labels() ends up emitting no geometry. It would be safer to treat an all-zero (or otherwise non-matching) label array as “unlabeled” and fall back to the unified mesh path.
src/physiotwin4d/register_images_base.py:413 - register_from() restores self.moving_image after registering the pre-warped data, but it leaves self.moving_image_pre / self.moving_mask / self.moving_labelmap pointing at the pre-warped inputs from the inner register() call. That makes the instance state inconsistent (moving_image is original, other moving_* fields are warped) and could confuse downstream logic/debugging or any subclass that inspects self.moving_mask/labelmap after register_from(). Consider restoring/clearing the other moving_* fields as well.
Signed-off-by: Stephen R. Aylward <stephen@aylward.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/physiotwin4d/workflow_convert_vtk_to_usd.py:172
- When
separate_by_cell_type=True, ConvertVTKToUSD names prims like{object}_Triangle(see vtk_to_usd/mesh_utils.py:37-38)._anatomy_candidates()only strips the_objectNsuffix, so it will try to resolve materials for..._Triangleand fail to fall back to the correct AnatomyGroup (becauseobject_groupsis keyed by the unsuffixed object name). This causes anatomy appearance to incorrectly fall back to the "other" material for cell-type-split meshes.
src/physiotwin4d/register_images_base.py:490 - The comment says "the total is the initial transform followed by that residual", but the next lines correctly explain that (because CompositeTransform applies the last-added transform first) the residual is applied first and the initial is applied second. Rewording this avoids confusion when reasoning about transform order.
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.
BUG: Convert Greedy transforms from RAS to LPS, unify registration seeding
Greedy reports its affine in RAS while ITK is LPS, but the 4x4 was copied
straight into an itk.AffineTransform, negating x and y. Recovering a known
(6, -4, 3) mm shift returned (+5.96, -4.01, -2.92) instead of (-6, +4, -3),
and warping by the result scored below the unregistered pair (foreground NCC
0.21 vs 0.32). Rigid, Affine and Deformable were all affected; the
displacement field was already LPS and is left alone. Existing Greedy tests
only asserted the transforms were non-None, so the sign error survived.
helper in conftest); ANTs and ICON were audited and are correct
in ANTs, pre-warped only the image in ICON, and double-applied in Greedy,
with one RegisterImagesBase.register_from()
displacement field that is only defined on the reference grid
workflow and its CLI flag
bounding-box scale estimation before ICP
RegisterModelsDistanceMaps.register()
include Case8Deploy in tutorial 09's case discovery
Summary by CodeRabbit
New Features
Updates
Documentation