ENH: Provide tutorial on workflow_finetune_icon_registration - #106
Conversation
Demonstrate on DirLab lung data. Compare performance with default weights.
|
Warning Review limit reached
Next review available in: 30 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds the ChangesICON finetuning workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Tutorial07
participant WorkflowFinetuneICONRegistration
participant uniGradICON
participant RegistrationEvaluator
Tutorial07->>WorkflowFinetuneICONRegistration: configure non-Case-1 cohort
WorkflowFinetuneICONRegistration->>uniGradICON: run finetuning when weights are absent
Tutorial07->>RegistrationEvaluator: run default and finetuned registration
RegistrationEvaluator-->>Tutorial07: return images, RMSE, timing, and loss
Tutorial07->>Tutorial07: write metrics and screenshots
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
Adds a new end-to-end Tutorial 7 demonstrating ICON finetuning on DIR-Lab 4D CT and comparing registration results against default weights, while also standardizing naming from “fine_tune” → “finetune” across the workflow API, docs, and tests.
Changes:
- Add
tutorial_07_finetune_icon_dirlab.pyto finetune uniGradICON on DIR-Lab and evaluate on a held-out Case 1 pair. - Rename/standardize the ICON finetuning workflow API and update references across tests, docs, experiments, and public exports.
- Document the uniGradICON
finetuningmodule source requirements inpyproject.toml.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_07_finetune_icon_dirlab.py | New tutorial: finetune on DIR-Lab cases (excluding Case 1) and compare default vs finetuned registration. |
| tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py | Renamed/updated tutorial 5 script content and outputs. |
| tutorials/README.md | Updates tutorial index (currently missing tutorial 7 entry). |
| tests/test_workflow_finetune_icon_registration.py | Updates tests to the WorkflowFinetuneICONRegistration naming/API. |
| tests/test_tutorials.py | Updates tutorial 5 runner to new filename (but still asserts a non-existent registered_file key). |
| statistics.md | Updates workflow filename reference. |
| src/physiotwin4d/workflow_finetune_icon_registration.py | Renames workflow class/args/methods to “finetune” naming; subprocess launch unchanged. |
| src/physiotwin4d/register_images_icon.py | Docstring wording updates (“finetuning”). |
| src/physiotwin4d/cli/reconstruct_highres_4d_ct.py | CLI help text wording update (“finetuning”). |
| src/physiotwin4d/init.py | Updates public import/export to WorkflowFinetuneICONRegistration. |
| README.md | Wording update (“finetuning”). |
| pyproject.toml | Adds uv source pin for uniGradICON finetuning branch; introduces an all extra. |
| experiments/README.md | Wording update (“finetuned/finetuning”). |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/setup.sh | Comment wording update (“finetuning”). |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/3-eval_icon.py | Comment wording update (“finetuning”). |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/2-finetune_icon.py | Updates workflow class/arg/method names to “finetune” naming. |
| docs/tutorials.rst | Updates tutorial 5 script reference. |
| docs/architecture.rst | Updates workflow naming references in architecture docs. |
Suppressed comments (1)
src/physiotwin4d/workflow_finetune_icon_registration.py:606
run_finetuning()always launchespython -m unigradicon.finetuning.finetunevia subprocess. If the installedunigradiconpackage doesn’t include thefinetuningmodule (as noted in pyproject.toml), this will fail with a less actionable subprocess error. Consider a small preflight check (whenunigradicon_src_pathis not being used) to raise a clearModuleNotFoundErrorwith install guidance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
========================================
Coverage 35.61% 35.61%
========================================
Files 64 64
Lines 8277 8170 -107
========================================
- Hits 2948 2910 -38
+ Misses 5329 5260 -69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py (1)
188-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConnect the baseline comparison to the tutorial output.
TestToolsconstructor only sets up directories, so theTestTools(...)call intutorials/tutorial_05_heart_fit_statistical_model_to_patient.pydoes not run baseline validation. Instantiatett, pass it into the screenshot logic, and calltt.compare_result_to_baseline_image()for the generated PNGs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py` around lines 188 - 225, Replace the standalone TestTools(...) call with a tt instance, then use tt in the screenshot-generation flow for before_path and after_path. After creating each PNG, call tt.compare_result_to_baseline_image() so both generated screenshots are validated against their baselines, while preserving the existing screenshot collection and tutorial_results output.pyproject.toml (1)
117-117: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
unigradiconto a reviewed commit.
pyproject.tomluses the mutable Git branchfeat-add-finetuning, and this repository has nouv.lockto capture the resolved commit. Pinunigradiconto a specific commit SHA or add an enforced lockfile before relying on the branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 117, Update the unigradicon dependency declaration in pyproject.toml to use a reviewed immutable commit SHA instead of the mutable feat-add-finetuning branch, or add and enforce a lockfile that records the resolved commit.
🤖 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 `@docs/tutorials.rst`:
- Line 285: Document Tutorial 7 in both indexes: in docs/tutorials.rst at lines
285-285, add its card, section, DIR-Lab dataset note, run command, and output
description; in tutorials/README.md at lines 21-21, add
tutorial_07_finetune_icon_dirlab.py with WorkflowFinetuneICONRegistration and
the DIR-Lab dataset.
In `@src/physiotwin4d/register_images_icon.py`:
- Line 63: Update the documentation near registration_method() and the
corresponding line around WorkflowFinetuneICONRegistration to describe the
configured finetune iteration count, noting that it defaults to 50 rather than
asserting a fixed 50-step count. Ensure the wording remains accurate when
number_of_iterations or CLI-provided finetune_steps overrides the default.
In `@src/physiotwin4d/workflow_finetune_icon_registration.py`:
- Around line 106-119: Update the doctest example in
WorkflowFinetuneICONRegistration to use double quotes for every string literal,
including file paths and finetune_name, while preserving the example’s structure
and values.
In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py`:
- Around line 89-92: Require pca_json before executing the workflow by raising
FileNotFoundError when it is missing, matching the existing pca_mean_file
handling. Update the pca_model loading block in the tutorial so downstream PCA
outputs remain guaranteed and the existing workflow assertions do not run with a
missing model.
- Around line 101-136: Update the cache condition guarding the segmentation flow
to require all four artifacts—the patient image, patient labelmap, heart
labelmap, and heart surface—to exist before loading cached outputs. If any
required path is missing, keep the existing generation and save logic in the
non-cached branch; only use the cached reads for a complete artifact set.
In `@tutorials/tutorial_07_finetune_icon_dirlab.py`:
- Around line 16-18: Update the checkpoint path documentation in the tutorial
and its related lines to use the factual reuse location returned by
WorkflowFinetuneICONRegistration.expected_weights_path():
tutorials/network_weights/icon_dirlab_4dct_model/checkpoints/Finetune_multi_final.trch.
Keep the documentation claims accurate and add or update docstrings or inline
comments as needed.
- Around line 171-173: Update the transform_tools.transform_image call to pass
result["inverse_transform"] when resampling moving_image onto fixed_image’s
grid. Keep forward_transform usage unchanged for point, surface, or mesh
transformations.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 117: Update the unigradicon dependency declaration in pyproject.toml to
use a reviewed immutable commit SHA instead of the mutable feat-add-finetuning
branch, or add and enforce a lockfile that records the resolved commit.
In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py`:
- Around line 188-225: Replace the standalone TestTools(...) call with a tt
instance, then use tt in the screenshot-generation flow for before_path and
after_path. After creating each PNG, call tt.compare_result_to_baseline_image()
so both generated screenshots are validated against their baselines, while
preserving the existing screenshot collection and tutorial_results output.
🪄 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 Plus
Run ID: 93ff7014-71bf-4cbc-bf1d-bd9a64e0f39d
📒 Files selected for processing (18)
README.mddocs/architecture.rstdocs/tutorials.rstexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/2-finetune_icon.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/3-eval_icon.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/setup.shexperiments/README.mdpyproject.tomlsrc/physiotwin4d/__init__.pysrc/physiotwin4d/cli/reconstruct_highres_4d_ct.pysrc/physiotwin4d/register_images_icon.pysrc/physiotwin4d/workflow_finetune_icon_registration.pystatistics.mdtests/test_tutorials.pytests/test_workflow_finetune_icon_registration.pytutorials/README.mdtutorials/tutorial_05_heart_fit_statistical_model_to_patient.pytutorials/tutorial_07_finetune_icon_dirlab.py
| registered = transform_tools.transform_image( | ||
| moving_image, result["forward_transform"], fixed_image | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the inverse transform for image resampling.
This call resamples moving_image on fixed_image's grid. Passing
result["forward_transform"] samples in the wrong direction. Use the inverse
registration transform. Otherwise, the saved registered image and its RMSE do
not represent the moving image registered to the fixed image.
Proposed fix
registered = transform_tools.transform_image(
- moving_image, result["forward_transform"], fixed_image
+ moving_image, result["inverse_transform"], fixed_image
)Based on learnings: use the inverse transform for image resampling and the
forward transform for points, surfaces, and meshes.
📝 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.
| registered = transform_tools.transform_image( | |
| moving_image, result["forward_transform"], fixed_image | |
| ) | |
| registered = transform_tools.transform_image( | |
| moving_image, result["inverse_transform"], fixed_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 `@tutorials/tutorial_07_finetune_icon_dirlab.py` around lines 171 - 173, Update
the transform_tools.transform_image call to pass result["inverse_transform"]
when resampling moving_image onto fixed_image’s grid. Keep forward_transform
usage unchanged for point, surface, or mesh transformations.
Source: Learnings
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py (1)
188-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConnect the baseline comparison to the tutorial output.
TestToolsconstructor only sets up directories, so theTestTools(...)call intutorials/tutorial_05_heart_fit_statistical_model_to_patient.pydoes not run baseline validation. Instantiatett, pass it into the screenshot logic, and calltt.compare_result_to_baseline_image()for the generated PNGs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py` around lines 188 - 225, Replace the standalone TestTools(...) call with a tt instance, then use tt in the screenshot-generation flow for before_path and after_path. After creating each PNG, call tt.compare_result_to_baseline_image() so both generated screenshots are validated against their baselines, while preserving the existing screenshot collection and tutorial_results output.pyproject.toml (1)
117-117: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
unigradiconto a reviewed commit.
pyproject.tomluses the mutable Git branchfeat-add-finetuning, and this repository has nouv.lockto capture the resolved commit. Pinunigradiconto a specific commit SHA or add an enforced lockfile before relying on the branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 117, Update the unigradicon dependency declaration in pyproject.toml to use a reviewed immutable commit SHA instead of the mutable feat-add-finetuning branch, or add and enforce a lockfile that records the resolved commit.
🤖 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 `@docs/tutorials.rst`:
- Line 285: Document Tutorial 7 in both indexes: in docs/tutorials.rst at lines
285-285, add its card, section, DIR-Lab dataset note, run command, and output
description; in tutorials/README.md at lines 21-21, add
tutorial_07_finetune_icon_dirlab.py with WorkflowFinetuneICONRegistration and
the DIR-Lab dataset.
In `@src/physiotwin4d/register_images_icon.py`:
- Line 63: Update the documentation near registration_method() and the
corresponding line around WorkflowFinetuneICONRegistration to describe the
configured finetune iteration count, noting that it defaults to 50 rather than
asserting a fixed 50-step count. Ensure the wording remains accurate when
number_of_iterations or CLI-provided finetune_steps overrides the default.
In `@src/physiotwin4d/workflow_finetune_icon_registration.py`:
- Around line 106-119: Update the doctest example in
WorkflowFinetuneICONRegistration to use double quotes for every string literal,
including file paths and finetune_name, while preserving the example’s structure
and values.
In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py`:
- Around line 89-92: Require pca_json before executing the workflow by raising
FileNotFoundError when it is missing, matching the existing pca_mean_file
handling. Update the pca_model loading block in the tutorial so downstream PCA
outputs remain guaranteed and the existing workflow assertions do not run with a
missing model.
- Around line 101-136: Update the cache condition guarding the segmentation flow
to require all four artifacts—the patient image, patient labelmap, heart
labelmap, and heart surface—to exist before loading cached outputs. If any
required path is missing, keep the existing generation and save logic in the
non-cached branch; only use the cached reads for a complete artifact set.
In `@tutorials/tutorial_07_finetune_icon_dirlab.py`:
- Around line 16-18: Update the checkpoint path documentation in the tutorial
and its related lines to use the factual reuse location returned by
WorkflowFinetuneICONRegistration.expected_weights_path():
tutorials/network_weights/icon_dirlab_4dct_model/checkpoints/Finetune_multi_final.trch.
Keep the documentation claims accurate and add or update docstrings or inline
comments as needed.
- Around line 171-173: Update the transform_tools.transform_image call to pass
result["inverse_transform"] when resampling moving_image onto fixed_image’s
grid. Keep forward_transform usage unchanged for point, surface, or mesh
transformations.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 117: Update the unigradicon dependency declaration in pyproject.toml to
use a reviewed immutable commit SHA instead of the mutable feat-add-finetuning
branch, or add and enforce a lockfile that records the resolved commit.
In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py`:
- Around line 188-225: Replace the standalone TestTools(...) call with a tt
instance, then use tt in the screenshot-generation flow for before_path and
after_path. After creating each PNG, call tt.compare_result_to_baseline_image()
so both generated screenshots are validated against their baselines, while
preserving the existing screenshot collection and tutorial_results output.
🪄 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 Plus
Run ID: 93ff7014-71bf-4cbc-bf1d-bd9a64e0f39d
📒 Files selected for processing (18)
README.mddocs/architecture.rstdocs/tutorials.rstexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/2-finetune_icon.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/3-eval_icon.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/setup.shexperiments/README.mdpyproject.tomlsrc/physiotwin4d/__init__.pysrc/physiotwin4d/cli/reconstruct_highres_4d_ct.pysrc/physiotwin4d/register_images_icon.pysrc/physiotwin4d/workflow_finetune_icon_registration.pystatistics.mdtests/test_tutorials.pytests/test_workflow_finetune_icon_registration.pytutorials/README.mdtutorials/tutorial_05_heart_fit_statistical_model_to_patient.pytutorials/tutorial_07_finetune_icon_dirlab.py
🛑 Comments failed to post (2)
tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py (2)
89-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require
pca_model.jsonbefore workflow execution.These lines treat the PCA model as optional. Later code requires
workflow.pca_template_modelandworkflow.pca_template_model_surface, which
the workflow documents as PCA-registration outputs. If the mean surface exists
butpca_model.jsonis missing, the tutorial performs processing and then
fails at the assertions.Raise
FileNotFoundErrorfor a missingpca_json, as done for
pca_mean_file, or make all downstream PCA-only outputs conditional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py` around lines 89 - 92, Require pca_json before executing the workflow by raising FileNotFoundError when it is missing, matching the existing pca_mean_file handling. Update the pca_model loading block in the tutorial so downstream PCA outputs remain guaranteed and the existing workflow assertions do not run with a missing model.
101-136: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the complete cache before loading cached outputs.
The cache condition checks only the patient image. If a prior run writes that
file but fails before either labelmap or the surface is saved, the next run
takes the cached branch and fails while reading a missing artifact.Check all four cache paths before taking the cached branch. Recreate the full
artifact set when any required cache file is absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_05_heart_fit_statistical_model_to_patient.py` around lines 101 - 136, Update the cache condition guarding the segmentation flow to require all four artifacts—the patient image, patient labelmap, heart labelmap, and heart surface—to exist before loading cached outputs. If any required path is missing, keep the existing generation and save logic in the non-cached branch; only use the cached reads for a complete artifact set.
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 (4)
tests/test_tutorials.py (2)
159-160: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the test image shape and axis order.
The test docstring only states the test purpose. Add the actual shape in
(X, Y, Z[, T])order and theLPSworld frame.As per coding guidelines, every test docstring must state the image shape and axis order.
🤖 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_tutorials.py` around lines 159 - 160, Update the TestTutorial05FitStatisticalModelToPatient docstring to include the test image dimensions in (X, Y, Z[, T]) order and explicitly identify the LPS world frame, while retaining its existing purpose description.Source: Coding guidelines
162-162: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the missing
tutorial_05_heart_fit_statistical_model_to_patientbaselines.
_class_namepoints totests/baselines/tutorial_05_heart_fit_statistical_model_to_patient, but this directory is not present. Either restore the baseline files under that path or update the baseline directory path before the test runs.🤖 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_tutorials.py` at line 162, Provide the missing baselines referenced by _class_name under tests/baselines/tutorial_05_heart_fit_statistical_model_to_patient, or update _class_name to the correct existing baseline directory before the test executes.src/physiotwin4d/workflow_finetune_icon_registration.py (2)
459-462: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid a trailing path separator in
PYTHONPATH.If
PYTHONPATHis unset, the result ends withos.pathsepand an empty entry. Python interprets an empty entry as the current working directory, which makes the caller's directory importable in the subprocess. Join only the non-empty parts.Proposed fix
if self.unigradicon_src_path is not None: - env["PYTHONPATH"] = ( - str(self.unigradicon_src_path) + os.pathsep + env.get("PYTHONPATH", "") - ) + existing_pythonpath = env.get("PYTHONPATH", "") + env["PYTHONPATH"] = os.pathsep.join( + part + for part in (str(self.unigradicon_src_path), existing_pythonpath) + if part + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/workflow_finetune_icon_registration.py` around lines 459 - 462, Update the PYTHONPATH construction in the unigradicon_src_path handling to join only non-empty path components, preventing a trailing os.pathsep when env["PYTHONPATH"] is unset while preserving any existing non-empty PYTHONPATH value.
397-399: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet
training.use_labelfromself._use_labelmapswhen writing segmentations.
training.use_labelis still hardcoded toFalseeven thoughprepare_dataset()writessegmentationentries. Set it toself._use_labelmapsso the finetuning config does not omit label-based supervision when segmentations are present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/workflow_finetune_icon_registration.py` around lines 397 - 399, Update the finetuning configuration’s `use_label` field to use `self._use_labelmaps` instead of the hardcoded `False`, preserving label-based supervision whenever `prepare_dataset()` writes segmentation entries.
🧹 Nitpick comments (1)
tests/test_workflow_finetune_icon_registration.py (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState the synthetic image shape and axis order in the fixture docstring.
This fixture builds synthetic volumes on disk. The docstring does not state the shape, the axis order, or the world frame. Add them, and state that the volumes are synthetic and small.
Proposed docstring fix
- """Two patients, two frames each, with matching labelmaps and masks on disk.""" + """Two patients, two frames each, with matching labelmaps and masks on disk. + + Synthetic volumes, shape (X, Y, Z, T) = (8, 8, 8, 1), LPS world frame. + """Apply the same change to the docstrings at Line 216 and Line 270.
As per coding guidelines,
tests/**/*.pymust "State image shape and axis order in every test docstring" and synthetic volumes "must be at most 64 voxels per side and this must be stated in the docstring".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workflow_finetune_icon_registration.py` at line 43, Update the fixture docstrings at the referenced locations to state that the volumes are small synthetic images, include their dimensions with no side exceeding 64 voxels, specify the axis order, and identify the world coordinate frame; apply the same wording consistently to all three docstrings.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 `@src/physiotwin4d/workflow_finetune_icon_registration.py`:
- Around line 186-187: Prevent finetuning from starting with no usable frames:
in src/physiotwin4d/workflow_finetune_icon_registration.py lines 186-187, update
use_labelmaps and use_masks to enable only when their companion lists contain at
least one non-None entry; in lines 345-353, have process() raise ValueError when
dataset_entries is empty before writing the dataset JSON.
---
Outside diff comments:
In `@src/physiotwin4d/workflow_finetune_icon_registration.py`:
- Around line 459-462: Update the PYTHONPATH construction in the
unigradicon_src_path handling to join only non-empty path components, preventing
a trailing os.pathsep when env["PYTHONPATH"] is unset while preserving any
existing non-empty PYTHONPATH value.
- Around line 397-399: Update the finetuning configuration’s `use_label` field
to use `self._use_labelmaps` instead of the hardcoded `False`, preserving
label-based supervision whenever `prepare_dataset()` writes segmentation
entries.
In `@tests/test_tutorials.py`:
- Around line 159-160: Update the TestTutorial05FitStatisticalModelToPatient
docstring to include the test image dimensions in (X, Y, Z[, T]) order and
explicitly identify the LPS world frame, while retaining its existing purpose
description.
- Line 162: Provide the missing baselines referenced by _class_name under
tests/baselines/tutorial_05_heart_fit_statistical_model_to_patient, or update
_class_name to the correct existing baseline directory before the test executes.
---
Nitpick comments:
In `@tests/test_workflow_finetune_icon_registration.py`:
- Line 43: Update the fixture docstrings at the referenced locations to state
that the volumes are small synthetic images, include their dimensions with no
side exceeding 64 voxels, specify the axis order, and identify the world
coordinate frame; apply the same wording consistently to all three 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 Plus
Run ID: 2c69d021-d065-4b57-a7a6-7f1705795cf5
📒 Files selected for processing (7)
docs/architecture.rstexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/2-finetune_icon.pysrc/physiotwin4d/register_images_icon.pysrc/physiotwin4d/workflow_finetune_icon_registration.pytests/test_tutorials.pytests/test_workflow_finetune_icon_registration.pytutorials/tutorial_07_finetune_icon_dirlab.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/physiotwin4d/register_images_icon.py
| self.use_labelmaps: bool = subject_labelmap_files is not None | ||
| self.use_masks: bool = subject_mask_files is not None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Finetuning can start on an empty dataset. Nothing checks that at least one usable frame survives companion-file filtering, so process() can launch uniGradICON with {"data": []}.
src/physiotwin4d/workflow_finetune_icon_registration.py#L186-L187: deriveuse_labelmapsanduse_masksfrom the presence of at least one non-Noneentry instead ofis not None, so an all-Nonecompanion list does not enable filtering that drops every frame.src/physiotwin4d/workflow_finetune_icon_registration.py#L345-L353: raise aValueErrorwhendataset_entriesis empty, before writing the dataset JSON.
📍 Affects 1 file
src/physiotwin4d/workflow_finetune_icon_registration.py#L186-L187(this comment)src/physiotwin4d/workflow_finetune_icon_registration.py#L345-L353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/physiotwin4d/workflow_finetune_icon_registration.py` around lines 186 -
187, Prevent finetuning from starting with no usable frames: in
src/physiotwin4d/workflow_finetune_icon_registration.py lines 186-187, update
use_labelmaps and use_masks to enable only when their companion lists contain at
least one non-None entry; in lines 345-353, have process() raise ValueError when
dataset_entries is empty before writing the dataset JSON.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/physiotwin4d/workflow_finetune_icon_registration.py:417
- The YAML config write should specify UTF-8 explicitly (matching other file writes in the repo). Also,
yaml.safe_dumpis preferable when emitting YAML to avoid accidentally serializing arbitrary Python objects.
config_yaml_path = self.experiment_dir / f"{self.finetune_name}_config.yaml"
with config_yaml_path.open("w") as fh:
yaml.dump(config, fh, default_flow_style=False, sort_keys=False)
src/physiotwin4d/workflow_finetune_icon_registration.py:18
- Add an explicit import for the finetuning-module preflight check. The process() method should verify that
unigradicon.finetuning.finetuneis importable (especially for pip installs where the PyPIunigradiconpackage may not ship finetuning), which requiresimportlib.utilto be available in this module.
import json
import logging
import os
import subprocess
import sys
src/physiotwin4d/workflow_finetune_icon_registration.py:472
process()launchespython -m unigradicon.finetuning.finetune, but if the installedunigradiconpackage doesn’t include thefinetuningmodule (as noted in pyproject comments), this will fail later with a less actionable subprocess error. Add an explicit preflight check that the module is importable (respectingunigradicon_src_pathwhen provided) and raise a clear error with install instructions before starting the run.
env = os.environ.copy()
env["PYTHONUTF8"] = "1"
if self.unigradicon_src_path is not None:
env["PYTHONPATH"] = (
str(self.unigradicon_src_path) + os.pathsep + env.get("PYTHONPATH", "")
)
cmd = [
sys.executable,
"-m",
"unigradicon.finetuning.finetune",
"--config",
str(config_yaml_path),
]
self.log_info("Launching finetuning subprocess: %s", " ".join(cmd))
subprocess.run(cmd, check=True, env=env)
src/physiotwin4d/workflow_finetune_icon_registration.py:347
- File writes should specify UTF-8 explicitly for cross-platform consistency (the codebase does this elsewhere, e.g. workflow_infer_physicsnemo.py). This avoids platform-default encodings affecting the generated JSON/YAML configs.
This issue also appears on line 415 of the same file.
dataset_json_path = self.experiment_dir / f"{self.finetune_name}_dataset.json"
with dataset_json_path.open("w") as fh:
json.dump({"data": dataset_entries}, fh, indent=2)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/physiotwin4d/workflow_finetune_icon_registration.py:69
- Docstring examples should follow the project’s double-quote rule; these example string literals use single quotes.
... output_dir=Path('d:/PhysioTwin4D/icon_finetuned'),
... finetune_name='duke_4d_gated_icon_ft',
... subject_labelmap_files=[
... ['pm0001/g000_labelmap.nii.gz', 'pm0001/g050_labelmap.nii.gz'],
... ['pm0002/g000_labelmap.nii.gz', 'pm0002/g050_labelmap.nii.gz'],
... ],
src/physiotwin4d/workflow_finetune_icon_registration.py:64
- Docstring examples should follow the project’s double-quote rule; these example string literals use single quotes.
This issue also appears on line 64 of the same file.
... ['pm0001/g000.nii.gz', 'pm0001/g050.nii.gz'],
... ['pm0002/g000.nii.gz', 'pm0002/g050.nii.gz'],
... ],
... output_dir=Path('d:/PhysioTwin4D/icon_finetuned'),
src/physiotwin4d/workflow_finetune_icon_registration.py:347
- prepare_dataset() will happily write an empty dataset JSON if all frames are skipped (e.g., missing required masks/segmentations), which will later fail inside uniGradICON with a harder-to-diagnose error. Consider failing fast with a clear message, and write JSON with an explicit UTF-8 encoding for Windows safety.
dataset_json_path = self.experiment_dir / f"{self.finetune_name}_dataset.json"
with dataset_json_path.open("w") as fh:
json.dump({"data": dataset_entries}, fh, indent=2)
src/physiotwin4d/workflow_finetune_icon_registration.py:417
- Write the YAML config with an explicit UTF-8 encoding to avoid platform-default encoding issues (e.g., cp1252 on Windows) if any paths contain non-ASCII characters.
config_yaml_path = self.experiment_dir / f"{self.finetune_name}_config.yaml"
with config_yaml_path.open("w") as fh:
yaml.dump(config, fh, default_flow_style=False, sort_keys=False)
src/physiotwin4d/workflow_finetune_icon_registration.py:470
- As written, process() always launches
-m unigradicon.finetuning.finetune, but the PR also notes the PyPIunigradiconrelease may not shipunigradicon.finetuning. Adding a small preflight check when unigradicon_src_path is not provided would fail fast with a clear install instruction.
cmd = [
sys.executable,
"-m",
"unigradicon.finetuning.finetune",
"--config",
Demonstrate on DirLab lung data. Compare performance with default weights.
Summary by CodeRabbit
New Features
Breaking Changes
Documentation