ENH: Standardize forward/inverse transform conventions in registration#63
Open
aylward wants to merge 3 commits into
Open
ENH: Standardize forward/inverse transform conventions in registration#63aylward wants to merge 3 commits into
aylward wants to merge 3 commits into
Conversation
Transform conventions
- Add docs/developer/transform_conventions.rst documenting the rule that
forward_transform warps the moving IMAGE onto the fixed grid, while warping
moving points/landmarks into fixed space uses inverse_transform (image and
point warps need opposite transforms), and that model (PCA) registration
returns transforms in the opposite orientation from image registration.
- Clarify docstrings/comments to this convention across
register_images_{base,ants,greedy,icon}, register_models_{distance_maps,
icp,pca}, register_time_series_images, and the workflow_* modules; link the
new page from registration_images.rst, registration_models.rst, index.rst.
- register_images_base: rework initial-transform handling to pre-warp the
moving image onto the fixed grid before registration (matching the ICON
backend) instead of composing an itk transform.
Longitudinal registration experiment
- Add 1-preregistration.py: register every gated frame to its patient
reference with ANTS and Greedy, recording per-label Dice and landmark
squared error, and writing the warped image, labelmap, mask, transforms,
and deformation-grid visualization per frame.
- 1-finetune_icon.py: merge each patient's ANTS- and Greedy-warped frames
into the ICON fine-tuning group and consume the warped loss masks; use
Optional[X] type hints.
- Update 2-recon_4d_icon_eval.py and 3-run_registration_method_comparison.py
for the convention; add tests/test_register_images_ants.py and
registration_test.py; drop the obsolete
experiments/Heart-GatedCT_To_USD/test_compare_registration_speed.py.
Code style
- Correct the documented string-quote standard to double quotes (the actual
codebase standard) in pyproject ruff quote-style/inline-quotes, CLAUDE.md,
AGENTS.md, and docs/contributing.rst.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThis PR introduces ChangesLabelmapTools Core & Documentation
Transform Convention Documentation
Registration Backend Updates (ANTs, Greedy, ICON)
Mask Generation Refactoring Across Components
ICON Fine-Tuning Workflow API Refactoring
Experiment Scripts & Tutorials
Code Style & Configuration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
Standardizes documentation and code around the forward/inverse transform direction conventions across image- and model-registration backends, adds a developer doc page on the rules, reworks ANTs initial-transform handling to pre-warp the moving image (matching the ICON backend), and adds new longitudinal-registration experiment scripts plus ANTs regression tests. Also corrects the documented string-quote standard to double quotes.
Changes:
- New
docs/developer/transform_conventions.rstplus docstring/comment clarifications throughoutregister_images_*,register_models_*,register_time_series_images, and workflow modules; usage sites in tutorials and experiments updated to use the matching transform. register_images_ants.pyinitial transform now pre-warps the moving image instead of passing an ANTs initial_transform;register_images_greedy.pywrites affine init to a temp file to avoid a heap-corruption crash;register_images_icon.pyadds a default weights-path fallback.- Experiments: new
1-preregistration.pyandregistration_test.py, updated2-recon_4d_icon_eval.pyand3-run_registration_method_comparison.py, removed obsoletetest_compare_registration_speed.py; new ANTs regression tests; quote-style updated to double inpyproject.toml,CLAUDE.md,AGENTS.md, anddocs/contributing.rst.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/developer/transform_conventions.rst | New developer doc capturing the forward/inverse rule. |
| docs/developer/registration_images.rst, registration_models.rst, index.rst | Link the new convention page. |
| docs/contributing.rst, CLAUDE.md, AGENTS.md, pyproject.toml | Switch documented quote style to double quotes. |
| src/physiomotion4d/register_images_base.py | Clarify forward/inverse semantics in docstrings. |
| src/physiomotion4d/register_images_ants.py | Apply initial transform via pre-warp; switch to "Affine"/"Rigid" presets. |
| src/physiomotion4d/register_images_greedy.py | Pass affine init via temp .mat file; add docstring/log updates. |
| src/physiomotion4d/register_images_icon.py | Docstring updates; add default weights-path fallback. |
| src/physiomotion4d/register_models_pca.py, register_models_icp.py, register_models_distance_maps.py | Document point-vs-image transform orientation. |
| src/physiomotion4d/register_time_series_images.py | Add set_fixed_labelmap; clarify return docstrings. |
| src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py | Clarify transform docstrings. |
| src/physiomotion4d/workflow_fine_tune_icon_registration.py | Change lncc_sigma default 5→1 and hard-code use_label=False. |
| tutorials/tutorial_08_dirlab_pca_time_series.py | Use phase_to_reference for warping reference mesh into phase space. |
| experiments/LongitudinalRegistration/1-preregistration.py | New cohort-wide ANTs vs Greedy pre-registration script. |
| experiments/LongitudinalRegistration/1-finetune_icon.py | Merge ANTs/Greedy warped frames into ICON fine-tune groups. |
| experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py | Wire labelmaps/masks through; reuse warped masks. |
| experiments/LongitudinalRegistration/3-run_registration_method_comparison.py | Use forward transform for landmark warps. |
| experiments/LongitudinalRegistration/registration_test.py | New single-pair sanity script. |
| experiments/Heart-GatedCT_To_USD/test_compare_registration_speed.py | Obsolete script removed. |
| tests/test_register_images_ants.py | Add NCC-based regression tests for initial-transform composition and Affine/Rigid presets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "lncc_sigma": self.lncc_sigma, | ||
| "loss_function_masking": self.uses_masks, | ||
| "use_label": self.uses_segmentations, | ||
| "use_label": False, |
| lambda_value: float = 1.5, | ||
| dice_loss_weight: float = 0.5, | ||
| lncc_sigma: int = 5, | ||
| lncc_sigma: int = 1, |
|
|
||
| # Debug knob: when non-empty, only these patient IDs are processed. | ||
| # Set to ``[]`` (or ``None``) to run the full cohort. | ||
| debug_subjects: list[str] = ["pm0002"] |
Comment on lines
+306
to
+328
| main_module = sys.modules.get("__main__") | ||
| main_file = getattr(main_module, "__file__", None) | ||
| top_dir = pathlib.Path.cwd() | ||
| if main_file is not None: | ||
| top_dir = pathlib.Path(main_file).resolve().parent | ||
| if self.use_multi_modality: | ||
| if self.weights_path is None: | ||
| self.weights_path = str( | ||
| top_dir | ||
| / "network_weights" | ||
| / "multigradicon1.0" | ||
| / "Step_2_final.trch" | ||
| ) | ||
| self.net = get_multigradicon( | ||
| loss_fn=icon.LNCC(sigma=5), | ||
| apply_intensity_conservation_loss=self.use_mass_preservation, | ||
| weights_location=self.weights_path, | ||
| ) | ||
| else: | ||
| if self.weights_path is None: | ||
| self.weights_path = str( | ||
| top_dir / "network_weights" / "unigradicon1.0" / "Step_2_final.trch" | ||
| ) |
![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)
experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py (1)
165-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the fixed labelmap into the registrar too.
fixed_labelmapis loaded on Line 136, but this setup only callsset_fixed_mask().RegisterTimeSeriesImagesforwards fixed-side labelmap logic throughset_fixed_labelmap(...), so the newmoving_labelmaps=argument is currently one-sided and won't activate any fixed/moving labelmap path.Suggested fix
registrar = RegisterTimeSeriesImages(registration_method="ICON") registrar.set_modality("ct") registrar.set_fixed_image(fixed_image) registrar.set_fixed_mask(fixed_mask) + registrar.set_fixed_labelmap(fixed_labelmap) registrar.set_number_of_iterations_ICON(icon_iterations) if weights_path is not None: registrar.registrar_ICON.set_weights_path(str(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 `@experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py` around lines 165 - 176, The registrar setup omits the fixed-side labelmap, so when you call RegisterTimeSeriesImages and later register_time_series with moving_labelmaps the fixed/moving labelmap path isn't activated; call registrar.set_fixed_labelmap(fixed_labelmap) after set_fixed_mask (using the fixed_labelmap variable loaded earlier) so the registrar has both fixed and moving labelmaps before register_time_series is invoked, ensuring RegisterTimeSeriesImages's fixed/moving labelmap logic runs.src/physiomotion4d/workflow_fine_tune_icon_registration.py (1)
520-533:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't disable label supervision in the shared workflow.
Hard-coding
training.use_labeltoFalsemeans callers that pass segmentations still build segmentation entries and may skip frames missing segs inprepare_dataset(), but the generated YAML will never enable label loss. That breaks this class's data-driven contract and the existing expectation intests/test_workflow_fine_tune_icon_registration.py.Suggested fix
- "use_label": False, + "use_label": self.uses_segmentations,🤖 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_fine_tune_icon_registration.py` around lines 520 - 533, The YAML generator hard-codes "use_label": False which disables label supervision; change it to respect the instance setting by replacing the False with self.use_label (or use getattr(self, "use_label", True) if the attribute may not exist) so the generated config reflects whether labels are provided; update the code around where "training": {... "use_label": ... } is assembled (refer to use_label / uses_masks fields and prepare_dataset() in this class) to ensure tests and data-driven behavior remain correct.
🧹 Nitpick comments (2)
AGENTS.md (1)
127-128: ⚡ Quick winSync downstream agent rule docs with this new quote policy.
This change is good, but
.agents/agents/implementation.mdstill says “single quotes for strings,” which can cause conflicting agent behavior. Please update that rule to match this source-of-truth guidance.🤖 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.md` around lines 127 - 128, Update the downstream agent rule that currently reads "single quotes for strings" in implementation.md to match AGENTS.md's policy "Double quotes for strings and docstrings. Keep lines at or below 88 characters." Replace the rule text and any examples in implementation.md so they use double quotes for strings and docstrings and enforce the 88-character line limit; search for the exact phrase "single quotes for strings" to locate all occurrences and adjust examples and explanatory text accordingly.src/physiomotion4d/register_images_base.py (1)
62-63: ⚡ Quick winUse double quotes in docstring examples for string literals.
The updated examples still use single-quoted keys; switch to double quotes to match the repository Python style rule.
Proposed docstring fix
- ... 'forward_transform': tfm_forward, # warps moving image -> fixed grid - ... 'inverse_transform': tfm_inverse, # warps fixed image -> moving grid + ... "forward_transform": tfm_forward, # warps moving image -> fixed grid + ... "inverse_transform": tfm_inverse, # warps fixed image -> moving grid @@ - >>> forward_tfm = result['forward_transform'] # warps moving image -> fixed grid - >>> inverse_tfm = result['inverse_transform'] # warps fixed image -> moving grid + >>> forward_tfm = result["forward_transform"] # warps moving image -> fixed grid + >>> inverse_tfm = result["inverse_transform"] # warps fixed image -> moving gridAs per coding guidelines, "Use double quotes for strings and docstrings".
Also applies to: 71-72
🤖 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_images_base.py` around lines 62 - 63, Update the docstring examples to use double quotes for all string literals: replace occurrences of single-quoted keys like 'forward_transform' and 'inverse_transform' (and the similar single-quoted examples around lines 71-72) with double-quoted versions ("forward_transform", "inverse_transform") so they follow the repository style rule for strings/docstrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/LongitudinalRegistration/1-preregistration.py`:
- Around line 89-91: The debug_subjects default currently limits processing to a
single patient ("pm0002"), so change the debug_subjects variable in
1-preregistration.py to an empty list or None (e.g., debug_subjects: list[str] =
[] or debug_subjects = None) so the script runs the full cohort by default;
ensure any downstream checks that test debug_subjects handle both [] and None
(or adjust them) to avoid silently filtering the cohort.
In `@experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py`:
- Around line 41-46: The hard-coded finetuned_weights_path uses
"network_weights_final.trch" but the fine-tune workflow emits
"Finetune_multi_final.trch" (see
WorkflowFineTuneICONRegistration.expected_weights_path()); update
finetuned_weights_path to reference the actual checkpoint name produced by the
fine-tune workflow—either replace the filename with "Finetune_multi_final.trch"
or, better, compute the path by calling
WorkflowFineTuneICONRegistration.expected_weights_path()/equivalent helper so
the eval script points to the exact checkpoint the finetune workflow produces
(refer to finetuned_weights_path and
WorkflowFineTuneICONRegistration.expected_weights_path).
In `@src/physiomotion4d/register_images_ants.py`:
- Around line 597-611: When an initial_forward_transform is applied you pre-warp
the moving image (using TransformTools.transform_image into
self.moving_image_pre) but forget to pre-warp the corresponding moving mask,
leaving self.moving_mask in the original moving grid and causing masked ANTs
runs to use inconsistent coordinate systems; update the same pre-warp block to
also transform self.moving_mask with TransformTools.transform_image (into the
fixed grid) so both self.moving_image_pre and self.moving_mask are aligned
before the masked registration, and apply the same fix for the other pre-warp
occurrence around the later registration path that mirrors this logic.
- Around line 597-611: The code pre-warps the moving image using
TransformTools.transform_image but resamples it onto self.fixed_image while
ants.registration later uses self.fixed_image_pre as the fixed image; change the
resampling target to self.fixed_image_pre so the pre-warped moving image and the
subsequent registration share the exact same grid (update the call in the block
that checks initial_forward_transform inside register_images_ants.py to pass
self.fixed_image_pre instead of self.fixed_image, referencing
self.moving_image_pre, initial_forward_transform, and
TransformTools.transform_image).
In `@src/physiomotion4d/register_images_greedy.py`:
- Around line 158-160: The command-building currently injects the temp affine
filepath (created via tempfile.mkstemp and saved with np.savetxt into variable
initial_affine_file) directly into the Greedy command string (see uses in cmd
and cmd_def), which breaks if the temp path contains spaces; change to pass the
init-transform path via the picsl-greedy wrapper’s placeholder/kwargs
substitution (i.e., use the wrapper’s parameter for "-ia"/"-it" instead of
f-string interpolation), or if substitution isn’t available create the temp file
in a guaranteed space-free directory like /tmp (use
tempfile.NamedTemporaryFile(dir="/tmp", delete=False) or equivalent) and
reference that variable without string concatenation so the wrapper receives it
as a single argument; update usages around where cmd += f" -ia
{initial_affine_file}" and the "-it {initial_affine_file}" insertion in cmd_def
accordingly.
- Around line 140-160: The function _write_affine_matrix_file writes a 4x4 RAS
matrix but callers (e.g., registration_method building initial_affine from
initial_forward_transform.GetMatrix()/GetTranslation()/GetCenter) supply ITK/LPS
matrices, so the .mat is in the wrong convention; fix by converting the incoming
LPS affine into Greedy/RAS before saving: construct S = np.diag([-1, -1, 1, 1])
and replace the write value with mat_ras = S @ mat_4x4 @ S (then save mat_ras),
and update the _write_affine_matrix_file docstring to state it accepts an
ITK/LPS matrix and writes a Greedy RAS .mat (or explicitly note the conversion).
In `@src/physiomotion4d/register_images_icon.py`:
- Around line 93-95: The docstring for the setter set_weights_path in
register_images_icon.py is contradictory: it currently says the target file must
not exist (for downloading) but the setter is also used to point to an existing
custom checkpoint; update the docstring to clearly state both behaviors — if the
path points to a non-existent file the weights will be downloaded there (typical
suffix ".trch"), and if it points to an existing file it will be used as the
custom checkpoint to load — and remove the absolute "must not exist" wording
while giving both cases and expected suffix/format.
- Around line 306-328: The code currently sets top_dir to the parent of
__main__.__file__ without validating that the script lives inside the project,
which can point to non-writable launcher dirs; change the top_dir selection
logic in the block initializing top_dir/main_file so that you: (1) start with
pathlib.Path.cwd() as default, (2) if main_file is not None, resolve it and only
adopt its parent when that parent is a descendant of the project cwd or contains
a project marker (e.g., .git, pyproject.toml, setup.py), otherwise keep cwd; (3)
additionally verify the chosen top_dir is writable and if not fallback to a
writable directory (e.g., user home or cwd); update the code around
main_module/main_file/top_dir and use this validated top_dir when building
default self.weights_path for both use_multi_modality branches so
get_multigradicon and the unigradicon path logic use the safe, writable
fallback.
In `@src/physiomotion4d/register_time_series_images.py`:
- Around line 78-79: Update the example string literals to use double quotes:
replace single-quoted strings in the example assignments referencing
result['forward_transforms'] and result['inverse_transforms'] (symbols:
forward_tfms, inverse_tfms, result) with double-quoted equivalents, and apply
the same change to the other occurrences noted (the similar examples around the
occurrences of those variables later in the file).
- Around line 214-215: The docstring for the parameter fixed_labelmap currently
documents it as required itk.Image but the function signature uses
Optional[itk.Image]; update the Args section to reflect Optional[itk.Image] (or
itk.Image | None) and indicate it may be None/default omitted, and adjust the
description to say it is an optional labelmap defining the ROI; locate the
parameter entry for fixed_labelmap in the docstring of the
register_time_series_images function and change its type and wording
accordingly.
In `@tests/test_register_images_ants.py`:
- Around line 333-543: Add coverage that validates the inverse composition path
by warping the fixed image using the returned inverse transform and scoring
against the original moving image grid: after each registration call that
currently uses result["forward_transform"] (e.g., baseline, identity_result,
prior_result, and the matrix/transform_type loops), also retrieve
result["inverse_transform"] and call
TransformTools().transform_image(fixed_image, inverse_transform, moving_image,
interpolation_method="linear") then compute _foreground_ncc between
itk.array_from_image(moving_image) and the warped-fixed array (using the same
foreground mask) and assert it meets the same improvement/equality thresholds as
the forward checks; ensure you reference result["inverse_transform"] and
TransformTools.transform_image when adding these assertions.
---
Outside diff comments:
In `@experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py`:
- Around line 165-176: The registrar setup omits the fixed-side labelmap, so
when you call RegisterTimeSeriesImages and later register_time_series with
moving_labelmaps the fixed/moving labelmap path isn't activated; call
registrar.set_fixed_labelmap(fixed_labelmap) after set_fixed_mask (using the
fixed_labelmap variable loaded earlier) so the registrar has both fixed and
moving labelmaps before register_time_series is invoked, ensuring
RegisterTimeSeriesImages's fixed/moving labelmap logic runs.
In `@src/physiomotion4d/workflow_fine_tune_icon_registration.py`:
- Around line 520-533: The YAML generator hard-codes "use_label": False which
disables label supervision; change it to respect the instance setting by
replacing the False with self.use_label (or use getattr(self, "use_label", True)
if the attribute may not exist) so the generated config reflects whether labels
are provided; update the code around where "training": {... "use_label": ... }
is assembled (refer to use_label / uses_masks fields and prepare_dataset() in
this class) to ensure tests and data-driven behavior remain correct.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 127-128: Update the downstream agent rule that currently reads
"single quotes for strings" in implementation.md to match AGENTS.md's policy
"Double quotes for strings and docstrings. Keep lines at or below 88
characters." Replace the rule text and any examples in implementation.md so they
use double quotes for strings and docstrings and enforce the 88-character line
limit; search for the exact phrase "single quotes for strings" to locate all
occurrences and adjust examples and explanatory text accordingly.
In `@src/physiomotion4d/register_images_base.py`:
- Around line 62-63: Update the docstring examples to use double quotes for all
string literals: replace occurrences of single-quoted keys like
'forward_transform' and 'inverse_transform' (and the similar single-quoted
examples around lines 71-72) with double-quoted versions ("forward_transform",
"inverse_transform") so they follow the repository style rule for
strings/docstrings.
🪄 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: 39d7ea3b-8dba-43f9-b50d-891b3e1d2918
📒 Files selected for processing (26)
AGENTS.mdCLAUDE.mddocs/contributing.rstdocs/developer/registration_images.rstdocs/developer/registration_models.rstdocs/developer/transform_conventions.rstdocs/index.rstexperiments/Heart-GatedCT_To_USD/test_compare_registration_speed.pyexperiments/LongitudinalRegistration/1-finetune_icon.pyexperiments/LongitudinalRegistration/1-preregistration.pyexperiments/LongitudinalRegistration/2-recon_4d_icon_eval.pyexperiments/LongitudinalRegistration/3-run_registration_method_comparison.pyexperiments/LongitudinalRegistration/registration_test.pypyproject.tomlsrc/physiomotion4d/register_images_ants.pysrc/physiomotion4d/register_images_base.pysrc/physiomotion4d/register_images_greedy.pysrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/register_models_distance_maps.pysrc/physiomotion4d/register_models_icp.pysrc/physiomotion4d/register_models_pca.pysrc/physiomotion4d/register_time_series_images.pysrc/physiomotion4d/workflow_fine_tune_icon_registration.pysrc/physiomotion4d/workflow_reconstruct_highres_4d_ct.pytests/test_register_images_ants.pytutorials/tutorial_08_dirlab_pca_time_series.py
💤 Files with no reviewable changes (1)
- experiments/Heart-GatedCT_To_USD/test_compare_registration_speed.py
Comment on lines
+89
to
+91
| # Debug knob: when non-empty, only these patient IDs are processed. | ||
| # Set to ``[]`` (or ``None``) to run the full cohort. | ||
| debug_subjects: list[str] = ["pm0002"] |
There was a problem hiding this comment.
Remove the default single-subject filter.
Line 91 ships with ["pm0002"], so this script never performs the cohort-wide comparison promised by its header/comments unless the user edits the file first. That silently skews the summary CSV/table and leaves downstream preregistration augmentation incomplete by default.
Suggested fix
- debug_subjects: list[str] = ["pm0002"]
+ debug_subjects: list[str] = []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/LongitudinalRegistration/1-preregistration.py` around lines 89 -
91, The debug_subjects default currently limits processing to a single patient
("pm0002"), so change the debug_subjects variable in 1-preregistration.py to an
empty list or None (e.g., debug_subjects: list[str] = [] or debug_subjects =
None) so the script runs the full cohort by default; ensure any downstream
checks that test debug_subjects handle both [] and None (or adjust them) to
avoid silently filtering the cohort.
Comment on lines
+41
to
+46
| finetuned_weights_path = ( | ||
| output_dir | ||
| / "icon_finetuned" | ||
| / "icon_finetuned_model" | ||
| / "checkpoints" | ||
| / "network_weights_final.trch" |
There was a problem hiding this comment.
Point this at the checkpoint 1-finetune_icon.py actually produces.
This hard-coded filename doesn't match WorkflowFineTuneICONRegistration.expected_weights_path(), which still resolves to .../checkpoints/Finetune_multi_final.trch. As written, the eval script won't find the checkpoint emitted by the companion fine-tuning workflow.
Suggested fix
finetuned_weights_path = (
output_dir
/ "icon_finetuned"
/ "icon_finetuned_model"
/ "checkpoints"
- / "network_weights_final.trch"
+ / "Finetune_multi_final.trch"
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py` around lines 41
- 46, The hard-coded finetuned_weights_path uses "network_weights_final.trch"
but the fine-tune workflow emits "Finetune_multi_final.trch" (see
WorkflowFineTuneICONRegistration.expected_weights_path()); update
finetuned_weights_path to reference the actual checkpoint name produced by the
fine-tune workflow—either replace the filename with "Finetune_multi_final.trch"
or, better, compute the path by calling
WorkflowFineTuneICONRegistration.expected_weights_path()/equivalent helper so
the eval script points to the exact checkpoint the finetune workflow produces
(refer to finetuned_weights_path and
WorkflowFineTuneICONRegistration.expected_weights_path).
Comment on lines
+597
to
611
| # Apply any initial transform by pre-warping the moving image onto the | ||
| # fixed grid (the same approach RegisterImagesICON uses), instead of | ||
| # passing it to ants.registration as an initial_transform. ANTS | ||
| # mishandles matrix (affine/translation) initial transforms, badly | ||
| # corrupting the result; pre-warping keeps the composition below | ||
| # self-consistent for any initial transform type. The registration then | ||
| # solves the residual and the composition recovers the full transform. | ||
| if initial_forward_transform is not None: | ||
| self.log_info("Converting initial ITK transform to ANTs format...") | ||
| initial_transform = self.itk_transform_to_ANTSfile( | ||
| itk_tfm=initial_forward_transform, | ||
| reference_image=self.fixed_image, | ||
| output_filename="initial_transform_temp.mat", | ||
| self.log_info("Pre-warping moving image with initial transform...") | ||
| transform_tools = TransformTools() | ||
| self.moving_image_pre = transform_tools.transform_image( | ||
| self.moving_image_pre, | ||
| initial_forward_transform, | ||
| self.fixed_image, | ||
| ) |
There was a problem hiding this comment.
Pre-warp the moving mask when an initial transform is supplied.
The new path moves self.moving_image_pre onto the fixed grid, but self.moving_mask stays on the original moving grid and is still passed into the masked ANTs call. That gives ANTs a moving image and moving mask in different coordinate systems, so masked runs can optimize the wrong ROI.
Also applies to: 668-675
🤖 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_images_ants.py` around lines 597 - 611, When an
initial_forward_transform is applied you pre-warp the moving image (using
TransformTools.transform_image into self.moving_image_pre) but forget to
pre-warp the corresponding moving mask, leaving self.moving_mask in the original
moving grid and causing masked ANTs runs to use inconsistent coordinate systems;
update the same pre-warp block to also transform self.moving_mask with
TransformTools.transform_image (into the fixed grid) so both
self.moving_image_pre and self.moving_mask are aligned before the masked
registration, and apply the same fix for the other pre-warp occurrence around
the later registration path that mirrors this logic.
Pre-warp onto the same fixed grid ANTs actually registers against.
self.moving_image_pre is resampled against self.fixed_image, but the subsequent ants.registration() call uses self.fixed_image_pre as fixed. If preprocess() changes spacing/origin/size, the residual registration is solving on a different domain than the one used for the initial alignment.
Suggested fix
if initial_forward_transform is not None:
self.log_info("Pre-warping moving image with initial transform...")
transform_tools = TransformTools()
self.moving_image_pre = transform_tools.transform_image(
self.moving_image_pre,
initial_forward_transform,
- self.fixed_image,
+ self.fixed_image_pre,
)🤖 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_images_ants.py` around lines 597 - 611, The code
pre-warps the moving image using TransformTools.transform_image but resamples it
onto self.fixed_image while ants.registration later uses self.fixed_image_pre as
the fixed image; change the resampling target to self.fixed_image_pre so the
pre-warped moving image and the subsequent registration share the exact same
grid (update the call in the block that checks initial_forward_transform inside
register_images_ants.py to pass self.fixed_image_pre instead of
self.fixed_image, referencing self.moving_image_pre, initial_forward_transform,
and TransformTools.transform_image).
Comment on lines
+140
to
+160
| def _write_affine_matrix_file(self, mat_4x4: NDArray[np.float64]) -> str: | ||
| """Write a 4x4 RAS affine matrix to a temporary Greedy ``.mat`` file. | ||
|
|
||
| Greedy's in-memory interface corrupts the heap when a numpy affine | ||
| matrix is supplied as an initial transform (``-ia``/``-it``); passing a | ||
| file path instead avoids that native crash. Greedy reads a plain-text | ||
| 4x4 RAS matrix, which is what ``numpy.savetxt`` writes here. | ||
|
|
||
| Args: | ||
| mat_4x4: 4x4 affine matrix in RAS (Greedy) convention. | ||
|
|
||
| Returns: | ||
| Path to the written temporary ``.mat`` file. The caller is | ||
| responsible for deleting it. | ||
| """ | ||
| mat_4x4 = np.asarray(mat_4x4, dtype=np.float64) | ||
| if mat_4x4.shape != (4, 4): | ||
| raise ValueError(f"Expected 4x4 matrix, got shape {mat_4x4.shape}") | ||
| fd, path = tempfile.mkstemp(suffix=".mat", prefix="greedy_aff_") | ||
| os.close(fd) | ||
| np.savetxt(path, mat_4x4, fmt="%.10f") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/register_images_greedy.py"
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== Extract: _write_affine_matrix_file() (lines ~140-200) =="
nl -ba "$FILE" | sed -n '120,210p'
echo
echo "== Search for _write_affine_matrix_file( calls =="
rg -n "_write_affine_matrix_file\s*\(" "$FILE"
echo
echo "== Extract: call sites around those matches =="
python3 - <<'PY'
import re, subprocess, os, sys, textwrap
FILE="src/physiomotion4d/register_images_greedy.py"
out=subprocess.check_output(["rg","-n",r"_write_affine_matrix_file\s*\(",FILE],text=True)
lines=[]
for line in out.strip().splitlines():
m=re.match(r".+:(\d+):",line)
if m: lines.append(int(m.group(1)))
for ln in sorted(lines):
start=max(1,ln-25); end=ln+45
print(f"\n--- Context for call at line {ln} ({start}-{end}) ---")
subprocess.run(["bash","-lc",f"nl -ba {FILE} | sed -n '{start},{end}p'"],check=True)
PY
echo
echo "== Extract: referenced lines 228-229 =="
nl -ba "$FILE" | sed -n '215,245p'
echo
echo "== Extract: referenced lines 275-278 =="
nl -ba "$FILE" | sed -n '255,305p'
echo
echo "== Check for print() usage and itk.imwrite usage in this file =="
rg -n "print\(" "$FILE" || true
rg -n "itk\.imwrite|imwrite\(" "$FILE" || true
echo
echo "== Check for string-double-quotes and line length violations (rough) =="
# Quick heuristic: find long lines > 120 just to catch obvious issues
python3 - <<'PY'
import pathlib
FILE=pathlib.Path("src/physiomotion4d/register_images_greedy.py")
long=[]
for i,line in enumerate(FILE.read_text().splitlines(),start=1):
if len(line)>120:
long.append((i,len(line),line[:140]))
print("Long lines >120:",len(long))
for i,l,txt in long[:30]:
print(i,l,txt.replace("\t"," "))
PYRepository: Project-MONAI/physiomotion4d
Length of output: 301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/register_images_greedy.py"
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== Extract: _write_affine_matrix_file() (lines 120-220) =="
python3 - <<'PY'
import itertools
path="src/physiomotion4d/register_images_greedy.py"
start, end = 120, 220
with open(path, "r", encoding="utf-8") as f:
for i, line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:4d}: {line.rstrip()}")
if i > end:
break
PY
echo
echo "== Search for _write_affine_matrix_file( calls =="
rg -n "_write_affine_matrix_file\s*\(" "$FILE" || true
echo
echo
echo "== Extract: context around each call =="
python3 - <<'PY'
import re, subprocess
FILE="src/physiomotion4d/register_images_greedy.py"
text=subprocess.check_output(["rg","-n",r"_write_affine_matrix_file\s*\(",FILE],text=True)
lines=[]
for m in re.finditer(r"^.+:(\d+):",text,flags=re.M):
lines.append(int(m.group(1)))
for ln in sorted(set(lines)):
start=max(1,ln-25); end=ln+45
print(f"\n--- call at line {ln} (show {start}-{end}) ---")
with open(FILE,"r",encoding="utf-8") as f:
for i,line in enumerate(f,start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end:
break
PY
echo
echo "== Extract: referenced lines 228-240 =="
python3 - <<'PY'
path="src/physiomotion4d/register_images_greedy.py"
start,end=228,240
with open(path,"r",encoding="utf-8") as f:
for i,line in enumerate(f,start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end: break
PY
echo
echo "== Extract: referenced lines 275-290 =="
python3 - <<'PY'
path="src/physiomotion4d/register_images_greedy.py"
start,end=275,290
with open(path,"r",encoding="utf-8") as f:
for i,line in enumerate(f,start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end: break
PY
echo
echo "== Check for print() usage and itk.imwrite usage in this file =="
rg -n "print\(" "$FILE" || true
rg -n "itk\.imwrite|imwrite\(" "$FILE" || true
echo
echo
echo "== Check for long lines > 120 chars =="
python3 - <<'PY'
import pathlib
FILE=pathlib.Path("src/physiomotion4d/register_images_greedy.py")
long=[]
for i,line in enumerate(FILE.read_text(encoding="utf-8").splitlines(),start=1):
if len(line)>120:
long.append((i,len(line),line))
print("Count:",len(long))
for i,l,txt in long[:20]:
print(i,l,txt[:160].replace("\t"," "))
PYRepository: Project-MONAI/physiomotion4d
Length of output: 6864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/register_images_greedy.py"
echo "== Search for initial_affine derivation and related symbols =="
rg -n "initial_affine|initial_forward_transform|LPS|RAS|diag\\(\\-1|lps_to_ras|ras_to_lps" "$FILE"
echo
echo "== Extract: lines around initial_affine assignments (broad) =="
python3 - <<'PY'
import re
path="src/physiomotion4d/register_images_greedy.py"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
hits=[]
for i,l in enumerate(lines, start=1):
if re.search(r"\binitial_affine\b", l):
hits.append(i)
for ln in sorted(set(hits)):
start=max(1,ln-35); end=min(len(lines), ln+35)
print(f"\n--- around line {ln} ({start}-{end}) ---")
for j in range(start,end+1):
print(f"{j:4d}: {lines[j-1].rstrip()}")
PY
echo
echo "== Search for where transforms are converted to arrays (matrix extraction) =="
rg -n "to.*array|GetMatrix|Transform|matrix|affine.*matrix|array_from" "$FILE"
echo
echo "== Check for any coordinate-convention conversions in this module =="
rg -n "ras|lps|world|convention|orientation" "$FILE"Repository: Project-MONAI/physiomotion4d
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/register_images_greedy.py"
echo "== print() usage =="
rg -n "print\(" "$FILE" || true
echo
echo "== Long lines >88 chars (count + samples) =="
python3 - <<'PY'
import pathlib
path=pathlib.Path("src/physiomotion4d/register_images_greedy.py")
long=[]
for i,line in enumerate(path.read_text(encoding="utf-8").splitlines(), start=1):
if len(line)>88:
long.append((i,len(line),line))
print("Count >",88,":",len(long))
for i,l,txt in long[:25]:
print(i,l,txt.replace("\t"," ")[:140])
PY
echo
echo "== itk.imwrite usage (if any) =="
rg -n "itk\.imwrite|itk\.ImageFileWriter|imwrite\(" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for LPS/RAS convention docs and any conversion helpers.
rg -n "LPS|RAS|lps_to_ras|ras_to_lps|diag\\(\\[-1\\.|transform_conventions" . -S
echo
echo "== Find Greedy/greedy_aff usage =="
rg -n "greedy_aff_|Greedy3D|picsl_greedy|Greedy" src/physiomotion4d -S
echo
echo "== Inspect _try_import_greedy definition =="
rg -n "def _try_import_greedy|_try_import_greedy\\(" src/physiomotion4d/register_images_greedy.py src/physiomotion4d -S
# Print the Greedy-related imports / helper defs at top of the file
echo
echo "== Top of register_images_greedy.py (imports/utilities) =="
python3 - <<'PY'
path="src/physiomotion4d/register_images_greedy.py"
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f,start=1):
if i>120: break
print(f"{i:4d}: {line.rstrip()}")
PYRepository: Project-MONAI/physiomotion4d
Length of output: 24510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect transform conventions doc for LPS/RAS/Greedy notes
for f in docs/developer/transform_conventions.* docs/developer/transform_conventions/*; do
if [ -e "$f" ]; then
echo "== Found: $f =="
ls -la "$f" | head -n 20
fi
done
# Greedy/picsl-specific mentions in docs
rg -n "Greedy|picsl-greedy|picsl_greedy|-ia|-it|\\.mat|RAS|LPS|affine" docs -S || true
# Inspect how ANTs backend serializes initial transforms (for comparison)
FILE_ANTS="src/physiomotion4d/register_images_ants.py"
if [ -e "$FILE_ANTS" ]; then
rg -n "initial_forward_transform|initial_affine|-ia|-it|\\.mat|RAS|LPS" "$FILE_ANTS" -S || true
fi
# Inspect RegisterImagesBase conventions handling
FILE_BASE="src/physiomotion4d/register_images_base.py"
if [ -e "$FILE_BASE" ]; then
rg -n "transform_conventions|RAS|LPS|matrix|affine" "$FILE_BASE" -S || true
fiRepository: Project-MONAI/physiomotion4d
Length of output: 4154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
DOC="docs/developer/transform_conventions.rst"
echo "== File: $DOC =="
wc -l "$DOC"
echo
echo "== Extract: Greedy / RAS / LPS sections =="
# show lines containing Greedy or RAS/LPS mentions in the doc
rg -n "Greedy|picsl-greedy|picsl_greedy|RAS|LPS|\\-ia|\\-it|\\.mat|affine init|matrix" "$DOC" -n -S || true
# Print the whole doc around those matches (limit output)
python3 - <<'PY'
import re
path="docs/developer/transform_conventions.rst"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
need=[]
for i,l in enumerate(lines, start=1):
if re.search(r"Greedy|picsl-greedy|picsl_greedy|RAS|LPS|\-ia|\-it|\.mat|affine init|matrix", l):
need.append(i)
# de-dup sorted
need=sorted(set(need))
print("Matches:",len(need))
# print up to first 8 match contexts
shown=0
for ln in need:
start=max(1,ln-18); end=min(len(lines),ln+25)
if shown>=8: break
print(f"\n--- context around {ln} ({start}-{end}) ---")
for j in range(start,end+1):
print(f"{j:4d}: {lines[j-1].rstrip()}")
shown+=1
PYRepository: Project-MONAI/physiomotion4d
Length of output: 5168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
DOC="docs/api/registration/greedy.rst"
echo "== File: $DOC =="
wc -l "$DOC"
echo
echo "== Extract: initial_forward_transform / -ia / -it / .mat =="
rg -n "initial_forward_transform|-ia|-it|\\.mat|RAS|LPS|matrix" "$DOC" -S || true
echo
echo "== Context around those matches (limited) =="
python3 - <<'PY'
import re
path="docs/api/registration/greedy.rst"
lines=open(path,'r',encoding='utf-8').read().splitlines()
patterns=[r"initial_forward_transform", r"-ia", r"-it", r"\.mat", r"RAS", r"LPS", r"matrix"]
idx=[i+1 for i,l in enumerate(lines) if any(re.search(p,l) for p in patterns)]
idx=sorted(set(idx))
print("Matches:",len(idx))
for ln in idx[:12]:
start=max(1,ln-12); end=min(len(lines),ln+20)
print(f"\n--- context around line {ln} ({start}-{end}) ---")
for j in range(start,end+1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: Project-MONAI/physiomotion4d
Length of output: 271
🌐 Web query:
picsl-greedy -ia -it mat coordinate system RAS LPS
💡 Result:
picsl-greedy (Greedy) uses the standard medical-image anatomical conventions, and the -it / -o affine matrix it writes is expressed in RAS voxel-coordinate ordering (not LPS). 1) What -it means - In Greedy, -it supplies a chain of transforms (affine matrices and/or warps) that map the moving image into a reference space (i.e., it is applied to the moving image before registration). It is described as equivalent to reslicing the moving image into the fixed image space using the same chain of transformations. [1] 2) What coordinate system the written affine matrix uses (RAS) - Greedy’s documentation states that the -o output affine matrix maps voxels in fixed image space to voxels in moving image space, and gives the explicit relationship using ras(·): - [ras(B); 1] = Matrix * [ras(B); 1] [1] - This indicates the affine matrix representation is in “ras” coordinates (RAS), not “lps”. [1] 3) How this relates to RAS vs LPS in general - Many medical-imaging conventions use LPS (common in DICOM), while RAS is used internally by some toolkits; the difference between RAS and LPS is that the sign of the first two coordinate axes is inverted. [2] - Therefore, if you are interoperating with DICOM/LPS-based systems, you may need to flip the first two axes when converting to/from the Greedy (RAS) convention. [2] Answer summary - For the specific Greedy/picsl-greedy question “mat coordinate system RAS LPS”: Greedy’s -o affine “mat” is expressed using ras(·) coordinates (RAS). [1] If you tell me which software you’re converting to/from (e.g., SimpleITK, 3D Slicer, elastix, ITK), I can outline the exact axis-flip/convention mapping you need.
Citations:
- 1: https://greedy.readthedocs.io/en/latest/reference.html
- 2: https://slicer.readthedocs.io/en/5.8/user_guide/coordinate_systems.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
BASE="src/physiomotion4d/register_images_base.py"
echo "== File: $BASE =="
wc -l "$BASE"
echo
echo "== Search for RAS/LPS and any conversion helpers in base =="
rg -n "RAS|LPS|ras_to|lps_to|diag\\(\\-1|transform_conventions|Greedy|picsl-greedy|\\.mat" "$BASE" -S || true
echo
echo "== Extract: likely backend boundary/convention code (lines 240-380) =="
python3 - <<'PY'
path="src/physiomotion4d/register_images_base.py"
start,end=240,420
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f,start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end: break
PY
echo
echo "== Extract: around compose/initial_forward_transform handling (search in file) =="
rg -n "initial_forward_transform|compose|forward_transform|inverse_transform|transform_image|transform_dataset" "$BASE" -S || trueRepository: Project-MONAI/physiomotion4d
Length of output: 9965
Fix Greedy temp-file affine convention (LPS→RAS) for -ia/-it
_write_affine_matrix_file() writes a “4x4 RAS” matrix, but registration_method() builds initial_affine directly from initial_forward_transform.GetMatrix()/GetTranslation()/GetCenter (ITK/LPS convention) and passes it through unchanged (lines ~140-162, ~342-356). With PICSL Greedy, the -ia/-it .mat is interpreted in RAS voxel-coordinate ordering, so the initial transform can be serialized in the wrong basis.
Convert from ITK/LPS to Greedy/RAS at the Greedy boundary (or change the helper contract/docstring to accept ITK/LPS and convert before writing).
🤖 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_images_greedy.py` around lines 140 - 160, The
function _write_affine_matrix_file writes a 4x4 RAS matrix but callers (e.g.,
registration_method building initial_affine from
initial_forward_transform.GetMatrix()/GetTranslation()/GetCenter) supply ITK/LPS
matrices, so the .mat is in the wrong convention; fix by converting the incoming
LPS affine into Greedy/RAS before saving: construct S = np.diag([-1, -1, 1, 1])
and replace the write value with mat_ras = S @ mat_4x4 @ S (then save mat_ras),
and update the _write_affine_matrix_file docstring to state it accepts an
ITK/LPS matrix and writes a Greedy RAS .mat (or explicitly note the conversion).
Comment on lines
+158
to
+160
| fd, path = tempfile.mkstemp(suffix=".mat", prefix="greedy_aff_") | ||
| os.close(fd) | ||
| np.savetxt(path, mat_4x4, fmt="%.10f") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant sections in the file
FILE="src/physiomotion4d/register_images_greedy.py"
ls -l "$FILE" 2>/dev/null || true
# Show around the originally referenced lines (158-160)
nl -ba "$FILE" | sed -n '130,260p'
# Show any other nearby occurrences of "-ia" / "-it" and temp paths
rg -n --hidden --no-ignore-vcs "(-ia|-it|mkstemp|tempfile|savetxt|greedy_aff_)" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/register_images_greedy.py"
# Print around the referenced line ranges using awk (no nl dependency)
echo "=== Lines 140-190 ==="
awk 'NR>=140 && NR<=190 {printf "%5d %s\n", NR, $0}' "$FILE"
echo "=== Lines 215-245 ==="
awk 'NR>=215 && NR<=245 {printf "%5d %s\n", NR, $0}' "$FILE"
echo "=== Lines 255-310 ==="
awk 'NR>=255 && NR<=310 {printf "%5d %s\n", NR, $0}' "$FILE"
echo "=== Search for Greedy -ia/-it and temp path usage ==="
rg -n --hidden --no-ignore-vcs "(-ia|-it|mkstemp|tempfile|savetxt|greedy_aff_|cmd\\s*=|Greedy|greedy)" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 11260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search within repo for Greedy3D / execute implementation or wrappers
rg -n --hidden --no-ignore-vcs "picsl_greedy|Greedy3D|def execute|execute\\(" src/physiomotion4d || true
# Search for any quoting/shlex usage related to greedy commands
rg -n --hidden --no-ignore-vcs "shlex|quote\\(|mkdtemp\\(|tempdir|tempfile\\.gettempdir" src/physiomotion4d/register_images_greedy.py src/physiomotion4d || trueRepository: Project-MONAI/physiomotion4d
Length of output: 948
🌐 Web query:
picsl-greedy Greedy3D execute cmd kwargs placeholders -ia -it
💡 Result:
In the context of the picsl-greedy Python package, the Greedy3D.execute method is used to run greedy registration commands [1][2][3]. The greedy tool itself is a fast deformable registration software for 2D and 3D medical images [4][5][6]. Regarding your query about execute cmd kwargs placeholders like -ia and -it: 1. Placeholders and Keyword Arguments: The picsl-greedy Python wrapper allows you to substitute file paths in your greedy command string with arbitrary placeholders (e.g., my_fixed, my_moving) [2][3]. You then pass these placeholders as keyword arguments to the execute method, mapping them to objects in your Python environment such as SimpleITK images or NumPy arrays [2][3]. For example: g.execute('-i my_fixed my_moving -o my_output', my_fixed=img_obj, my_moving=img_obj) [2][3] 2. Standard Greedy Flags (-ia, -it): These are standard command-line flags for the underlying greedy registration tool, not specific to the Python wrapper's placeholder functionality [4]. * -ia: Often used in combination with other flags to specify input image affine/initialization parameters [4]. * -it: Typically stands for "Initial Transformation," used to provide a starting transformation matrix or field for the registration process [4]. The picsl-greedy wrapper simply passes these strings directly to the greedy binary [2][3]. You can use the standard greedy documentation to understand how these specific flags function within your registration pipeline [4].
Citations:
- 1: https://github.com/pyushkevich/greedy_python
- 2: https://pypi.org/project/picsl-greedy/0.0.3/
- 3: https://pypi.org/project/picsl-greedy/
- 4: https://sites.google.com/view/greedyreg/documentation
- 5: https://pypi.org/project/picsl-greedy/0.0.7/
- 6: https://sourceforge.net/projects/greedy-reg/files/
Avoid interpolating temporary affine file paths into Greedy command strings
src/physiomotion4d/register_images_greedy.py builds the Greedy commands by directly embedding the absolute temp path returned from tempfile.mkstemp():
cmd += f" -ia {initial_affine_file}"(around 229)-it {initial_affine_file}incmd_def(around 277-278)
If the temp directory path can contain spaces, whitespace tokenization can break the -ia/-it arguments. Use picsl-greedy’s placeholder/kwargs substitution for the init-transform argument instead of splicing the path into the command string; otherwise, ensure the temp file is created under a known space-free directory (e.g., /tmp) / escape in a way the wrapper will keep it as a single argument.
🤖 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_images_greedy.py` around lines 158 - 160, The
command-building currently injects the temp affine filepath (created via
tempfile.mkstemp and saved with np.savetxt into variable initial_affine_file)
directly into the Greedy command string (see uses in cmd and cmd_def), which
breaks if the temp path contains spaces; change to pass the init-transform path
via the picsl-greedy wrapper’s placeholder/kwargs substitution (i.e., use the
wrapper’s parameter for "-ia"/"-it" instead of f-string interpolation), or if
substitution isn’t available create the temp file in a guaranteed space-free
directory like /tmp (use tempfile.NamedTemporaryFile(dir="/tmp", delete=False)
or equivalent) and reference that variable without string concatenation so the
wrapper receives it as a single argument; update usages around where cmd += f"
-ia {initial_affine_file}" and the "-it {initial_affine_file}" insertion in
cmd_def accordingly.
Comment on lines
+93
to
+95
| Also, use this to specify the path to store the downloaded weights. The | ||
| file must not exist for the weights to be downloaded correctly. Typical | ||
| suffix is ".trch". |
There was a problem hiding this comment.
Fix contradictory set_weights_path contract text.
At Line 93, the docstring says the target file must not exist, but this setter is also documented/used for loading an existing custom checkpoint. That contradiction will mislead callers.
Proposed docstring fix
- Also, use this to specify the path to store the downloaded weights. The
- file must not exist for the weights to be downloaded correctly. Typical
- suffix is ".trch".
+ Also, this path is passed to the UniGradICON loader as ``weights_location``:
+ it can point to an existing checkpoint to load, or to a target path used
+ by the loader when downloading default weights. Typical suffix is ".trch".📝 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
| Also, use this to specify the path to store the downloaded weights. The | |
| file must not exist for the weights to be downloaded correctly. Typical | |
| suffix is ".trch". | |
| Also, this path is passed to the UniGradICON loader as ``weights_location``: | |
| it can point to an existing checkpoint to load, or to a target path used | |
| by the loader when downloading default weights. Typical suffix is ".trch". |
🤖 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_images_icon.py` around lines 93 - 95, The
docstring for the setter set_weights_path in register_images_icon.py is
contradictory: it currently says the target file must not exist (for
downloading) but the setter is also used to point to an existing custom
checkpoint; update the docstring to clearly state both behaviors — if the path
points to a non-existent file the weights will be downloaded there (typical
suffix ".trch"), and if it points to an existing file it will be used as the
custom checkpoint to load — and remove the absolute "must not exist" wording
while giving both cases and expected suffix/format.
Comment on lines
+306
to
+328
| main_module = sys.modules.get("__main__") | ||
| main_file = getattr(main_module, "__file__", None) | ||
| top_dir = pathlib.Path.cwd() | ||
| if main_file is not None: | ||
| top_dir = pathlib.Path(main_file).resolve().parent | ||
| if self.use_multi_modality: | ||
| if self.weights_path is None: | ||
| self.weights_path = str( | ||
| top_dir | ||
| / "network_weights" | ||
| / "multigradicon1.0" | ||
| / "Step_2_final.trch" | ||
| ) | ||
| self.net = get_multigradicon( | ||
| loss_fn=icon.LNCC(sigma=5), | ||
| apply_intensity_conservation_loss=self.use_mass_preservation, | ||
| weights_location=self.weights_path, | ||
| ) | ||
| else: | ||
| if self.weights_path is None: | ||
| self.weights_path = str( | ||
| top_dir / "network_weights" / "unigradicon1.0" / "Step_2_final.trch" | ||
| ) |
There was a problem hiding this comment.
Harden default weights root fallback when __main__.__file__ points outside the project.
At Line 309-Line 310, choosing script-parent unconditionally can resolve to non-writable launcher dirs (test runners, console entrypoints), which breaks default weight loading for callers that don’t set weights_path.
Proposed resilience fix
main_module = sys.modules.get("__main__")
main_file = getattr(main_module, "__file__", None)
top_dir = pathlib.Path.cwd()
if main_file is not None:
- top_dir = pathlib.Path(main_file).resolve().parent
+ candidate = pathlib.Path(main_file).resolve().parent
+ # Prefer script-relative directory only when it looks usable;
+ # otherwise keep cwd fallback.
+ if (candidate / "network_weights").exists() or candidate.is_dir():
+ top_dir = candidate🤖 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_images_icon.py` around lines 306 - 328, The code
currently sets top_dir to the parent of __main__.__file__ without validating
that the script lives inside the project, which can point to non-writable
launcher dirs; change the top_dir selection logic in the block initializing
top_dir/main_file so that you: (1) start with pathlib.Path.cwd() as default, (2)
if main_file is not None, resolve it and only adopt its parent when that parent
is a descendant of the project cwd or contains a project marker (e.g., .git,
pyproject.toml, setup.py), otherwise keep cwd; (3) additionally verify the
chosen top_dir is writable and if not fallback to a writable directory (e.g.,
user home or cwd); update the code around main_module/main_file/top_dir and use
this validated top_dir when building default self.weights_path for both
use_multi_modality branches so get_multigradicon and the unigradicon path logic
use the safe, writable fallback.
Comment on lines
+78
to
+79
| >>> forward_tfms = result['forward_transforms'] # warp moving images -> fixed grid | ||
| >>> inverse_tfms = result['inverse_transforms'] # warp fixed image -> moving grids |
There was a problem hiding this comment.
Use double quotes in changed Python examples.
Changed example snippets still use single quotes for string literals.
Proposed style fix
- >>> forward_tfms = result['forward_transforms'] # warp moving images -> fixed grid
- >>> inverse_tfms = result['inverse_transforms'] # warp fixed image -> moving grids
+ >>> forward_tfms = result["forward_transforms"] # warp moving images -> fixed grid
+ >>> inverse_tfms = result["inverse_transforms"] # warp fixed image -> moving grids
...
- ... moving_labelmaps=labelmap_list, # Optional
+ ... moving_labelmaps=labelmap_list, # Optional
...
- ... zip(result['forward_transforms'], result['losses'])
+ ... zip(result["forward_transforms"], result["losses"])As per coding guidelines: "Use double quotes for strings and docstrings".
Also applies to: 294-295
🤖 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_time_series_images.py` around lines 78 - 79,
Update the example string literals to use double quotes: replace single-quoted
strings in the example assignments referencing result['forward_transforms'] and
result['inverse_transforms'] (symbols: forward_tfms, inverse_tfms, result) with
double-quoted equivalents, and apply the same change to the other occurrences
noted (the similar examples around the occurrences of those variables later in
the file).
Comment on lines
+214
to
+215
| fixed_labelmap (itk.Image): Labelmap defining ROI | ||
| """ |
There was a problem hiding this comment.
Docstring type should match Optional[itk.Image].
fixed_labelmap is typed as optional in the signature, but the Args section documents it as required.
Proposed docstring fix
- fixed_labelmap (itk.Image): Labelmap defining ROI
+ fixed_labelmap (Optional[itk.Image]): Labelmap defining ROI, or NoneAs per coding guidelines: "Use full type hints with mypy strict mode; use Optional[X] not X | None".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fixed_labelmap (itk.Image): Labelmap defining ROI | |
| """ | |
| fixed_labelmap (Optional[itk.Image]): Labelmap defining ROI, or 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 `@src/physiomotion4d/register_time_series_images.py` around lines 214 - 215,
The docstring for the parameter fixed_labelmap currently documents it as
required itk.Image but the function signature uses Optional[itk.Image]; update
the Args section to reflect Optional[itk.Image] (or itk.Image | None) and
indicate it may be None/default omitted, and adjust the description to say it is
an optional labelmap defining the ROI; locate the parameter entry for
fixed_labelmap in the docstring of the register_time_series_images function and
change its type and wording accordingly.
Comment on lines
+333
to
+543
| def test_initial_transform_composition_metrics( | ||
| self, | ||
| registrar_ANTS: RegisterImagesANTS, | ||
| test_images: list[Any], | ||
| test_directories: dict[str, Path], | ||
| ) -> None: | ||
| """Verify the initial_forward_transform composition path with metrics. | ||
|
|
||
| Exercises the two initial-transform inputs the platform actually uses | ||
| (identity and a prior deformable forward_transform, as in prior-based | ||
| time-series registration) and confirms the composed forward_transform | ||
| warps the moving image onto the fixed grid. Scored with foreground NCC | ||
| over the brightest 30% of the fixed image (tissue/blood pool). See | ||
| docs/developer/transform_conventions. | ||
|
|
||
| Asserted facts: | ||
| * a plain registration improves on the unregistered pair, | ||
| * an identity initial reproduces the baseline exactly (the | ||
| composition machinery is a structurally correct no-op; note an | ||
| identity AffineTransform is itself a matrix initial), | ||
| * a prior-deformable initial reaches the no-initial baseline quality | ||
| (the composition recovers the full transform). | ||
|
|
||
| The initial transform is applied by pre-warping the moving image (as in | ||
| RegisterImagesICON), which keeps the composition self-consistent for any | ||
| initial transform type. | ||
| """ | ||
| output_dir = test_directories["output"] | ||
| reg_output_dir = output_dir / "registration_ANTS" | ||
| reg_output_dir.mkdir(exist_ok=True) | ||
|
|
||
| # Pick two phases that are far apart in the cycle so there is real motion. | ||
| fixed_image = test_images[0] | ||
| moving_image = test_images[min(10, len(test_images) - 1)] | ||
|
|
||
| fixed_arr = itk.array_from_image(fixed_image) | ||
| # Moving and fixed share the acquisition grid (split from one 4D image), | ||
| # so the moving array is directly comparable for the unregistered score. | ||
| moving_arr = itk.array_from_image(moving_image) | ||
| threshold = float(np.percentile(fixed_arr, 70.0)) | ||
| foreground = fixed_arr > threshold | ||
|
|
||
| transform_tools = TransformTools() | ||
|
|
||
| def warp_score(forward_transform: Any) -> float: | ||
| warped = transform_tools.transform_image( | ||
| moving_image, | ||
| forward_transform, | ||
| fixed_image, | ||
| interpolation_method="linear", | ||
| ) | ||
| return _foreground_ncc(fixed_arr, itk.array_from_image(warped), foreground) | ||
|
|
||
| ncc_unregistered = _foreground_ncc(fixed_arr, moving_arr, foreground) | ||
|
|
||
| # Baseline: no initial transform. | ||
| registrar_ANTS.set_modality("ct") | ||
| registrar_ANTS.set_fixed_image(fixed_image) | ||
| baseline = registrar_ANTS.register(moving_image=moving_image) | ||
| ncc_baseline = warp_score(baseline["forward_transform"]) | ||
|
|
||
| # Identity initial: the composition machinery must be a no-op. | ||
| identity = itk.AffineTransform[itk.D, 3].New() | ||
| identity.SetIdentity() | ||
| registrar_identity = RegisterImagesANTS() | ||
| registrar_identity.set_modality("ct") | ||
| registrar_identity.set_fixed_image(fixed_image) | ||
| identity_result = registrar_identity.register( | ||
| moving_image=moving_image, initial_forward_transform=identity | ||
| ) | ||
| ncc_identity = warp_score(identity_result["forward_transform"]) | ||
|
|
||
| # Prior deformable initial: the realistic time-series prior use case. | ||
| registrar_prior = RegisterImagesANTS() | ||
| registrar_prior.set_modality("ct") | ||
| registrar_prior.set_fixed_image(fixed_image) | ||
| prior_result = registrar_prior.register( | ||
| moving_image=moving_image, | ||
| initial_forward_transform=baseline["forward_transform"], | ||
| ) | ||
| ncc_prior = warp_score(prior_result["forward_transform"]) | ||
|
|
||
| print("\nANTS initial-transform composition metrics (foreground NCC):") | ||
| print(f" unregistered: {ncc_unregistered:.4f}") | ||
| print(f" baseline (no initial): {ncc_baseline:.4f}") | ||
| print(f" identity initial: {ncc_identity:.4f}") | ||
| print(f" prior-deformable init: {ncc_prior:.4f}") | ||
|
|
||
| warped_prior = transform_tools.transform_image( | ||
| moving_image, | ||
| prior_result["forward_transform"], | ||
| fixed_image, | ||
| interpolation_method="linear", | ||
| ) | ||
| itk.imwrite( | ||
| warped_prior, | ||
| str(reg_output_dir / "ants_warped_prior_initial.mha"), | ||
| compression=True, | ||
| ) | ||
|
|
||
| # Registration must improve alignment over the unregistered pair. | ||
| assert ncc_baseline > ncc_unregistered, ( | ||
| f"Baseline registration did not improve alignment: " | ||
| f"{ncc_baseline:.4f} <= {ncc_unregistered:.4f}" | ||
| ) | ||
| # Identity initial must reproduce the baseline (composition is a no-op). | ||
| assert abs(ncc_identity - ncc_baseline) < 0.03, ( | ||
| f"Identity initial transform changed the result: " | ||
| f"identity={ncc_identity:.4f} vs baseline={ncc_baseline:.4f}" | ||
| ) | ||
| # A prior-deformable initial must reach the no-initial baseline quality | ||
| # (the composition recovers the full transform). | ||
| assert ncc_prior >= ncc_baseline - 0.03, ( | ||
| f"Prior-initial composition did not reach baseline quality: " | ||
| f"{ncc_prior:.4f} < {ncc_baseline:.4f} - 0.03" | ||
| ) | ||
|
|
||
| def test_initial_transform_matrix_composition( | ||
| self, | ||
| registrar_ANTS: RegisterImagesANTS, | ||
| test_images: list[Any], | ||
| ) -> None: | ||
| """A matrix (translation/affine) initial composes without corruption. | ||
|
|
||
| Regression guard for the previously-broken matrix initial_transform | ||
| path: feeding a translation initial used to corrupt the composition | ||
| (foreground NCC far below the unregistered pair). With the moving image | ||
| pre-warped by the initial, the composed forward_transform must align the | ||
| moving image onto the fixed grid at least as well as the unregistered | ||
| pair. | ||
| """ | ||
| fixed_image = test_images[0] | ||
| moving_image = test_images[min(10, len(test_images) - 1)] | ||
|
|
||
| fixed_arr = itk.array_from_image(fixed_image) | ||
| threshold = float(np.percentile(fixed_arr, 70.0)) | ||
| foreground = fixed_arr > threshold | ||
| ncc_unregistered = _foreground_ncc( | ||
| fixed_arr, itk.array_from_image(moving_image), foreground | ||
| ) | ||
|
|
||
| translation = itk.TranslationTransform[itk.D, 3].New() | ||
| translation.SetOffset([-5.0, -5.0, -5.0]) | ||
|
|
||
| registrar_ANTS.set_modality("ct") | ||
| registrar_ANTS.set_fixed_image(fixed_image) | ||
| result = registrar_ANTS.register( | ||
| moving_image=moving_image, initial_forward_transform=translation | ||
| ) | ||
|
|
||
| transform_tools = TransformTools() | ||
| warped = transform_tools.transform_image( | ||
| moving_image, | ||
| result["forward_transform"], | ||
| fixed_image, | ||
| interpolation_method="linear", | ||
| ) | ||
| ncc = _foreground_ncc(fixed_arr, itk.array_from_image(warped), foreground) | ||
| print( | ||
| f"\nMatrix-initial composed NCC={ncc:.4f} (unregistered={ncc_unregistered:.4f})" | ||
| ) | ||
| assert ncc > ncc_unregistered, ( | ||
| f"Matrix-initial composition worse than unregistered: " | ||
| f"{ncc:.4f} <= {ncc_unregistered:.4f}" | ||
| ) | ||
|
|
||
| def test_affine_and_rigid_transform_types( | ||
| self, | ||
| registrar_ANTS: RegisterImagesANTS, | ||
| test_images: list[Any], | ||
| ) -> None: | ||
| """Affine and Rigid transform types run and improve alignment. | ||
|
|
||
| Regression guard for the ANTS preset names: ``set_transform_type`` | ||
| previously mapped Affine/Rigid to ``antsRegistration{Affine,Rigid}Quick`` | ||
| preset strings that do not exist in antspy, raising ValueError. Each | ||
| type must now run and warp the moving image onto the fixed grid at least | ||
| as well as the unregistered pair. | ||
| """ | ||
| fixed_image = test_images[0] | ||
| moving_image = test_images[min(10, len(test_images) - 1)] | ||
|
|
||
| fixed_arr = itk.array_from_image(fixed_image) | ||
| threshold = float(np.percentile(fixed_arr, 70.0)) | ||
| foreground = fixed_arr > threshold | ||
| ncc_unregistered = _foreground_ncc( | ||
| fixed_arr, itk.array_from_image(moving_image), foreground | ||
| ) | ||
|
|
||
| transform_tools = TransformTools() | ||
| for transform_type in ("Rigid", "Affine"): | ||
| registrar = RegisterImagesANTS() | ||
| registrar.set_modality("ct") | ||
| registrar.set_transform_type(transform_type) | ||
| registrar.set_fixed_image(fixed_image) | ||
| result = registrar.register(moving_image=moving_image) | ||
| warped = transform_tools.transform_image( | ||
| moving_image, | ||
| result["forward_transform"], | ||
| fixed_image, | ||
| interpolation_method="linear", | ||
| ) | ||
| ncc = _foreground_ncc(fixed_arr, itk.array_from_image(warped), foreground) | ||
| print( | ||
| f"\n{transform_type} transform NCC={ncc:.4f} " | ||
| f"(unregistered={ncc_unregistered:.4f})" | ||
| ) | ||
| assert ncc > ncc_unregistered, ( | ||
| f"{transform_type} registration did not improve alignment: " | ||
| f"{ncc:.4f} <= {ncc_unregistered:.4f}" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Please add inverse-composition coverage too.
These regressions only score forward_transform, but the implementation also changed how inverse_transform is built when initial_forward_transform is present. A broken fixed→moving path would still pass this suite, so I'd add one check that warps the fixed image with inverse_transform and compares it against the expected moving-grid result.
As per coding guidelines, "Update docstrings and tests for every changed public method".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_register_images_ants.py` around lines 333 - 543, Add coverage that
validates the inverse composition path by warping the fixed image using the
returned inverse transform and scoring against the original moving image grid:
after each registration call that currently uses result["forward_transform"]
(e.g., baseline, identity_result, prior_result, and the matrix/transform_type
loops), also retrieve result["inverse_transform"] and call
TransformTools().transform_image(fixed_image, inverse_transform, moving_image,
interpolation_method="linear") then compute _foreground_ncc between
itk.array_from_image(moving_image) and the warped-fixed array (using the same
foreground mask) and assert it meets the same improvement/equality thresholds as
the forward checks; ensure you reference result["inverse_transform"] and
TransformTools.transform_image when adding these assertions.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #63 +/- ##
==========================================
+ Coverage 32.58% 32.80% +0.22%
==========================================
Files 52 53 +1
Lines 7156 7181 +25
==========================================
+ Hits 2332 2356 +24
- Misses 4824 4825 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/physiomotion4d/workflow_fine_tune_icon_registration.py (1)
749-767:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid double-dilating the registration masks.
apply_registration()derives dilated binary masks here, butRegisterImagesBase.set_fixed_mask()andRegisterImagesBase.register()convert and dilate incoming masks again. That turns onemask_dilation_mminto two successive dilations and widens the ROI more than the caller requested.Proposed fix
- fixed_mask = ( - self.labelmap_tools.convert_labelmap_to_mask( - reference_segmentation, dilation_in_mm=self.mask_dilation_mm - ) - if reference_segmentation is not None - else None - ) - moving_masks: Optional[list[Optional[itk.Image]]] = None - if moving_segmentations is not None: - moving_masks = [ - ( - self.labelmap_tools.convert_labelmap_to_mask( - seg, dilation_in_mm=self.mask_dilation_mm - ) - if seg is not None - else None - ) - for seg in moving_segmentations - ] + # RegisterImagesBase converts aligned binary masks/labelmaps to the + # actual registration masks, including dilation. + fixed_mask = reference_segmentation + moving_masks = moving_segmentations🤖 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_fine_tune_icon_registration.py` around lines 749 - 767, The code currently pre-converts/dilates segmentations into masks using self.labelmap_tools.convert_labelmap_to_mask (producing fixed_mask and moving_masks), which causes RegisterImagesBase.set_fixed_mask() / RegisterImagesBase.register() to convert/dilate them again and double-dilate; to fix, stop pre-dilating here in apply_registration: instead of calling self.labelmap_tools.convert_labelmap_to_mask use the raw reference_segmentation and moving_segmentations (or None) so fixed_mask and moving_masks hold the original labelmaps, and let RegisterImagesBase.set_fixed_mask() / register() perform the single conversion/dilation using mask_dilation_mm; update the variables fixed_mask and moving_masks accordingly and remove the dilation_in_mm argument usage in this function.
♻️ Duplicate comments (1)
src/physiomotion4d/register_images_ants.py (1)
602-609:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPre-warp onto the registration grid and pre-warp the moving mask.
Two critical coordinate-system mismatches:
Grid mismatch: Line 608 resamples
moving_image_preontoself.fixed_image, but the subsequentants.registration()calls (lines 668, 684) useself.fixed_image_preasfixed. Ifpreprocess()changes spacing/origin/size, the pre-warped image and the registration optimize on different grids, causing alignment errors.Mask mismatch: When
initial_forward_transformis supplied,self.moving_image_preis pre-warped onto the fixed grid (line 605-608) butself.moving_mask(set at line 590) remains on the original moving grid. The masked registration (line 672) then passes an image and mask in different coordinate systems to ANTs, so the mask constrains the wrong ROI.🐛 Proposed fix
if initial_forward_transform is not None: self.log_info("Pre-warping moving image with initial transform...") transform_tools = TransformTools() self.moving_image_pre = transform_tools.transform_image( self.moving_image_pre, initial_forward_transform, - self.fixed_image, + self.fixed_image_pre, ) + if self.moving_mask is not None: + self.moving_mask = transform_tools.transform_image( + self.moving_mask, + initial_forward_transform, + self.fixed_image_pre, + 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/register_images_ants.py` around lines 602 - 609, The code pre-warps self.moving_image_pre with initial_forward_transform onto self.fixed_image but later registration uses self.fixed_image_pre and expects both image and mask on the same preprocessed grid; also self.moving_mask is left on the original moving grid, so ants.registration receives misaligned inputs. Fix by pre-warping both the moving image and the moving mask with initial_forward_transform onto the same grid used for registration (the preprocess() output referenced by self.fixed_image_pre), i.e., call TransformTools.transform_image/transform_mask (or the equivalent method) to resample self.moving_image_pre and self.moving_mask into the fixed_image_pre coordinate system whenever initial_forward_transform is provided, so that ants.registration(...) always receives image, mask and fixed that share the same spacing/origin/size.
🧹 Nitpick comments (2)
tests/test_labelmap_tools.py (2)
98-109: ⚡ Quick winActually verify direction metadata here.
This test currently proves spacing and origin preservation only. If
convert_labelmap_to_mask()dropped direction cosines, it would still pass. Setting a non-identity direction onlabelmapand asserting it onmaskwould cover the behavior the docstring promises.🤖 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_labelmap_tools.py` around lines 98 - 109, The test test_preserves_image_information currently only checks spacing and origin; update it to also set a non-identity direction on the input labelmap (use labelmap.SetDirection with a 3x3 direction cosine tuple) before calling labelmap_tools.convert_labelmap_to_mask and then assert that mask.GetDirection() matches the same direction values; reference the test function name, the labelmap.SetDirection call, and the labelmap_tools.convert_labelmap_to_mask / mask.GetDirection methods to locate where to add the additional setup and assertion.
28-28: ⚡ Quick winAdd shape and axis-order details to these test docstrings.
These tests build synthetic
itk.Imagevolumes from NumPy arrays, but the docstrings do not spell out the array shape or the NumPy-vs-ITK axis order. In this module that distinction is part of the behavior under test.As per coding guidelines "
tests/**/*.py: State image shape and axis order in every test docstring" and "**/*.py: State axis order and shape explicitly in every docstring and comment that touches arrays".Also applies to: 42-42, 58-58, 81-81, 99-99
🤖 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_labelmap_tools.py` at line 28, Update the test docstrings in test_labelmap_tools.py for the tests that construct synthetic itk.Image volumes so each docstring explicitly states the NumPy array shape and the axis-order mapping between NumPy and ITK (e.g., "NumPy array shape (Z, Y, X); ITK image axis order (X, Y, Z)"), applying the same change to the other similar tests in the file; locate the docstrings in the tests that build itk.Image from NumPy arrays and add a one-line note with the exact array shape and axis order there.
🤖 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.
Outside diff comments:
In `@src/physiomotion4d/workflow_fine_tune_icon_registration.py`:
- Around line 749-767: The code currently pre-converts/dilates segmentations
into masks using self.labelmap_tools.convert_labelmap_to_mask (producing
fixed_mask and moving_masks), which causes RegisterImagesBase.set_fixed_mask() /
RegisterImagesBase.register() to convert/dilate them again and double-dilate; to
fix, stop pre-dilating here in apply_registration: instead of calling
self.labelmap_tools.convert_labelmap_to_mask use the raw reference_segmentation
and moving_segmentations (or None) so fixed_mask and moving_masks hold the
original labelmaps, and let RegisterImagesBase.set_fixed_mask() / register()
perform the single conversion/dilation using mask_dilation_mm; update the
variables fixed_mask and moving_masks accordingly and remove the dilation_in_mm
argument usage in this function.
---
Duplicate comments:
In `@src/physiomotion4d/register_images_ants.py`:
- Around line 602-609: The code pre-warps self.moving_image_pre with
initial_forward_transform onto self.fixed_image but later registration uses
self.fixed_image_pre and expects both image and mask on the same preprocessed
grid; also self.moving_mask is left on the original moving grid, so
ants.registration receives misaligned inputs. Fix by pre-warping both the moving
image and the moving mask with initial_forward_transform onto the same grid used
for registration (the preprocess() output referenced by self.fixed_image_pre),
i.e., call TransformTools.transform_image/transform_mask (or the equivalent
method) to resample self.moving_image_pre and self.moving_mask into the
fixed_image_pre coordinate system whenever initial_forward_transform is
provided, so that ants.registration(...) always receives image, mask and fixed
that share the same spacing/origin/size.
---
Nitpick comments:
In `@tests/test_labelmap_tools.py`:
- Around line 98-109: The test test_preserves_image_information currently only
checks spacing and origin; update it to also set a non-identity direction on the
input labelmap (use labelmap.SetDirection with a 3x3 direction cosine tuple)
before calling labelmap_tools.convert_labelmap_to_mask and then assert that
mask.GetDirection() matches the same direction values; reference the test
function name, the labelmap.SetDirection call, and the
labelmap_tools.convert_labelmap_to_mask / mask.GetDirection methods to locate
where to add the additional setup and assertion.
- Line 28: Update the test docstrings in test_labelmap_tools.py for the tests
that construct synthetic itk.Image volumes so each docstring explicitly states
the NumPy array shape and the axis-order mapping between NumPy and ITK (e.g.,
"NumPy array shape (Z, Y, X); ITK image axis order (X, Y, Z)"), applying the
same change to the other similar tests in the file; locate the docstrings in the
tests that build itk.Image from NumPy arrays and add a one-line note with the
exact array shape and axis order there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11651f73-7a7e-439e-b8c0-535dfccca177
📒 Files selected for processing (21)
docs/api/utilities/index.rstdocs/api/utilities/labelmap_tools.rstexperiments/LongitudinalRegistration/1-finetune_icon.pyexperiments/LongitudinalRegistration/1-preregistration.pyexperiments/LongitudinalRegistration/2-recon_4d_icon_eval.pypyproject.tomlsrc/physiomotion4d/__init__.pysrc/physiomotion4d/contour_tools.pysrc/physiomotion4d/image_tools.pysrc/physiomotion4d/labelmap_tools.pysrc/physiomotion4d/register_images_ants.pysrc/physiomotion4d/register_images_base.pysrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/register_models_distance_maps.pysrc/physiomotion4d/segment_anatomy_base.pysrc/physiomotion4d/segment_heart_simpleware.pysrc/physiomotion4d/workflow_fine_tune_icon_registration.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.pytests/test_image_tools.pytests/test_labelmap_tools.pytests/test_workflow_fine_tune_icon_registration.py
💤 Files with no reviewable changes (2)
- src/physiomotion4d/register_images_icon.py
- src/physiomotion4d/segment_anatomy_base.py
✅ Files skipped from review due to trivial changes (4)
- src/physiomotion4d/init.py
- tests/test_image_tools.py
- docs/api/utilities/index.rst
- docs/api/utilities/labelmap_tools.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- experiments/LongitudinalRegistration/1-preregistration.py
- experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/physiomotion4d/register_images_ants.py:565
- The docstring still says initial transforms are "converted from ITK to ANTs format automatically", but the implementation now applies the initial transform by pre-warping the moving image and passes
initial_transform=["identity"]toants.registration. This line should be updated to avoid misleading API consumers.
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
Comment on lines
320
to
323
| @staticmethod | ||
| def _posix(path: Union[str, Path]) -> str: | ||
| """Return a forward-slashed string path (uniGradICON expects POSIX paths).""" | ||
| return str(path).replace("\\", "/") |
Comment on lines
+370
to
+372
| def prepare_dataset( | ||
| self, use_segmentations: bool = True, use_masks: bool = True | ||
| ) -> Path: |
Comment on lines
+395
to
+396
| self._use_segmentations = use_segmentations | ||
| self._use_masks = use_masks |
Comment on lines
+526
to
528
| "loss_function_masking": self._use_masks, | ||
| "use_label": False, | ||
| "roi_masking": False, |
Comment on lines
731
to
744
| @@ -745,8 +739,8 @@ def apply_registration( | |||
| if moving_segmentations is not None: | |||
| moving_masks = [ | |||
| ( | |||
| RegisterImagesICON.create_mask( | |||
| seg, dilation_mm=self.mask_dilation_mm | |||
| self.labelmap_tools.convert_labelmap_to_mask( | |||
| seg, dilation_in_mm=self.mask_dilation_mm | |||
| ) | |||
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/physiomotion4d/workflow_fine_tune_icon_registration.py (1)
486-527:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
prepare_config()has no initialized mask mode.After this refactor,
training.loss_function_maskingcomes fromself._use_masks, butprepare_config(dataset_json_path=...)can still be called beforeprepare_dataset(). In that case the YAML getsloss_function_masking: null, which is an invalid config shape for a boolean flag.Proposed fix
if dataset_json_path is None: raise ValueError( "dataset_json_path not provided and prepare_dataset() has not " "been called yet" ) + if self._use_masks is None: + raise ValueError( + "prepare_config() requires prepare_dataset() so mask settings " + "are initialized" + ) experiment_name = self.experiment_dir / f"{self.fine_tune_name}_model"🤖 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_fine_tune_icon_registration.py` around lines 486 - 527, prepare_config currently writes training.loss_function_masking from self._use_masks which may be None if prepare_dataset hasn't been run; detect this early by checking self._use_masks in prepare_config and raise a ValueError (or similar) if it is None with a clear message that prepare_dataset must be called (or mask mode set) before calling prepare_config; update any tests/call sites accordingly so training.loss_function_masking in the emitted YAML is always a boolean (True/False) and never null.experiments/LongitudinalRegistration/1-initial_registration.py (3)
289-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment: three backends now run, not two.
The comment says "register each frame to the reference under both backends" and "the ANTS-vs-Greedy comparison", but the loop iterates
methods = ["ANTS", "Greedy", "ICON"].Proposed wording fix
-# its labelmap, mask, and landmarks; then register each frame to the -# reference under both backends. Each frame starts from identity so the -# ANTS-vs-Greedy comparison is independent across frames. +# its labelmap, mask, and landmarks; then register each frame to the +# reference under all three backends. Each frame starts from identity so +# the per-backend comparison is independent across frames.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/LongitudinalRegistration/1-initial_registration.py` around lines 289 - 290, The comment above the methods loop is stale — it mentions "both backends" and "ANTS-vs-Greedy" but the code uses methods = ["ANTS", "Greedy", "ICON"]; update that comment (the block preceding the loop that references "reference under both backends" and "ANTS-vs-Greedy comparison") to mention three backends and a three-way comparison (or explicitly list ANTS, Greedy, and ICON) and adjust wording so it no longer implies only two methods.
263-280:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix docstring mismatch:
load_or_derive_maskalways recomputes instead of cachingIn
experiments/LongitudinalRegistration/1-initial_registration.py(load_or_derive_mask, lines 263-280), the cache read is commented out, so the function always re-derives and rewrites"<stem>_labelmap_mask.nii.gz"—contradicting the docstring’s “cached … or derive it” promise. Either restore themask_path.exists()branch or update the docstring to say it always recomputes.Also, the docstring doesn’t mention that labels
[1, 2, 3, 4]are excluded viaexclude_labels.Proposed fix (restore caching)
- # Force mask update - # if mask_path.exists(): - # return itk.imread(str(mask_path)) + if mask_path.exists(): + return itk.imread(str(mask_path)) mask = labelmap_tools.convert_labelmap_to_mask( labelmap, dilation_in_mm=mask_dilation_mm, exclude_labels=[1, 2, 3, 4], # These are labels for the interior of the heart chambers (the LV, RV, LA, RA) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/LongitudinalRegistration/1-initial_registration.py` around lines 263 - 280, The function load_or_derive_mask currently always recomputes and overwrites the mask because the mask_path.exists() branch is commented out; restore the caching behavior by re-enabling the existence check so the function returns itk.imread(str(mask_path)) when the cached mask exists, otherwise derive, write, and return the mask as before; also update the docstring to mention the exclude_labels argument (labels [1,2,3,4] are excluded) so the docstring accurately describes both caching behavior and the excluded labels.
411-414:⚠️ Potential issue | 🟠 MajorPossible double dilation of fixed/moving masks.
experiments/LongitudinalRegistration/1-initial_registration.py’sload_or_derive_mask()already derivesfixed_mask/moving_masksusingLabelmapTools.convert_labelmap_to_mask(..., dilation_in_mm=mask_dilation_mm)(3 mm). After that,reg.set_mask_dilation(mask_dilation_mm)plusreg.set_fixed_mask(fixed_mask)dilates again becauseRegisterImagesBase.set_fixed_mask()also callsconvert_labelmap_to_mask(..., dilation_in_mm=self.mask_dilation_mm)(andregister()applies the same conversion to the providedmoving_mask). This can effectively turn the ROI into ~6 mm and mismatch the masks written to disk for ICON eval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/LongitudinalRegistration/1-initial_registration.py` around lines 411 - 414, The masks are being dilated twice: load_or_derive_mask() calls LabelmapTools.convert_labelmap_to_mask(..., dilation_in_mm=mask_dilation_mm) and then reg.set_mask_dilation(mask_dilation_mm) + reg.set_fixed_mask(fixed_mask) triggers another dilation inside RegisterImagesBase.set_fixed_mask()/register(); fix by ensuring dilation happens only once — either stop pre-dilating in load_or_derive_mask (call convert_labelmap_to_mask with dilation_in_mm=0 and let reg.set_mask_dilation(mask_dilation_mm) handle it) or, if you keep pre-dilated masks, set reg.set_mask_dilation(0) before reg.set_fixed_mask(fixed_mask)/reg.set_moving_mask(...); update the code to use one of these approaches and keep references to load_or_derive_mask, LabelmapTools.convert_labelmap_to_mask, reg.set_mask_dilation, reg.set_fixed_mask, and register() when making the change.
🧹 Nitpick comments (1)
src/physiomotion4d/workflow_fine_tune_icon_registration.py (1)
160-160: ⚡ Quick winDocument
mask_exclude_labelsin__init__'s Args block.The new public parameter is in the signature, but the Args section never describes it, so the docstring no longer matches the API.
Proposed fix
mask_dilation_mm: Physical radius (millimeters) of binary dilation applied to the >0 labelmap when deriving the loss-masking binary mask via :meth:`LabelmapTools.convert_labelmap_to_mask`. Ignored when no segmentations are supplied. Default 5.0 mm. + mask_exclude_labels: Optional label values to exclude when + deriving binary masks from segmentation labelmaps. Default + ``None``. mask_dir: Directory where derived binary masks are written and looked up. ``None`` (default) writes each derived mask nextAs per coding guidelines, "Update docstrings for every changed public method; keep claims factual."
🤖 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_fine_tune_icon_registration.py` at line 160, The __init__ signature for the class in workflow_fine_tune_icon_registration.py added the public parameter mask_exclude_labels: Optional[list[int]] = None but the Args section of the __init__ docstring was not updated; update the __init__ docstring to include a factual entry for mask_exclude_labels (type Optional[list[int]], default None) describing its purpose (which labels to exclude from the mask) and expected format, matching the signature and keeping wording concise and objective.
🤖 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/physiomotion4d/workflow_fine_tune_icon_registration.py`:
- Around line 362-365: The in-memory ROI masks built in apply_registration() are
not honoring self.mask_exclude_labels, causing a mismatch with _derive_mask();
update the calls that build masks in apply_registration() (the call to
labelmap_tools.convert_labelmap_to_mask and any other mask construction between
the block around the shown lines and the later block at ~731-749) to pass
exclude_labels=self.mask_exclude_labels and use the same dilation parameter
(self.mask_dilation_mm) so apply-time masks match _derive_mask(); search for
convert_labelmap_to_mask and any manual mask creation in apply_registration()
and add the exclude_labels parameter where missing.
- Around line 370-372: The prepare_dataset logic currently makes
segmentations/masks effectively mandatory by overwriting seg_list to None when
subject_segmentation_files is None while use_segmentations/use_masks default to
True; update prepare_dataset (and the similar block referenced around lines
395-467) so that: if use_segmentations is False, do not expect or require
subject_segmentation_files and do not touch seg_list; if use_masks is True only
attempt to derive masks from segmentations when seg_list is present (i.e.,
subject_segmentation_files provided) and otherwise fall back to
subject_mask_files if available; avoid setting seg_list=None just because mask
derivation is requested—only clear or use seg_list when the user explicitly
requested segmentations via use_segmentations and none were provided; adjust
handling of subject_segmentation_files, subject_mask_files, use_segmentations,
use_masks, seg_list, and mask_list accordingly so frames are not skipped
unnecessarily.
---
Outside diff comments:
In `@experiments/LongitudinalRegistration/1-initial_registration.py`:
- Around line 289-290: The comment above the methods loop is stale — it mentions
"both backends" and "ANTS-vs-Greedy" but the code uses methods = ["ANTS",
"Greedy", "ICON"]; update that comment (the block preceding the loop that
references "reference under both backends" and "ANTS-vs-Greedy comparison") to
mention three backends and a three-way comparison (or explicitly list ANTS,
Greedy, and ICON) and adjust wording so it no longer implies only two methods.
- Around line 263-280: The function load_or_derive_mask currently always
recomputes and overwrites the mask because the mask_path.exists() branch is
commented out; restore the caching behavior by re-enabling the existence check
so the function returns itk.imread(str(mask_path)) when the cached mask exists,
otherwise derive, write, and return the mask as before; also update the
docstring to mention the exclude_labels argument (labels [1,2,3,4] are excluded)
so the docstring accurately describes both caching behavior and the excluded
labels.
- Around line 411-414: The masks are being dilated twice: load_or_derive_mask()
calls LabelmapTools.convert_labelmap_to_mask(...,
dilation_in_mm=mask_dilation_mm) and then
reg.set_mask_dilation(mask_dilation_mm) + reg.set_fixed_mask(fixed_mask)
triggers another dilation inside RegisterImagesBase.set_fixed_mask()/register();
fix by ensuring dilation happens only once — either stop pre-dilating in
load_or_derive_mask (call convert_labelmap_to_mask with dilation_in_mm=0 and let
reg.set_mask_dilation(mask_dilation_mm) handle it) or, if you keep pre-dilated
masks, set reg.set_mask_dilation(0) before
reg.set_fixed_mask(fixed_mask)/reg.set_moving_mask(...); update the code to use
one of these approaches and keep references to load_or_derive_mask,
LabelmapTools.convert_labelmap_to_mask, reg.set_mask_dilation,
reg.set_fixed_mask, and register() when making the change.
In `@src/physiomotion4d/workflow_fine_tune_icon_registration.py`:
- Around line 486-527: prepare_config currently writes
training.loss_function_masking from self._use_masks which may be None if
prepare_dataset hasn't been run; detect this early by checking self._use_masks
in prepare_config and raise a ValueError (or similar) if it is None with a clear
message that prepare_dataset must be called (or mask mode set) before calling
prepare_config; update any tests/call sites accordingly so
training.loss_function_masking in the emitted YAML is always a boolean
(True/False) and never null.
---
Nitpick comments:
In `@src/physiomotion4d/workflow_fine_tune_icon_registration.py`:
- Line 160: The __init__ signature for the class in
workflow_fine_tune_icon_registration.py added the public parameter
mask_exclude_labels: Optional[list[int]] = None but the Args section of the
__init__ docstring was not updated; update the __init__ docstring to include a
factual entry for mask_exclude_labels (type Optional[list[int]], default None)
describing its purpose (which labels to exclude from the mask) and expected
format, matching the signature and keeping wording concise and objective.
🪄 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: 6bd8701b-995f-41fe-b042-9ebfb2c4d6a3
📒 Files selected for processing (8)
experiments/LongitudinalRegistration/1-initial_registration.pyexperiments/LongitudinalRegistration/2-finetune_icon.pyexperiments/LongitudinalRegistration/3-recon_4d_icon_eval.pyexperiments/LongitudinalRegistration/4-recon_4d_all_eval.pyexperiments/LongitudinalRegistration/registration_test.pysrc/physiomotion4d/labelmap_tools.pysrc/physiomotion4d/workflow_fine_tune_icon_registration.pytests/test_labelmap_tools.py
💤 Files with no reviewable changes (3)
- experiments/LongitudinalRegistration/4-recon_4d_all_eval.py
- experiments/LongitudinalRegistration/2-finetune_icon.py
- experiments/LongitudinalRegistration/3-recon_4d_icon_eval.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_labelmap_tools.py
- src/physiomotion4d/labelmap_tools.py
Comment on lines
+362
to
+365
| mask = self.labelmap_tools.convert_labelmap_to_mask( | ||
| labelmap, | ||
| dilation_in_mm=self.mask_dilation_mm, | ||
| exclude_labels=self.mask_exclude_labels, |
There was a problem hiding this comment.
Honor mask_exclude_labels when building apply-time ROI masks.
_derive_mask() already excludes self.mask_exclude_labels, but the in-memory masks in apply_registration() do not. With the same workflow configuration, fine-tuning and apply-time registration can therefore use different ROIs.
Proposed fix
fixed_mask = (
self.labelmap_tools.convert_labelmap_to_mask(
- reference_segmentation, dilation_in_mm=self.mask_dilation_mm
+ reference_segmentation,
+ dilation_in_mm=self.mask_dilation_mm,
+ exclude_labels=self.mask_exclude_labels,
)
if reference_segmentation is not None
else None
)
...
self.labelmap_tools.convert_labelmap_to_mask(
- seg, dilation_in_mm=self.mask_dilation_mm
+ seg,
+ dilation_in_mm=self.mask_dilation_mm,
+ exclude_labels=self.mask_exclude_labels,
)Also applies to: 731-749
🤖 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_fine_tune_icon_registration.py` around lines 362
- 365, The in-memory ROI masks built in apply_registration() are not honoring
self.mask_exclude_labels, causing a mismatch with _derive_mask(); update the
calls that build masks in apply_registration() (the call to
labelmap_tools.convert_labelmap_to_mask and any other mask construction between
the block around the shown lines and the later block at ~731-749) to pass
exclude_labels=self.mask_exclude_labels and use the same dilation parameter
(self.mask_dilation_mm) so apply-time masks match _derive_mask(); search for
convert_labelmap_to_mask and any manual mask creation in apply_registration()
and add the exclude_labels parameter where missing.
Comment on lines
+370
to
+372
| def prepare_dataset( | ||
| self, use_segmentations: bool = True, use_masks: bool = True | ||
| ) -> Path: |
There was a problem hiding this comment.
Don't let the new flags make optional companions mandatory.
Line 182 says subject_segmentation_files=None disables paired-with-seg training, but run_fine_tuning() still reaches this code with use_segmentations=True and use_masks=True by default. That now skips every frame unless segmentations and derivable masks exist, and use_segmentations=False, use_masks=True also breaks because seg_list is overwritten to None before mask derivation.
Proposed fix
- def prepare_dataset(
- self, use_segmentations: bool = True, use_masks: bool = True
- ) -> Path:
+ def prepare_dataset(
+ self,
+ use_segmentations: Optional[bool] = None,
+ use_masks: Optional[bool] = None,
+ ) -> Path:
...
- self._use_segmentations = use_segmentations
- self._use_masks = use_masks
+ use_segmentations = (
+ self.subject_segmentation_files is not None
+ if use_segmentations is None
+ else use_segmentations
+ )
+ use_masks = (
+ self.subject_mask_files is not None
+ or self.subject_segmentation_files is not None
+ if use_masks is None
+ else use_masks
+ )
+
+ self._use_segmentations = use_segmentations
+ self._use_masks = use_masks
...
- if not use_segmentations:
- seg_list = [None] * len(image_files)
- else:
- seg_list = (
- self.subject_segmentation_files[subject_index]
- if self.subject_segmentation_files is not None
- else [None] * len(image_files)
- )
+ seg_list = (
+ self.subject_segmentation_files[subject_index]
+ if self.subject_segmentation_files is not None
+ else [None] * len(image_files)
+ )Also applies to: 395-467
🤖 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_fine_tune_icon_registration.py` around lines 370
- 372, The prepare_dataset logic currently makes segmentations/masks effectively
mandatory by overwriting seg_list to None when subject_segmentation_files is
None while use_segmentations/use_masks default to True; update prepare_dataset
(and the similar block referenced around lines 395-467) so that: if
use_segmentations is False, do not expect or require subject_segmentation_files
and do not touch seg_list; if use_masks is True only attempt to derive masks
from segmentations when seg_list is present (i.e., subject_segmentation_files
provided) and otherwise fall back to subject_mask_files if available; avoid
setting seg_list=None just because mask derivation is requested—only clear or
use seg_list when the user explicitly requested segmentations via
use_segmentations and none were provided; adjust handling of
subject_segmentation_files, subject_mask_files, use_segmentations, use_masks,
seg_list, and mask_list accordingly so frames are not skipped unnecessarily.
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.
Transform conventions
Longitudinal registration experiment
Code style
Summary by CodeRabbit
New Features
LabelmapToolsutility to public API for converting segmentation labelmaps to binary masks with optional label exclusion and dilation.set_fixed_labelmap()method in time-series registration workflow.Documentation
Bug Fixes
Chores