ENH: Experiment for Fine Tuning ICON#65
Merged
Merged
Conversation
…or time-series Remove RegisterImagesANTS from RegisterTimeSeriesImages and all dependent workflows/CLIs, replacing every ANTS_ICON and greedy_ICON path with the unified Greedy_ICON pipeline. Normalize method-name casing throughout (greedy → Greedy, greedy_ICON → Greedy_ICON). In the longitudinal registration experiment scripts: - 2-finetune_icon.py: support multiple pre-registration directories, require masks already on disk (drop load_or_derive_mask), rename "warped" to "init" throughout. - 3-recon_4d_icon_eval.py → 3-eval_icon.py: add warped-reference re-segmentation via SegmentHeartSimpleware and a second landmark-error table comparing warped-reference landmarks against time-point landmarks. - Add composite_time_series_mid_slice.py: stacks middle Z slices from a directory of MHA images into a composite volume for visual QA, with adjacent-slice RMSE reporting.
|
Warning Review limit reached
More reviews will be available in 28 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR replaces ANTs with Greedy across registration plumbing and workflows, renames segmentation APIs to labelmaps with explicit optional masks in the ICON fine-tuning workflow, expands longitudinal experiment conditions and adds an ICON evaluation script with warped-reference metrics, and introduces a composite mid-slice utility. ChangesANTs to Greedy Registration Backend Migration
ICON Fine-Tuning Workflow API and Implementation
Experiments and Evaluation
ICON types and tests
🎯 4 (Complex) | ⏱️ ~50 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
This PR updates the ICON fine-tuning and evaluation experiment pipeline and shifts several workflows/tutorials from ANTs(+ICON) terminology and defaults to Greedy(+ICON), with additional support for labelmaps/masks in the fine-tuning workflow.
Changes:
- Switch high-res 4D CT reconstruction (tutorial/CLI/workflow) defaults and knobs from ANTs(+ICON) to Greedy(+ICON).
- Update ICON fine-tuning workflow API and config/dataset generation to use “labelmaps” (and optionally masks) more explicitly.
- Refresh longitudinal experiment scripts (initial registration configs, fine-tune dataset augmentation, evaluation outputs, and add a mid-slice composite utility).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_06_reconstruct_highres_4d_ct.py | Updates tutorial defaults to Greedy+ICON and renames iteration settings accordingly. |
| src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py | Renames ANTs iteration plumbing to Greedy and updates docs/defaults. |
| src/physiomotion4d/workflow_fit_statistical_model_to_patient.py | Swaps ANTs registrar usage for Greedy in mask-to-image refinement path and updates docs. |
| src/physiomotion4d/workflow_fine_tune_icon_registration.py | Renames segmentation terminology to labelmaps, adjusts defaults, expands apply API with masks, and updates dataset/config generation. |
| src/physiomotion4d/workflow_convert_image_to_usd.py | Replaces ANTs registration backend option with Greedy. |
| src/physiomotion4d/register_time_series_images.py | Removes ANTs and legacy lowercase method-string support in favor of Greedy/ICON/Greedy_ICON. |
| src/physiomotion4d/cli/reconstruct_highres_4d_ct.py | Updates CLI to Greedy options (but currently has argparse destination/attribute mismatches). |
| experiments/LongitudinalRegistration/composite_time_series_mid_slice.py | Adds a utility script to build a middle-slice composite volume and report adjacent-slice RMSE. |
| experiments/LongitudinalRegistration/3-eval_icon.py | Updates eval outputs/paths and adds warped-reference re-segmentation/landmark comparison. |
| experiments/LongitudinalRegistration/2-finetune_icon.py | Changes dataset augmentation to pull “init” frames from external registration dirs and updates mask handling. |
| experiments/LongitudinalRegistration/1-initial_registration.py | Expands parameter sweep lists for initial Greedy registrations. |
Comments suppressed due to low confidence (4)
src/physiomotion4d/cli/reconstruct_highres_4d_ct.py:118
- argparse option names with capital letters produce attributes like args.Greedy_iterations/args.ICON_iterations. The code later reads args.icon_iterations (lowercase), which will raise AttributeError at runtime. Set explicit dest names (lowercase) for both options and use those attributes consistently.
parser.add_argument(
"--Greedy-iterations",
nargs="+",
type=int,
help="Greedy multi-resolution iterations (e.g., 30 15 7 3). Default: [30, 15, 7, 3]",
)
parser.add_argument(
"--ICON-iterations",
type=int,
help="ICON fine-tuning iterations. Default: 20",
)
src/physiomotion4d/cli/reconstruct_highres_4d_ct.py:302
- This block reads args.Greedy_iterations and args.icon_iterations, but with the current argparse definitions only args.Greedy_iterations / args.ICON_iterations would exist. After adding explicit dest= values for both args, update this block to use the new lowercase attribute names.
# Set number of iterations based on registration method and CLI arguments
if args.Greedy_iterations:
workflow.set_number_of_iterations_Greedy(args.Greedy_iterations)
else:
workflow.set_number_of_iterations_Greedy([30, 15, 7, 3])
if args.icon_iterations:
workflow.set_number_of_iterations_ICON(args.icon_iterations)
else:
workflow.set_number_of_iterations_ICON(20)
src/physiomotion4d/workflow_fine_tune_icon_registration.py:460
- The warning message still refers to "segmentation" even though this workflow now uses the term "labelmap" throughout (and the parameter is seg_file from subject_labelmap_files). This makes logs harder to interpret when debugging missing companions.
if use_labelmaps:
if seg_file is None or not Path(seg_file).exists():
self.log_warning(
"Skipping %s: segmentation missing for paired-with-seg "
"training (seg=%s)",
image_path,
seg_file,
)
experiments/LongitudinalRegistration/3-eval_icon.py:273
- itk.imwrite is passed a pathlib.Path here, while most other calls in this script convert paths to str(). ITK's Python wrappers don't consistently accept Path objects across versions; convert to str() to avoid TypeError.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
536
to
540
| "dice_loss_weight": self.dice_loss_weight, | ||
| "lncc_sigma": self.lncc_sigma, | ||
| "loss_function_masking": self._use_masks, | ||
| "use_label": self._use_segmentations, | ||
| "use_label": False, | ||
| "roi_masking": False, |
Comment on lines
+123
to
137
| def gather_init_frames( | ||
| initial_registration_dir: Path, | ||
| ) -> tuple[list[Path], list[Optional[Path]], list[Optional[Path]]]: | ||
| """Return ``(warped_image_paths, warped_labelmap_paths, warped_mask_paths)`` for one | ||
| ``initial_registration_dir / <method> / <patient_id>`` directory. | ||
| """Return ``(init_image_paths, init_labelmap_paths, init_mask_paths)`` for one | ||
| ``initial_registration_dir / <patient_id>`` directory. | ||
|
|
||
| Enumerates the warped moving images (``<stem>.mha``), excluding the | ||
| Enumerates the init moving images (``<stem>.mha``), excluding the | ||
| ``_labelmap.mha`` and ``_mask.mha`` | ||
| companions, and pairs each with its ``<stem>_labelmap.mha`` (``None`` when | ||
| that labelmap is absent). Returns empty lists when ``method_dir`` does | ||
| not exist. | ||
| that labelmap is absent). Returns empty lists when | ||
| ``initial_registration_dir`` does not exist. | ||
| """ | ||
| if not method_dir.is_dir(): | ||
| if not initial_registration_dir.is_dir(): | ||
| print(f" {patient_id}: registration dir {initial_registration_dir} not found") | ||
| return [], [], [] |
Comment on lines
+203
to
+205
| init_images, init_labelmaps, init_masks = gather_init_frames( | ||
| initial_registration_patient_dir | ||
| ) |
Comment on lines
658
to
662
| - Warps the moving image, segmentation, and landmarks into reference | ||
| space using ``forward_transform``. Segmentations use nearest-neighbor | ||
| space using ``forward_transform``. Labelmaps use nearest-neighbor | ||
| interpolation. Landmarks use ``inverse_transform.TransformPoint`` | ||
| (resampler-convention transform: maps moving-grid points back to | ||
| reference-grid points). |
Comment on lines
663
to
665
| - Warps the reference image, segmentation, and landmarks into each | ||
| moving-image space using ``inverse_transform`` for image/segmentation | ||
| moving-image space using ``inverse_transform`` for image/labelmap | ||
| resampling and ``forward_transform.TransformPoint`` for landmarks. |
Comment on lines
+718
to
+721
| - ``reference_to_moving_labelmaps`` (``list[Optional[itk.Image]]``): | ||
| the reference segmentation resampled onto each moving grid | ||
| with nearest-neighbor interpolation. ``None`` for every | ||
| entry when ``reference_segmentation`` was ``None``. | ||
| entry when ``reference_labelmap`` was ``None``. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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-finetune_icon.py (1)
123-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
patient_idis captured from outer scope instead of passed as a parameter.
gather_init_framesreferencespatient_id(lines 136, 153, 160) but doesn't receive it as a parameter. It relies on closure over the outer loop variable defined later at line 199. While this works in the current call site, it's fragile and confusing—calling this function from a different context would raiseNameError.Suggested fix: pass patient_id as a parameter
def gather_init_frames( initial_registration_dir: Path, + patient_id: str, ) -> tuple[list[Path], list[Optional[Path]], list[Optional[Path]]]:And update the call site at line 203-205:
init_images, init_labelmaps, init_masks = gather_init_frames( - initial_registration_patient_dir + initial_registration_patient_dir, patient_id )🤖 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-finetune_icon.py` around lines 123 - 162, The function gather_init_frames captures patient_id from an outer scope which is fragile; change the signature to accept patient_id (e.g., def gather_init_frames(initial_registration_dir: Path, patient_id: str) -> tuple[...]) and update all references inside gather_init_frames (the print messages) to use the parameter instead of a closure; then update the call site(s) that invoke gather_init_frames to pass the appropriate patient_id value (ensure the return type and behavior remain unchanged).src/physiomotion4d/cli/reconstruct_highres_4d_ct.py (1)
299-302:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix attribute name to match CLI argument.
The CLI argument
--ICON-iterations(line 115) is converted by argparse toargs.ICON_iterations(uppercase with underscore), but line 299 referencesargs.icon_iterations(lowercase). This will cause anAttributeErrorat runtime.🐛 Proposed fix
- if args.icon_iterations: - workflow.set_number_of_iterations_ICON(args.icon_iterations) + if args.ICON_iterations: + workflow.set_number_of_iterations_ICON(args.ICON_iterations) else: workflow.set_number_of_iterations_ICON(20)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiomotion4d/cli/reconstruct_highres_4d_ct.py` around lines 299 - 302, The code references args.icon_iterations but argparse stores the flag as args.ICON_iterations; update the conditional to use args.ICON_iterations (and keep the same fallback of 20) so the call to workflow.set_number_of_iterations_ICON uses the correct attribute name; look for the CLI parsing that defines --ICON-iterations and the call site workflow.set_number_of_iterations_ICON to make this change.
🧹 Nitpick comments (2)
src/physiomotion4d/workflow_fine_tune_icon_registration.py (1)
789-797: 💤 Low valueAddress or track the TODO comment.
The TODO indicates that reference frame and register_reference handling may need adjustment. The current code passes
reference_frame=0andregister_reference=Truewhich may not align with the caller's expectations when the reference image is explicitly provided.Would you like me to open an issue to track this TODO, or should it be addressed in this PR?
🤖 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 789 - 797, The TODO about setting the reference frame means we must not hardcode reference_frame=0 and register_reference=True when a reference image may be provided by the caller; update the call to registrar.register_time_series to compute the correct reference_frame (e.g., index of the explicitly provided reference in moving_images or a provided reference_index attribute) and set register_reference=False when the reference image is already supplied, or make both reference_frame and register_reference configurable from the workflow API; change the invocation around moving_images/moving_masks/moving_labelmaps to use the computed reference_frame and register_reference values so the registrar behavior matches the caller's expectations.experiments/LongitudinalRegistration/composite_time_series_mid_slice.py (1)
66-76: ⚡ Quick winConsider validating spacing and orientation consistency across input images.
The function validates that middle slice shapes match but does not check whether all input images have consistent spacing, origin, or direction. If the input images have different metadata, the composite will use only the first image's spacing, which may not accurately represent the full dataset.
Adding validation to ensure consistent metadata would improve correctness, especially if this utility is later used with heterogeneous inputs.
🛡️ Proposed metadata validation
expected_shape: Optional[tuple[int, ...]] = None + expected_spacing: Optional[tuple[float, ...]] = None for image_path in image_files: middle_slice, image = extract_middle_slice(image_path) if first_image is None: first_image = image expected_shape = middle_slice.shape + expected_spacing = image.GetSpacing() elif middle_slice.shape != expected_shape: raise ValueError( f"Middle slice shape mismatch for {image_path}: " f"expected {expected_shape}, got {middle_slice.shape}" ) + elif image.GetSpacing() != expected_spacing: + raise ValueError( + f"Spacing mismatch for {image_path}: " + f"expected {expected_spacing}, got {image.GetSpacing()}" + ) middle_slices.append(middle_slice)🤖 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/composite_time_series_mid_slice.py` around lines 66 - 76, Current loop only checks middle slice shapes; update it to also validate image metadata (spacing, origin, direction) for each image to ensure consistency—either extend extract_middle_slice to return metadata (spacing, origin, direction) or call a helper like load_image_metadata(image_path) inside the loop, capture the first image's metadata (e.g., first_metadata) and compare subsequent images' metadata to it, and raise a ValueError with a clear message if any spacing/origin/direction mismatch is found; keep using first_image/expected_shape and append middle_slices only after metadata matches.
🤖 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/2-finetune_icon.py`:
- Around line 187-188: The mask Path is appended unconditionally which can lead
to attempts to read missing mask files; modify the code that builds mask_paths
to mirror the labelmap handling: construct mask = seg_dir / f.replace(".nii.gz",
"_mask.nii.gz"), check mask.exists(), and append mask if present or append None
when missing (especially important when mask_dilation_mm == 0) so downstream
code can skip/derive masks safely; update any downstream expectations of
mask_paths to accept None just like the labelmap list.
In `@experiments/LongitudinalRegistration/3-eval_icon.py`:
- Around line 269-273: The itk.imwrite call that saves warped_ref currently
passes a Path object (method_dir / f"{subject_id}_g{timepoint}_warped_ref.mha");
change it to pass a string path like str(method_dir /
f"{subject_id}_g{timepoint}_warped_ref.mha") to match the later usage and ensure
compatibility with ITK versions that expect string filenames (refer to the
itk.imwrite call that writes warped_ref and the other call that already uses
str()).
In `@experiments/LongitudinalRegistration/composite_time_series_mid_slice.py`:
- Around line 88-101: Update the docstring of adjacent_slice_rmse to explicitly
state the expected 3D array shape and axis order (shape (N, Y, X) where N is the
number of slices and Y,X are spatial dims) and note that RMSE is computed
between consecutive slices along the first axis (axis 0 / N). Keep the rest of
the validation and implementation unchanged; ensure the docstring mentions
units/type expectations (numeric array) if relevant.
- Around line 58-85: Update the docstring for create_composite_volume to
explicitly document the axis order and shape: state that the function stacks
middle Z slices into a numpy array with shape (N, Y, X) (N = number of images)
by stacking along axis 0 (composite_array), and that itk.image_from_array
converts this to an ITK image with voxel ordering (X, Y, Z) where Z == N
(composite_image); also mention that spacing is set using
first_image.GetSpacing() with Z spacing forced to 1.0.
- Around line 46-55: Update the docstring for extract_middle_slice to explicitly
state the numpy array axis order and shape: note that itk.array_from_image
returns arrays in (Z, Y, X) order, and the function returns the middle Z slice
as a 2D numpy array with shape (Y, X) along with the original itk.Image; keep
the one-line summary and add a sentence describing the array ordering and
returned shape so callers know indexing semantics.
---
Outside diff comments:
In `@experiments/LongitudinalRegistration/2-finetune_icon.py`:
- Around line 123-162: The function gather_init_frames captures patient_id from
an outer scope which is fragile; change the signature to accept patient_id
(e.g., def gather_init_frames(initial_registration_dir: Path, patient_id: str)
-> tuple[...]) and update all references inside gather_init_frames (the print
messages) to use the parameter instead of a closure; then update the call
site(s) that invoke gather_init_frames to pass the appropriate patient_id value
(ensure the return type and behavior remain unchanged).
In `@src/physiomotion4d/cli/reconstruct_highres_4d_ct.py`:
- Around line 299-302: The code references args.icon_iterations but argparse
stores the flag as args.ICON_iterations; update the conditional to use
args.ICON_iterations (and keep the same fallback of 20) so the call to
workflow.set_number_of_iterations_ICON uses the correct attribute name; look for
the CLI parsing that defines --ICON-iterations and the call site
workflow.set_number_of_iterations_ICON to make this change.
---
Nitpick comments:
In `@experiments/LongitudinalRegistration/composite_time_series_mid_slice.py`:
- Around line 66-76: Current loop only checks middle slice shapes; update it to
also validate image metadata (spacing, origin, direction) for each image to
ensure consistency—either extend extract_middle_slice to return metadata
(spacing, origin, direction) or call a helper like
load_image_metadata(image_path) inside the loop, capture the first image's
metadata (e.g., first_metadata) and compare subsequent images' metadata to it,
and raise a ValueError with a clear message if any spacing/origin/direction
mismatch is found; keep using first_image/expected_shape and append
middle_slices only after metadata matches.
In `@src/physiomotion4d/workflow_fine_tune_icon_registration.py`:
- Around line 789-797: The TODO about setting the reference frame means we must
not hardcode reference_frame=0 and register_reference=True when a reference
image may be provided by the caller; update the call to
registrar.register_time_series to compute the correct reference_frame (e.g.,
index of the explicitly provided reference in moving_images or a provided
reference_index attribute) and set register_reference=False when the reference
image is already supplied, or make both reference_frame and register_reference
configurable from the workflow API; change the invocation around
moving_images/moving_masks/moving_labelmaps to use the computed reference_frame
and register_reference values so the registrar behavior matches the caller's
expectations.
🪄 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: f5e57d1c-5014-43a7-b84f-a699ad7cad81
📒 Files selected for processing (11)
experiments/LongitudinalRegistration/1-initial_registration.pyexperiments/LongitudinalRegistration/2-finetune_icon.pyexperiments/LongitudinalRegistration/3-eval_icon.pyexperiments/LongitudinalRegistration/composite_time_series_mid_slice.pysrc/physiomotion4d/cli/reconstruct_highres_4d_ct.pysrc/physiomotion4d/register_time_series_images.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_fine_tune_icon_registration.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.pysrc/physiomotion4d/workflow_reconstruct_highres_4d_ct.pytutorials/tutorial_06_reconstruct_highres_4d_ct.py
Comment on lines
+187
to
188
| mask = seg_dir / f.replace(".nii.gz", "_mask.nii.gz") | ||
| mask_paths.append(mask) |
There was a problem hiding this comment.
Mask path appended without existence check, unlike labelmap handling.
Line 186 correctly appends None when the labelmap doesn't exist, but line 188 unconditionally appends the mask Path even if the file is missing. When mask_dilation_mm=0 (line 236), the workflow won't derive masks and will fail if it tries to read a non-existent mask file.
labelmap_paths.append(labelmap if labelmap.exists() else None)
mask = seg_dir / f.replace(".nii.gz", "_mask.nii.gz")
- mask_paths.append(mask)
+ mask_paths.append(mask if mask.exists() else 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
| mask = seg_dir / f.replace(".nii.gz", "_mask.nii.gz") | |
| mask_paths.append(mask) | |
| mask = seg_dir / f.replace(".nii.gz", "_mask.nii.gz") | |
| mask_paths.append(mask if mask.exists() else 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 `@experiments/LongitudinalRegistration/2-finetune_icon.py` around lines 187 -
188, The mask Path is appended unconditionally which can lead to attempts to
read missing mask files; modify the code that builds mask_paths to mirror the
labelmap handling: construct mask = seg_dir / f.replace(".nii.gz",
"_mask.nii.gz"), check mask.exists(), and append mask if present or append None
when missing (especially important when mask_dilation_mm == 0) so downstream
code can skip/derive masks safely; update any downstream expectations of
mask_paths to accept None just like the labelmap list.
Comment on lines
+46
to
+55
| def extract_middle_slice(image_path: Path) -> tuple[np.ndarray, itk.Image]: | ||
| """Read ``image_path`` and return its middle Z slice and ITK image.""" | ||
| image = itk.imread(str(image_path)) | ||
| image_array = itk.array_from_image(image) | ||
| if image_array.ndim != 3: | ||
| raise ValueError( | ||
| f"Expected 3D image at {image_path}, got array shape {image_array.shape}" | ||
| ) | ||
| middle_slice_index = image_array.shape[0] // 2 | ||
| return image_array[middle_slice_index, :, :], image |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document array axis order and shape in docstring.
The docstring must state the axis order and shape of the returned numpy array. As per coding guidelines, every docstring that touches arrays must explicitly state axis order and shape.
The function returns a tuple where the first element is a 2D numpy array with shape (Y, X) (middle Z slice from the ITK image converted via itk.array_from_image, which uses Z, Y, X ordering).
📝 Proposed docstring update
def extract_middle_slice(image_path: Path) -> tuple[np.ndarray, itk.Image]:
- """Read ``image_path`` and return its middle Z slice and ITK image."""
+ """Read ``image_path`` and return its middle Z slice and ITK image.
+
+ Returns:
+ Tuple of (middle_slice_array, original_image) where middle_slice_array
+ has shape (Y, X) in numpy convention, and original_image is the full
+ 3D ITK image in (X, Y, Z) orientation.
+ """
image = itk.imread(str(image_path))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def extract_middle_slice(image_path: Path) -> tuple[np.ndarray, itk.Image]: | |
| """Read ``image_path`` and return its middle Z slice and ITK image.""" | |
| image = itk.imread(str(image_path)) | |
| image_array = itk.array_from_image(image) | |
| if image_array.ndim != 3: | |
| raise ValueError( | |
| f"Expected 3D image at {image_path}, got array shape {image_array.shape}" | |
| ) | |
| middle_slice_index = image_array.shape[0] // 2 | |
| return image_array[middle_slice_index, :, :], image | |
| def extract_middle_slice(image_path: Path) -> tuple[np.ndarray, itk.Image]: | |
| """Read ``image_path`` and return its middle Z slice and ITK image. | |
| Returns: | |
| Tuple of (middle_slice_array, original_image) where middle_slice_array | |
| has shape (Y, X) in numpy convention, and original_image is the full | |
| 3D ITK image in (X, Y, Z) orientation. | |
| """ | |
| image = itk.imread(str(image_path)) | |
| image_array = itk.array_from_image(image) | |
| if image_array.ndim != 3: | |
| raise ValueError( | |
| f"Expected 3D image at {image_path}, got array shape {image_array.shape}" | |
| ) | |
| middle_slice_index = image_array.shape[0] // 2 | |
| return image_array[middle_slice_index, :, :], image |
🤖 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/composite_time_series_mid_slice.py`
around lines 46 - 55, Update the docstring for extract_middle_slice to
explicitly state the numpy array axis order and shape: note that
itk.array_from_image returns arrays in (Z, Y, X) order, and the function returns
the middle Z slice as a 2D numpy array with shape (Y, X) along with the original
itk.Image; keep the one-line summary and add a sentence describing the array
ordering and returned shape so callers know indexing semantics.
Source: Coding guidelines
Comment on lines
+58
to
+85
| def create_composite_volume(image_files: list[Path]) -> itk.Image: | ||
| """Stack middle Z slices from 3D ITK images into a composite volume.""" | ||
| if not image_files: | ||
| raise ValueError("No input images were provided") | ||
|
|
||
| middle_slices: list[np.ndarray] = [] | ||
| first_image: Optional[itk.Image] = None | ||
| expected_shape: Optional[tuple[int, ...]] = None | ||
| for image_path in image_files: | ||
| middle_slice, image = extract_middle_slice(image_path) | ||
| if first_image is None: | ||
| first_image = image | ||
| expected_shape = middle_slice.shape | ||
| elif middle_slice.shape != expected_shape: | ||
| raise ValueError( | ||
| f"Middle slice shape mismatch for {image_path}: " | ||
| f"expected {expected_shape}, got {middle_slice.shape}" | ||
| ) | ||
| middle_slices.append(middle_slice) | ||
|
|
||
| composite_array = np.stack(middle_slices, axis=0) | ||
| composite_image = itk.image_from_array(composite_array) | ||
| if first_image is None: | ||
| raise ValueError("No input images were read") | ||
|
|
||
| input_spacing = first_image.GetSpacing() | ||
| composite_image.SetSpacing((float(input_spacing[0]), float(input_spacing[1]), 1.0)) | ||
| return composite_image |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document composite volume axis order and shape in docstring.
The docstring must explicitly state the axis order and shape of the composite volume. As per coding guidelines, every docstring that touches arrays must state axis order and shape.
The function stacks middle slices along axis 0, creating a numpy array with shape (N, Y, X) where N is the number of input images. When converted to an ITK image, this becomes an ITK image in (X, Y, Z) orientation where the Z dimension equals N.
📝 Proposed docstring update
def create_composite_volume(image_files: list[Path]) -> itk.Image:
- """Stack middle Z slices from 3D ITK images into a composite volume."""
+ """Stack middle Z slices from 3D ITK images into a composite volume.
+
+ Returns:
+ ITK image with shape (X, Y, N) in LPS orientation where N equals the
+ number of input images. Spacing is set to (input_X_spacing,
+ input_Y_spacing, 1.0).
+ """
if not image_files:🤖 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/composite_time_series_mid_slice.py`
around lines 58 - 85, Update the docstring for create_composite_volume to
explicitly document the axis order and shape: state that the function stacks
middle Z slices into a numpy array with shape (N, Y, X) (N = number of images)
by stacking along axis 0 (composite_array), and that itk.image_from_array
converts this to an ITK image with voxel ordering (X, Y, Z) where Z == N
(composite_image); also mention that spacing is set using
first_image.GetSpacing() with Z spacing forced to 1.0.
Source: Coding guidelines
Comment on lines
+88
to
+101
| def adjacent_slice_rmse(composite_array: np.ndarray) -> list[float]: | ||
| """Return RMSE values between each adjacent slice in ``composite_array``.""" | ||
| if composite_array.ndim != 3: | ||
| raise ValueError( | ||
| f"Expected 3D composite array, got shape {composite_array.shape}" | ||
| ) | ||
|
|
||
| rmse_values: list[float] = [] | ||
| for slice_index in range(composite_array.shape[0] - 1): | ||
| difference = composite_array[slice_index + 1].astype( | ||
| np.float64 | ||
| ) - composite_array[slice_index].astype(np.float64) | ||
| rmse_values.append(float(np.sqrt(np.mean(difference**2)))) | ||
| return rmse_values |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document expected array shape and axis order in docstring.
The docstring must explicitly state the expected shape and axis order of the input array. As per coding guidelines, every docstring that touches arrays must state axis order and shape.
The function expects a 3D array with shape (N, Y, X) and computes RMSE between consecutive slices along the first axis (N).
📝 Proposed docstring update
def adjacent_slice_rmse(composite_array: np.ndarray) -> list[float]:
- """Return RMSE values between each adjacent slice in ``composite_array``."""
+ """Return RMSE values between each adjacent slice in ``composite_array``.
+
+ Args:
+ composite_array: 3D numpy array with shape (N, Y, X) where N is the
+ number of slices.
+
+ Returns:
+ List of N-1 RMSE values, one for each adjacent slice pair along axis 0.
+ """
if composite_array.ndim != 3:🤖 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/composite_time_series_mid_slice.py`
around lines 88 - 101, Update the docstring of adjacent_slice_rmse to explicitly
state the expected 3D array shape and axis order (shape (N, Y, X) where N is the
number of slices and Y,X are spatial dims) and note that RMSE is computed
between consecutive slices along the first axis (axis 0 / N). Keep the rest of
the validation and implementation unchanged; ensure the docstring mentions
units/type expectations (numeric array) if relevant.
Source: Coding guidelines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
==========================================
+ Coverage 32.75% 32.92% +0.16%
==========================================
Files 53 53
Lines 7262 7229 -33
==========================================
+ Hits 2379 2380 +1
+ Misses 4883 4849 -34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
experiments/LongitudinalRegistration/3-eval_icon.py (1)
96-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when the split produces no held-out subjects.
If
timepoint_base_diris empty or contains only one patient,
test_subjectsbecomes empty, no CSVs are created, and Line 382 later crashes
ondetail_file.open(...). Raise an explicit error here, or short-circuit the
aggregate sections when there is nothing to evaluate.Proposed fix
all_patient_ids = sorted( p.name for p in timepoint_base_dir.iterdir() if p.is_dir() and p.name.startswith("pm00") ) +if len(all_patient_ids) < 2: + raise ValueError( + f"Need at least 2 subjects under {timepoint_base_dir} to form a " + f"held-out split; found {len(all_patient_ids)}" + ) + n_train = max( 1, min(len(all_patient_ids) - 1, round(train_fraction * len(all_patient_ids))) ) test_subjects = all_patient_ids[n_train:] +if not test_subjects: + raise ValueError( + f"Held-out split is empty for train_fraction={train_fraction}" + )🤖 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/3-eval_icon.py` around lines 96 - 109, Check the split logic after building all_patient_ids (from timepoint_base_dir) and ensure we fail fast if there are no held-out subjects: after computing n_train and test_subjects, add a check that raises a clear exception (or returns/short-circuits the rest of the evaluation) when test_subjects is empty so downstream code that opens detail files (later file I/O) does not crash; reference variables all_patient_ids, n_train, test_subjects and the timepoint_base_dir iteration to locate the spot to insert this guard or short-circuit logic.src/physiomotion4d/register_time_series_images.py (1)
120-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the
Nonecontract on this setter.The signature now accepts
Optional[int], but the docstring still reads like
callers must pass a concrete step count. Please state whetherNonedisables
extra ICON fine-tuning or preserves the backend default, and keep
RegisterImagesICON.set_number_of_iterations()aligned with the same contract.As per coding guidelines, "Update docstrings and tests for every changed public
method in Python."🤖 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 120 - 127, The setter set_number_of_iterations_ICON currently accepts Optional[int] but its docstring doesn't state the None contract; update the docstring to explicitly state what None means (e.g., "None disables extra ICON fine-tuning" or "None preserves backend default") and mirror that exact contract in RegisterImagesICON.set_number_of_iterations() so both public APIs behave and document the same way; update the parameter description to mention allowed values and None semantics, and adjust/add tests to cover passing an int and passing None.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/LongitudinalRegistration/3-eval_icon.py`:
- Around line 53-55: Update the stale comments that claim every method registers
to the "70th-percentile gated frame" to correctly state that the fixed image is
taken from files matching "*ref.nii.gz" (the actual fixed image source used when
loading the fixed image), and adjust the same wording near the all_methods
declaration and the other occurrence around the duplicate comment; search for
the all_methods block and any nearby comments referencing "70th-percentile" and
replace them with a brief accurate note that the reference/fixed image comes
from "*ref.nii.gz".
---
Outside diff comments:
In `@experiments/LongitudinalRegistration/3-eval_icon.py`:
- Around line 96-109: Check the split logic after building all_patient_ids (from
timepoint_base_dir) and ensure we fail fast if there are no held-out subjects:
after computing n_train and test_subjects, add a check that raises a clear
exception (or returns/short-circuits the rest of the evaluation) when
test_subjects is empty so downstream code that opens detail files (later file
I/O) does not crash; reference variables all_patient_ids, n_train, test_subjects
and the timepoint_base_dir iteration to locate the spot to insert this guard or
short-circuit logic.
In `@src/physiomotion4d/register_time_series_images.py`:
- Around line 120-127: The setter set_number_of_iterations_ICON currently
accepts Optional[int] but its docstring doesn't state the None contract; update
the docstring to explicitly state what None means (e.g., "None disables extra
ICON fine-tuning" or "None preserves backend default") and mirror that exact
contract in RegisterImagesICON.set_number_of_iterations() so both public APIs
behave and document the same way; update the parameter description to mention
allowed values and None semantics, and adjust/add tests to cover passing an int
and passing None.
🪄 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: 03614ac8-9b3c-4090-859d-f61ee0229d0f
📒 Files selected for processing (4)
experiments/LongitudinalRegistration/3-eval_icon.pysrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/register_time_series_images.pytests/test_workflow_fine_tune_icon_registration.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
src/physiomotion4d/cli/reconstruct_highres_4d_ct.py:118
--ICON-iterationsis defined without an explicitdest, so argparse will createargs.ICON_iterations, but later code readsargs.icon_iterations(lowercase). This will raiseAttributeErrorat runtime when the CLI is used.
parser.add_argument(
"--ICON-iterations",
type=int,
help="ICON fine-tuning iterations. Default: 20",
)
src/physiomotion4d/workflow_fine_tune_icon_registration.py:460
- This warning message still refers to "segmentation" and "paired-with-seg" even though the API has been renamed to labelmaps. This makes logs confusing and inconsistent with the dataset field name semantics in the surrounding code.
"Skipping %s: segmentation missing for paired-with-seg "
"training (seg=%s)",
image_path,
seg_file,
)
Comment on lines
658
to
662
| - Warps the moving image, segmentation, and landmarks into reference | ||
| space using ``forward_transform``. Segmentations use nearest-neighbor | ||
| space using ``forward_transform``. Labelmaps use nearest-neighbor | ||
| interpolation. Landmarks use ``inverse_transform.TransformPoint`` | ||
| (resampler-convention transform: maps moving-grid points back to | ||
| reference-grid points). |
Comment on lines
+686
to
+689
| moving_masks: Optional per-moving binary mask paths aligned with | ||
| ``moving_images``. Used to derive per-moving masks and returned | ||
| warped into reference space. Per-image ``None`` entries are | ||
| allowed. |
Comment on lines
+709
to
713
| - ``moving_to_reference_labelmaps`` (``list[Optional[itk.Image]]``): | ||
| each moving labelmap resampled onto the reference grid | ||
| with nearest-neighbor interpolation. ``None`` when the input | ||
| was ``None``. | ||
| - ``moving_to_reference_landmarks`` (``list[Optional[Landmarks]]``): |
Comment on lines
+879
to
883
| moving_lndmrks = ( | ||
| moving_landmarks[index] if moving_landmarks is not None else None | ||
| ) | ||
| if moving_lms is not None: | ||
| if moving_lndmrks is not None: | ||
| moving_to_reference_landmarks.append( |
Comment on lines
101
to
105
| self.registration_method_name: str = registration_method | ||
|
|
||
| self.registrar_ANTS = RegisterImagesANTS(log_level=log_level) | ||
| self.registrar_greedy = RegisterImagesGreedy(log_level=log_level) | ||
| self.registrar_ICON = RegisterImagesICON(log_level=log_level) | ||
|
|
Comment on lines
38
to
+39
| #: Supported registration backend identifiers. | ||
| REGISTRATION_METHODS: tuple[str, ...] = ("ANTS", "ICON") | ||
| REGISTRATION_METHODS: tuple[str, ...] = ("Greedy", "ICON") |
Comment on lines
+162
to
+163
| if self.registration_method == "Greedy": | ||
| self.log_info("Initializing Greedy registration...") |
Comment on lines
+123
to
+127
| def gather_init_frames( | ||
| initial_registration_dir: Path, | ||
| patient_id: str, | ||
| ) -> tuple[list[Path], list[Optional[Path]], list[Optional[Path]]]: | ||
| """Return ``(warped_image_paths, warped_labelmap_paths, warped_mask_paths)`` for one | ||
| ``initial_registration_dir / <method> / <patient_id>`` directory. | ||
| """Return ``(init_image_paths, init_labelmap_paths, init_mask_paths)`` for one |
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.
Summary by CodeRabbit
New Features
Changes