ENH: Per-organ tutorial parameters and distance-map registration fixes - #119
ENH: Per-organ tutorial parameters and distance-map registration fixes#119aylward wants to merge 1 commit into
Conversation
Introduce tutorials/parameters_lung_ct_dirlab.py and tutorials/parameters_heart_ct_kcl.py as the single source for each use case's mask dilation, distance-map saturation radius, PCA component counts, Greedy iteration schedule, segmenter class, and (heart only) the interior chamber label ids. Every tutorial that rasterizes or registers a distance map now reads the same values, so the maps a network is finetuned on match the maps it later infers over. No paths live in these modules; each tutorial keeps its own inputs and outputs. Add tutorials/tutorial_02_heart_distancemap_finetune_icon.py, which finetunes uniGradICON on heart distance maps built from the Duke-Heart-4DLabelmaps labelmaps with the chambers excluded. The heart needs its own run rather than reusing the lung weights: its registration mask is much tighter, so its distance maps saturate over a shorter radius and do not share an intensity distribution with the lung ones. Library fixes: - transform_tools.transform_image gains an explicit background_value. Resampling previously fell back to ITK's default of 0, which for CT is water, not air, so pre-warped moving images carried a false soft-tissue shell wherever they had no data. register_images_base now fills with -1000 HU for CT (exactly uniGradICON's window floor) on the image warp only; masks and labelmaps keep 0. - register_models_distance_maps composed the Greedy and ICON transforms in the wrong order. ITK's CompositeTransform applies back to front, so the residual must be added last in the forward and first in the inverse. Also drops two unconditional debug_*.nii.gz writes that crashed when mask_dilation_mm was 0. - register_from restores moving_image and clears moving_image_registered, so a registrar can be reused after an initialized run. - workflow_fit_statistical_model_to_patient grids the PCA field on a template-frame reference image rather than the patient image, and pads physically from spacing. - Default registrar for intensity registration switches from RegisterImagesGreedyICON/ICON to RegisterImagesGreedy in register_time_series_images and workflow_convert_image_to_usd. Distance-map registration keeps ICON, now with the finetuned weights. - convert_vtk_to_usd validates object names as USD identifiers and rejects duplicates; workflow_convert_vtk_to_usd wraps raw vtkDataSet. Rename number_of_components / number_of_modes to number_of_pca_components throughout the workflows and tutorials. tutorial_02_lung_finetune_icon now writes difference images (fixed minus registered) instead of the resampled volumes, and reports the chain's Greedy-stage-only score as its own row. The chain remains unconditional: on DIR-Lab, ICON's 175^3 residual grid is about 1.4 mm over the FOV, coarser than the 1.10 mm Greedy already achieves, so it cannot refine and the tutorial reports that honestly. tutorial_02_lung_distancemap_finetune_icon restricts its cached labelmaps to the lung labels. They previously held all 97 whole-body classes, and uniGradICON's Dice loss one-hots every shared class at 175^3 by batch 4, which saturated GPU memory. Baselines for the slow and GPU buckets will need refreshing: the composition-order fix and the PCA field grid change alter registration output.
WalkthroughThe PR changes selected workflows and tutorials from ICON-based registration to Greedy registration, adds configurable image and distance-map handling, centralizes heart and lung parameters, and introduces a Duke heart distance-map finetuning tutorial. ChangesRegistration and tutorial workflows
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Tutorial02Heart
participant DukeHeartLabelmaps
participant RegisterModelsDistanceMaps
participant ICONWeights
participant EvaluationOutputs
Tutorial02Heart->>DukeHeartLabelmaps: discover labelmaps and landmarks
DukeHeartLabelmaps-->>Tutorial02Heart: provide cases and evaluation inputs
Tutorial02Heart->>RegisterModelsDistanceMaps: train or load distance-map model
RegisterModelsDistanceMaps->>ICONWeights: save or validate checkpoint
Tutorial02Heart->>EvaluationOutputs: write transformed labelmaps and metrics
Possibly related PRs
🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
+ Coverage 42.18% 42.20% +0.01%
==========================================
Files 72 72
Lines 8742 8760 +18
==========================================
+ Hits 3688 3697 +9
- Misses 5054 5063 +9
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/physiotwin4d/transform_tools.py (1)
452-475: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse modality-aware fill values for off-grid intensity resampling.
Keep
background_value=0.0for masks and labelmaps. Use_prewarp_background_value()where intensity data is resampled to avoid zero-filling CT data as water. UpdateRegisterImagesBase.get_registered_image()and the time-series reconstruction path, then add a regression test for off-grid CT fill values.🤖 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/transform_tools.py` around lines 452 - 475, Update RegisterImagesBase.get_registered_image() and the time-series reconstruction path to pass _prewarp_background_value() for intensity-image resampling, while preserving background_value=0.0 for masks and labelmaps. Add a regression test verifying off-grid CT voxels receive the modality-appropriate fill value rather than zero.Source: Coding guidelines
🧹 Nitpick comments (3)
tutorials/parameters_heart_ct_kcl.py (1)
64-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the two interior-id fields use the same type.
interior_object_ids_totalsegmentatorisOptional[list[int]]andinterior_object_ids_simplewareislist[int]. Both always hold a list. The consumer parameterlabelmap_interior_object_idsacceptsOptional[list], solist[int]works for both. Drop theOptionalfor consistency, and then theOptionalimport becomes unused.♻️ Proposed change
- interior_object_ids_totalsegmentator: Optional[list[int]] = field( + interior_object_ids_totalsegmentator: list[int] = field( default_factory=lambda: [141, 142, 143, 144] )🤖 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/parameters_heart_ct_kcl.py` around lines 64 - 69, Update the interior_object_ids_totalsegmentator field to use list[int], matching interior_object_ids_simpleware and the labelmap_interior_object_ids consumer, and remove the now-unused Optional import.src/physiotwin4d/workflow_fit_statistical_model_to_patient.py (1)
248-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
distancemap_squared_maxto the classAttributesdocstring.The class docstring at lines 68-110 lists the configurable state, including
mask_dilation_mm. The new attribute is missing there. Add one line so readers find the knob and its default derivation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/workflow_fit_statistical_model_to_patient.py` at line 248, Update the Attributes class docstring to document distancemap_squared_max alongside the existing configurable state, including that its default is derived as appropriate from the implementation. Leave the attribute declaration and surrounding behavior unchanged.tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py (1)
95-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider defaulting
run_finetuningtoFalse.
run_finetuning = Truecombined with lines 225-230 deletesexperiment_diron every run.tutorials/tutorial_07_heart_fit_statistical_model_to_patient.pyreads the checkpoint from that tree, so a re-run of this tutorial discards the weights Tutorial 7 depends on before it retrains them. The sibling tutorialtutorials/tutorial_02_lung_finetune_icon.pydocuments the opposite default: finetuning is off so runs reuse the checkpoint. Align the two defaults, or state in the module docstring why the heart run always retrains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py` around lines 95 - 97, Default the run_finetuning setting in the tutorial to False, matching tutorials/tutorial_02_lung_finetune_icon.py so existing checkpoints are reused and experiment_dir is not deleted on reruns; retain the True option for explicitly requested retraining.
🤖 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/api/registration/chained.rst`:
- Around line 12-13: Update the documentation for RegisterImagesGreedyICON to
remove the incorrect claim that Tutorial 2 uses it. Either name a verified
consumer of the chained registrar or describe it only as the Greedy-then-ICON
pairing, while preserving the accurate statistical-model fit reference.
In `@docs/tutorials.rst`:
- Line 14: Update the tutorial summary near “Ten numbered stages across 17
runnable Python scripts” to avoid claiming all scripts are runnable: use “17
Python scripts” or explicitly distinguish the 16 runnable scripts from the
dataset-gated tutorial_02_duke_heart_distancemap_finetune_icon.py.
In `@src/physiotwin4d/register_images_base.py`:
- Around line 137-169: Add regression tests covering all branches of
_prewarp_background_value: an explicit override, CT returning -1000.0, and
non-CT using the moving image minimum. Add nearest-neighbor mask and labelmap
cases that verify zero fill, and assert the actual off-grid voxel value rather
than only output shape.
In `@src/physiotwin4d/transform_tools.py`:
- Around line 520-535: Update the background-value conversion in the resampling
flow before itk.resample_image_filter: for integer and discrete types, round to
an integer, validate it against np.iinfo(dtype), and reject out-of-range values
before invoking ITK; include boolean types via np.issubdtype(dtype, np.bool_).
Preserve floating-point conversion for non-discrete pixel types.
In `@src/physiotwin4d/workflow_reconstruct_highres_4d_ct.py`:
- Line 58: Update the class and constructor docstrings in
src/physiotwin4d/workflow_reconstruct_highres_4d_ct.py to consistently document
the standalone RegisterImagesGreedy backend and remove references to the retired
combined backend. In docs/tutorials.rst lines 313-321, remove Tutorial 3 from
Tutorial 2’s weight dependency; in lines 658-660, state that Tutorial 2 is
optional when stock weights are acceptable.
In `@tests/test_workflow_convert_image_to_usd.py`:
- Around line 121-122: Extend the migration test around workflow.registrar after
setting iterations to validate registration semantics, not just the
RegisterImagesGreedy type and artifact creation. Compare the registered image or
contours against an appropriate TestTools baseline, or assert a near-identity
result for the same-frame input, ensuring incorrect transform direction or
composition fails the test.
In `@tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py`:
- Around line 276-289: Update read_landmarks to validate the loaded markups
metadata before using control points: assert that the file’s coordinateSystem
field is exactly “LPS”, and fail clearly otherwise. Preserve the existing point
extraction only after this validation.
- Around line 198-217: After the training-case loop, validate that the collected
cohort is non-empty before the existing “Finetuning cohort” log and before
constructing WorkflowFinetuneICONRegistration. Mirror the held-out validation
behavior and fail immediately when subject_distance_map_files (and corresponding
subject IDs/labels) contains no surviving cases.
- Around line 139-151: The distance-map cache in distance_map_for currently keys
files only by frame stem, causing collisions across case directories. Include
the case directory name when constructing distance_map_file, while preserving
the existing derived_dir location and cache lookup behavior.
In `@tutorials/tutorial_07_heart_fit_statistical_model_to_patient.py`:
- Around line 164-169: Update the set_use_pca_registration call in the pca_model
block to pass the configured HEART_CT_KCL.pca_components(test_mode) value as
number_of_pca_components, matching the heart PCA builder and lung fit tutorial
instead of relying on the default.
---
Outside diff comments:
In `@src/physiotwin4d/transform_tools.py`:
- Around line 452-475: Update RegisterImagesBase.get_registered_image() and the
time-series reconstruction path to pass _prewarp_background_value() for
intensity-image resampling, while preserving background_value=0.0 for masks and
labelmaps. Add a regression test verifying off-grid CT voxels receive the
modality-appropriate fill value rather than zero.
---
Nitpick comments:
In `@src/physiotwin4d/workflow_fit_statistical_model_to_patient.py`:
- Line 248: Update the Attributes class docstring to document
distancemap_squared_max alongside the existing configurable state, including
that its default is derived as appropriate from the implementation. Leave the
attribute declaration and surrounding behavior unchanged.
In `@tutorials/parameters_heart_ct_kcl.py`:
- Around line 64-69: Update the interior_object_ids_totalsegmentator field to
use list[int], matching interior_object_ids_simpleware and the
labelmap_interior_object_ids consumer, and remove the now-unused Optional
import.
In `@tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py`:
- Around line 95-97: Default the run_finetuning setting in the tutorial to
False, matching tutorials/tutorial_02_lung_finetune_icon.py so existing
checkpoints are reused and experiment_dir is not deleted on reruns; retain the
True option for explicitly requested retraining.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e3e3d426-db8d-4bc2-a2e9-01e6df24641a
📒 Files selected for processing (30)
data/Duke-Heart-4DLabelmaps/.gitignoredata/Duke-Heart-4DLabelmaps/README.mddocs/api/registration/chained.rstdocs/tutorials.rstpyproject.tomlsrc/physiotwin4d/register_images_base.pysrc/physiotwin4d/register_images_chain.pysrc/physiotwin4d/register_models_distance_maps.pysrc/physiotwin4d/register_time_series_images.pysrc/physiotwin4d/transform_tools.pysrc/physiotwin4d/workflow_convert_image_to_usd.pysrc/physiotwin4d/workflow_fit_statistical_model_to_patient.pysrc/physiotwin4d/workflow_reconstruct_highres_4d_ct.pytests/test_workflow_convert_image_to_usd.pytests/test_workflow_reconstruct_highres_4d_ct.pytutorials/README.mdtutorials/parameters_heart_ct_kcl.pytutorials/parameters_lung_ct_dirlab.pytutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_01_lung_gated_ct_to_usd.pytutorials/tutorial_02_duke_heart_distancemap_finetune_icon.pytutorials/tutorial_02_lung_distancemap_finetune_icon.pytutorials/tutorial_02_lung_finetune_icon.pytutorials/tutorial_03_heart_reconstruct_highres_4d_ct.pytutorials/tutorial_03_lung_reconstruct_highres_4d_ct.pytutorials/tutorial_06_heart_create_statistical_model.pytutorials/tutorial_06_lung_create_statistical_model.pytutorials/tutorial_07_heart_fit_statistical_model_to_patient.pytutorials/tutorial_07_lung_fit_statistical_model_to_patient.pytutorials/tutorial_08_lung_fit_model_to_4d_patients.py
| ``RegisterImagesGreedyICON`` is the Greedy-then-ICON pairing, used by Tutorial 2 | ||
| and by the distance-map stage of the statistical-model fit. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching chained.rst:"
fd -a 'chained\.rst$' . || true
echo
echo "Target file excerpt:"
if [ -f docs/api/registration/chained.rst ]; then
nl -ba docs/api/registration/chained.rst | sed -n '1,80p'
fi
echo
echo "Search for RegisterImagesGreedyICON/RegisterModelsDistanceMaps/Tutorial 2:"
rg -n "RegisterImagesGreedyICON|RegisterModelsDistanceMaps|RegisterImagesGreedy|RegisterImagesICON|Tutorial 2|Tutorial.*2|distance-map|distance map" . || trueRepository: Project-MONAI/physiotwin4d
Length of output: 308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files relevant to registration:"
git ls-files | rg 'registration|tutorial|models|fit|distance' || true
echo
echo "All definitions/usages with some context:"
rg -n "class RegisterImagesGreedyICON|RegisterImagesGreedyICON\s*=|class RegisterModelsDistanceMaps|RegisterModelsDistanceMaps\s*=|RegisterImagesGreedy|RegisterImagesICON" . || true
echo
echo "Candidate files by name:"
git ls-files | rg 'registration|tutorial|statistical|model|distance' | head -200 || trueRepository: Project-MONAI/physiotwin4d
Length of output: 40451
Correct the documented consumers of RegisterImagesGreedyICON.
Tutorial 2 distance-map stages use separate RegisterImagesGreedy and RegisterImagesICON instances; they do not use RegisterImagesGreedyICON. Replace the Tutorial 2 claim with a consumer that actually uses the chained registrar, or keep the wording as the documented Greedy-then-ICON pattern.
🤖 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 `@docs/api/registration/chained.rst` around lines 12 - 13, Update the
documentation for RegisterImagesGreedyICON to remove the incorrect claim that
Tutorial 2 uses it. Either name a verified consumer of the chained registrar or
describe it only as the Greedy-then-ICON pairing, while preserving the accurate
statistical-model fit reference.
| def set_prewarp_background_value(self, background_value: float) -> None: | ||
| """Override the value a seeded registration's pre-warp writes off-grid. | ||
|
|
||
| Args: | ||
| background_value: Intensity written where the fixed grid samples | ||
| outside the moving image. Leave unset to derive it from the | ||
| modality; see :meth:`_prewarp_background_value`. | ||
| """ | ||
| self.prewarp_background_value = background_value | ||
|
|
||
| def _prewarp_background_value(self, moving_image: itk.Image) -> float: | ||
| """Return the intensity that means "no tissue" for the moving image. | ||
|
|
||
| Pre-warping onto the fixed grid samples outside the moving image | ||
| wherever the two extents disagree. ITK's default fill of 0 is wrong for | ||
| an intensity image: in CT it is water, so the filled region reads as | ||
| soft tissue rather than air and any downstream similarity metric treats | ||
| it as structure to match. -1000 HU is also uniGradICON's ``ct_window`` | ||
| lower bound, so it normalizes to exactly the same value as true air. | ||
|
|
||
| Args: | ||
| moving_image: Image being pre-warped. | ||
|
|
||
| Returns: | ||
| The explicit override when set, -1000.0 for CT, otherwise the | ||
| moving image's own minimum intensity. | ||
| """ | ||
| if self.prewarp_background_value is not None: | ||
| return self.prewarp_background_value | ||
| if self.modality == "ct": | ||
| return -1000.0 | ||
| return float(np.min(itk.GetArrayViewFromImage(moving_image))) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add regression tests for all pre-warp fill branches.
Cover the explicit override, CT -1000.0, and non-CT minimum branches. Also verify zero fill for nearest-neighbor masks and labelmaps. Assert an off-grid voxel value, not only the output shape.
As per coding guidelines: “For behavior changes and bug fixes, define verifiable success criteria and add or update tests that reproduce invalid inputs or regressions before implementation.”
Also applies to: 499-499
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/physiotwin4d/register_images_base.py` around lines 137 - 169, Add
regression tests covering all branches of _prewarp_background_value: an explicit
override, CT returning -1000.0, and non-CT using the moving image minimum. Add
nearest-neighbor mask and labelmap cases that verify zero fill, and assert the
actual off-grid voxel value rather than only output shape.
Source: Coding guidelines
| # ITK's wrapping types DefaultPixelValue to the image's pixel type, and | ||
| # rejects a Python float for an integer image. | ||
| dtype = itk.GetArrayViewFromImage(img).dtype | ||
| default_pixel_value: Union[int, float] = ( | ||
| int(round(background_value)) | ||
| if np.issubdtype(dtype, np.integer) | ||
| else float(background_value) | ||
| ) | ||
|
|
||
| img_reg = itk.resample_image_filter( | ||
| Input=img, | ||
| Transform=dftfm, | ||
| Interpolator=interpolator, | ||
| ReferenceImage=reference_image, | ||
| UseReferenceImage=True, | ||
| DefaultPixelValue=default_pixel_value, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'src/physiotwin4d/transform_tools.py' || true
echo "== relevant source =="
sed -n '480,555p' src/physiotwin4d/transform_tools.py | cat -n
echo "== function outline around class/method =="
ast-grep outline src/physiotwin4d/transform_tools.py --match transform_tools --view compact || true
rg -n "def .*|class .*|DefaultPixelValue|background_value|resample_image_filter|Transform" src/physiotwin4d/transform_tools.py
echo "== package itk version / dependency hints =="
for f in pyproject.toml requirements.txt requirements/*.txt setup.py setup.cfg; do
[ -f "$f" ] && echo "--- $f ---" && sed -n '1,220p' "$f"
doneRepository: Project-MONAI/physiotwin4d
Length of output: 17284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all background_value call sites =="
rg -n "transform_image\(|background_value\s*=" -S .
echo "== transform_image call sites with context =="
while IFS= read -r line; do
file=$(echo "$line" | cut -d: -f1)
n=$(echo "$line" | cut -d: -f2)
echo "--- $file:$n ---"
sed -n "$((n-8)),$((n+8))p" "$file" 2>/dev/null | cat -n
done < <(rg -n "background_value\s*=" -S .)
echo "== transform_image signature/docs =="
sed -n '440,492p' src/physiotwin4d/transform_tools.py | cat -n
echo "== ITK availability/version if present =="
python3 - <<'PY'
import importlib.util
spec = importlib.util.find_spec("itk")
print("itk_available", bool(spec), "itk_location", getattr(spec, "origin", None))
if spec:
import itk
print("itk_version", getattr(itk, "__version__", None))
PY
echo "== read-only probe of integer conversion and numpy iinfo behavior =="
python3 - <<'PY'
import numpy as np
for dtype_str, value in [("uint8", -1000.0), ("uint8", 350.0), ("uint16", "value"), ("uint8", True)]:
dtype = np.dtype(dtype_str)
rounded = int(round(value))
limits = np.iinfo(dtype)
print(f"{dtype_str} value={value!r} rounded={rounded} in_range={limits.min <= rounded <= limits.max}")
PY
echo "== inspect resample filter implementation docs via itk package if present =="
python3 - <<'PY'
import importlib.util
spec = importlib.util.find_spec("itk")
if not spec:
raise SystemExit
import itk
for name in ["_ImageIOHelper", "resample_image_filter"]:
obj = getattr(itk, name, None)
print(name, obj, type(obj))
print("has __doc__", bool(getattr(obj, "__doc__")))
doc = getattr(obj, "__doc__", "") or ""
lines = doc.splitlines()[:40]
for line in lines:
if line.strip().lower().startswith(("default", "pixel", "value", "arg")):
print(line)
PYRepository: Project-MONAI/physiotwin4d
Length of output: 8434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== focused call sites =="
for f in \
src/physiotwin4d/register_images_base.py \
src/physiotwin4d/workflow_fit_statistical_model_to_patient.py \
tutorials/tutorial_02_lung_finetune_icon.py \
tests/test_transform_tools.py
do
[ -f "$f" ] || continue
echo "--- $f ---"
grep -n "transform_image\s*(\|background_value\s*=" "$f" || true
done
echo "== register prewarp/distance-map call sites =="
sed -n '470,505p' src/physiotwin4d/register_images_base.py | cat -n
sed -n '730,765p' src/physiotwin4d/workflow_fit_statistical_model_to_patient.py | cat -n
echo "== tests around background and integer images =="
sed -n '40,190p' tests/test_transform_tools.py | cat -n
sed -n '685,735p' tests/test_transform_tools.py | cat -n
rg -n "uint8|uint16|int8|neighbour|nearest|distance|labelmap|background_value" tests src -S | head -n 200
echo "== python probe conversion and numpy iinfo =="
python3 - <<'PY'
import numpy as np
for dtype_str, value in [("uint8", -1000.0), ("uint8", 350.0), ("uint8", True), ("uint16", -999999.0)]:
dtype = np.dtype(dtype_str)
rounded = int(round(value))
limits = np.iinfo(dtype)
print(f"{dtype_str} value={value!r} rounded={rounded} in_range={limits.min <= rounded <= limits.max}")
PY
echo "== locate and inspect transform method implementation =="
rg -n " def transform_image\(" src/physiotwin4d/transform_tools.py
sed -n '446,540p' src/physiotwin4d/transform_tools.py | cat -nRepository: Project-MONAI/physiotwin4d
Length of output: 35765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== prewarp background resolver =="
sed -n '147,166p' src/physiotwin4d/register_images_base.py | cat -n
echo "== transform_image implementation =="
sed -n '446,540p' src/physiotwin4d/transform_tools.py | cat -n
echo "== focused examples with padded CT and transformed labelmap =="
sed -n '730,765p' src/physiotwin4d/workflow_fit_statistical_model_to_patient.py | cat -n
sed -n '358,370p' tutorials/tutorial_02_lung_finetune_icon.py | cat -n
echo "== exact Python behavior of background_value conversion =="
python3 - <<'PY'
import numpy as np
for dtype_str, value in [("uint8", -1000.0), ("uint8", 350.0), ("uint8", True), ("uint16", -999999.0), ("uint16", 65535.9)]:
dtype = np.dtype(dtype_str)
rounded = int(round(value))
limits = np.iinfo(dtype)
print(f"{dtype_str} value={value!r} rounded={rounded!r} dtype(rounded)={type(rounded).__name__} in_range={limits.min <= rounded <= limits.max}")
PYRepository: Project-MONAI/physiotwin4d
Length of output: 8925
🌐 Web query:
ITK Python itk image default_pixel_value unsigned integer overflow resample_image_filter DefaultPixelValue
💡 Result:
In ITK and SimpleITK, the ResampleImageFilter uses the DefaultPixelValue to fill regions sampled outside the input image domain [1][2][3]. While the filter interface often accepts a generic type (such as double in SimpleITK [3] or the specific PixelType in C++ ITK [4]), this value is internally cast to the pixel type of the output image [5][6]. Regarding unsigned integer overflow, ITK does not perform automatic bounds checking or clamping of the DefaultPixelValue to the range of the output pixel type by default [5][6]. If you provide a DefaultPixelValue that is outside the representable range of the output image's pixel type (e.g., a negative number or a value exceeding the maximum of an unsigned integer), the value will undergo standard C++ static_cast behavior, which can result in integer overflow or wrapping [5][6]. Key technical considerations: 1. Casting Behavior: The filter casts the DefaultPixelValue to the output PixelType [5][6]. For example, if your output image is unsigned 8-bit integer (sitkUInt8), and you set a DefaultPixelValue of -1, the resulting value will typically wrap around to 255. 2. Best Practices: To avoid unexpected overflow, always ensure the DefaultPixelValue is within the valid range of your target image pixel type (e.g., for 8-bit unsigned) [7]. 3. SimpleITK vs. ITK: SimpleITK provides a simplified interface where DefaultPixelValue is handled as a double [3], but the underlying casting to the target pixel type remains subject to the same potential for overflow if the value is incompatible with that type. If you require specific behavior (such as clamping), you must manually ensure the DefaultPixelValue is within the appropriate bounds before passing it to the filter [7].
Citations:
- 1: https://docs.itk.org/projects/doxygen/en/latest/classitk_1_1ResampleImageFilter.html
- 2: http://docs.itk.org/projects/doxygen/en/stable/classitk_1_1ResampleImageFilter.html
- 3: https://simpleitk.org/doxygen/v2_5/html/classitk_1_1simple_1_1ResampleImageFilter.html
- 4: https://docs.itk.org/projects/doxygen/en/stable/itkResampleImageFilter_8h_source.html
- 5: https://docs.itk.org/projects/doxygen/en/v5.3.0/itkResampleImageFilter_8h_source.html
- 6: https://docs.itk.org/projects/doxygen/en/v5.0.0/itkResampleImageFilter_8h_source.html
- 7: https://discourse.itk.org/t/isotropic-resampling-of-a-ct-scan/4160
🌐 Web query:
ITK 5.3.0 ResampleImageFilter Python DefaultPixelValue cast output pixel type static_cast
💡 Result:
In ITK 5.3.0, the ResampleImageFilter uses the DefaultPixelValue to fill pixels that fall outside the input image domain during resampling [1][2]. Regarding your technical query: 1. Pixel Type and Casting: The DefaultPixelValue is stored as the PixelType of the output image [1][3]. When you set this value, you are providing a value that must be compatible with the output image's pixel type. In the C++ implementation, this is managed by the template parameter TOutputImage [1][4]. Because the filter is templated, the value assigned to m_DefaultPixelValue is inherently constrained by the OutputImageType's PixelType [3][2]. 2. Python Context: When using the ITK Python wrapping, you generally do not need to perform manual static_cast calls yourself. The Python wrapping layer handles the conversion of native Python types (like int or float) into the appropriate C++ ITK PixelType automatically when you call SetDefaultPixelValue [5][6]. 3. Internal Mechanics: Internally, ITK does handle pixel conversion. For example, historical ITK versions have included a CastPixelWithBoundsChecking method [2][7][8] to handle the conversion of interpolated values to the output pixel type, ensuring the data is properly cast for the output image. If the value you provide to DefaultPixelValue is incompatible or requires specific casting, the ITK wrapping system or the underlying C++ template instantiation will typically handle the necessary assignment based on the output pixel type defined in your filter instantiation. If you encounter issues where the DefaultPixelValue is not being interpreted as expected, ensure that your output image type is explicitly defined (e.g., as unsigned char, float, etc.) and that the value passed to the filter is compatible with that specific type [5][4]. In Python, you can explicitly cast the value before setting it if you are concerned about precision or truncation, but this is usually handled by the ITK wrapper's type conversion system [5].
Citations:
- 1: https://docs.itk.org/projects/doxygen/en/v5.3.0/classitk%5F1%5F1ResampleImageFilter.html
- 2: http://docs.itk.org/projects/doxygen/en/stable/classitk_1_1ResampleImageFilter.html
- 3: https://docs.itk.org/projects/doxygen/en/v5.3.0/itkResampleImageFilter_8h_source.html
- 4: https://itk.org/files/Examples/src/Filtering/ImageGrid/ResampleAnImage/Documentation.html
- 5: https://itk.org/files/Examples/src/Filtering/ImageGrid/ResampleAVectorImage/Documentation.html
- 6: https://docs.itk.org/projects/doxygen/en/v5.3.0/Examples%5F2Filtering%5F2ResampleImageFilter2%5F8cxx-example.html
- 7: https://docs.itk.org/projects/doxygen/en/v5.1.0/classitk%5F1%5F1ResampleImageFilter.html
- 8: https://docs.itk.org/projects/doxygen/en/v4.12.0/itkResampleImageFilter_8h_source.html
🌐 Web query:
ITK source itkResampleImageFilter.hxx SetDefaultPixelValue static_cast or bounds checking
💡 Result:
In the ITK (Insight Segmentation and Registration Toolkit) ResampleImageFilter class, the SetDefaultPixelValue method is implemented via the standard ITK macro system [1][2]. Specifically, it uses itkSetMacro(DefaultPixelValue, PixelType), which generates a standard setter that assigns the provided argument directly to the m_DefaultPixelValue member variable [1][3][2]. The static_cast and bounds checking functionality you are inquiring about is not part of the SetDefaultPixelValue method itself [1][2]. Instead, that logic is located in the CastPixelWithBoundsChecking method (and related static helper functions like CastComponentWithBoundsChecking) within the ResampleImageFilter class [2][4][5]. This specialized mechanism is used internally during the image resampling process—specifically when interpolating pixel values and ensuring they are valid for the output pixel type—rather than for setting the default pixel value [2][5]. In summary: 1. SetDefaultPixelValue simply updates the member variable used when a transformed pixel falls outside the input image domain [6][7]. 2. CastPixelWithBoundsChecking is a separate internal utility method used to handle value conversions and clamping during the actual resampling execution [2][5].
Citations:
- 1: https://docs.itk.org/projects/doxygen/en/latest/itkResampleImageFilter_8h_source.html
- 2: https://docs.itk.org/projects/doxygen/en/v5.3.0/itkResampleImageFilter_8h_source.html
- 3: https://docs.itk.org/projects/doxygen/en/v3.8.0/itkResampleImageFilter_8h-source.html
- 4: https://docs.itk.org/projects/doxygen/en/v4.12.0/itkResampleImageFilter_8h_source.html
- 5: https://docs.itk.org/projects/doxygen/en/v4.8.0/classitk%5F1%5F1ResampleImageFilter.html
- 6: https://docs.itk.org/projects/doxygen/en/v5.1.0/classitk%5F1%5F1ResampleImageFilter.html
- 7: https://insightsoftwareconsortium.github.io/ITKDoxygen/classitk_1_1ResampleImageFilter.html
🌐 Web query:
site:discourse.itk.org ITK ResampleAnImage DefaultPixelValue unsigned char overflow
💡 Result:
In ITK and SimpleITK, the ResampleImageFilter relies on the DefaultPixelValue parameter to fill regions where the resampling grid maps to an area outside the input image domain [1][2]. When your output image pixel type is unsigned char (which has a range of 0–255), setting a DefaultPixelValue that falls outside this range will cause numerical overflow [3][4]. If you pass a value that is negative (e.g., -1000) or greater than 255 to an unsigned char image, the value will wrap around according to standard C++ integer overflow rules (often resulting in unexpected, low-intensity pixels) [3][4]. To resolve or prevent this behavior: 1. Ensure the DefaultPixelValue is within the valid range of your output image pixel type (e.g., for unsigned char). 2. If your data requires values outside the 0–255 range, change your output pixel type to a wider format such as float, int, or short (depending on your data requirements) before running the resampler [3][5]. 3. If you are resampling labels or mask images, ensure the DefaultPixelValue is set to a valid label value (e.g., 0 for background) [6][7]. 4. For pipelines involving floating-point calculations during interpolation, consider using a floating-point output type for the resampler, followed by a casting or rescaling filter to map the values back to unsigned char if necessary [3][8]. In SimpleITK, you can explicitly set the output pixel type when calling Execute or by using the SetOutputPixelType method to match your intended data range, which helps avoid implicit overflow issues during the resampling process [5][2].
Citations:
- 1: https://discourse.itk.org/t/resampling-to-volume-to-smaller-size-and-smaller-voxel-spacing/4984
- 2: https://discourse.itk.org/t/pixel-manipulations-in-c/5091
- 3: https://discourse.itk.org/t/resampleimagefilter-non-determinism-with-threading/862
- 4: https://discourse.itk.org/t/nifti-to-dicom-conversion-introduces-negative-intensities/4798
- 5: https://discourse.itk.org/t/simpleitk-wrong-resampling/4333
- 6: https://discourse.itk.org/t/resample-to-same-origin-spacing-and-size-as-another-image/3031
- 7: https://discourse.itk.org/t/resampling-pet-ct-and-binary-mask-images-to-resolution-1-1-1/2907
- 8: https://discourse.itk.org/t/writing-png-image-not-working/1431
Reject out-of-range integer background fills before resampling.
background_value=-1000.0 is valid for CT, but it becomes -1000 when converted for an unsigned integer image. ITK treats DefaultPixelValue as a member assignment then casts to the filter output pixel type, so unsigned fills can wrap and miss the intended air/background value. Validate the rounded integer against np.iinfo(dtype) before itk.resample_image_filter, and extend this to discrete pixel types such as np.issubdtype(dtype, np.bool_).
🤖 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/transform_tools.py` around lines 520 - 535, Update the
background-value conversion in the resampling flow before
itk.resample_image_filter: for integer and discrete types, round to an integer,
validate it against np.iinfo(dtype), and reject out-of-range values before
invoking ITK; include boolean types via np.issubdtype(dtype, np.bool_). Preserve
floating-point conversion for non-discrete pixel types.
| parameters (iteration counts, etc.) on the instance before passing it | ||
| in. Defaults to a new :class:`RegisterImagesGreedyICON` (Greedy followed | ||
| by ICON refinement) when omitted. | ||
| in. Defaults to a new :class:`RegisterImagesGreedy` when omitted. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize public documentation after the registration migration.
The implementation now uses standalone RegisterImagesGreedy, and Tutorial 8 treats Tutorial 2 distance-map weights as optional. The remaining documentation still advertises the removed combined backend and obsolete prerequisites.
- src/physiotwin4d/workflow_reconstruct_highres_4d_ct.py#L58-L58: update the class and constructor docstrings to describe
RegisterImagesGreedyconsistently. - docs/tutorials.rst#L313-L321: remove Tutorial 3 from the Tutorial 2 weight dependency.
- docs/tutorials.rst#L658-L660: update the Tutorial 8 run order so Tutorial 2 is optional when stock weights are acceptable.
As per coding guidelines, changed public docstrings must remain factual.
📍 Affects 2 files
src/physiotwin4d/workflow_reconstruct_highres_4d_ct.py#L58-L58(this comment)docs/tutorials.rst#L313-L321docs/tutorials.rst#L658-L660
🤖 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_reconstruct_highres_4d_ct.py` at line 58, Update
the class and constructor docstrings in
src/physiotwin4d/workflow_reconstruct_highres_4d_ct.py to consistently document
the standalone RegisterImagesGreedy backend and remove references to the retired
combined backend. In docs/tutorials.rst lines 313-321, remove Tutorial 3 from
Tutorial 2’s weight dependency; in lines 658-660, state that Tutorial 2 is
optional when stock weights are acceptable.
Source: Coding guidelines
| assert isinstance(workflow.registrar, RegisterImagesGreedy) | ||
| workflow.registrar.set_number_of_iterations([2]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add a semantic registration assertion to this migration test.
The test verifies the Greedy type and artifact creation, but it does not verify registration correctness. Because the backend and transform composition changed, an incorrect transform direction can still pass. Compare the registered image or contours with a TestTools baseline, or assert a near-identity result for the same-frame input.
As per coding guidelines: “When a test produces an image or surface, compare it with a baseline using utilities such as TestTools.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_workflow_convert_image_to_usd.py` around lines 121 - 122, Extend
the migration test around workflow.registrar after setting iterations to
validate registration semantics, not just the RegisterImagesGreedy type and
artifact creation. Compare the registered image or contours against an
appropriate TestTools baseline, or assert a near-identity result for the
same-frame input, ensuring incorrect transform direction or composition fails
the test.
Source: Coding guidelines
| def distance_map_for(labelmap_file: Path) -> Path: | ||
| """Rasterize one frame's heart distance map, caching it under derived_dir. | ||
|
|
||
| Mirrors ``RegisterModelsDistanceMaps._create_masks_from_models`` so the | ||
| finetuning inputs match what that class feeds ICON at inference: a | ||
| signed squared distance to the heart surface, normalized to [-1, 1] by | ||
| ``distance_squared_max``, then multiplied by 1000 to fill the | ||
| [-1000, 1000] window uniGradICON's CT preprocessing expects. | ||
| """ | ||
| stem = labelmap_file.name[: -len("_labelmap.nii.gz")] | ||
| distance_map_file = derived_dir / f"{stem}_distance_map.mha" | ||
| if distance_map_file.exists(): | ||
| return distance_map_file |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for Duke-Heart-4DLabelmaps frame naming conventions in the repo.
rg -n --hidden -C3 'Duke-Heart-4DLabelmaps|_labelmap\.nii\.gz|_landmark\.mrk\.json'Repository: Project-MONAI/physiotwin4d
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced tutorial around the distance-map cache code.
file="$(git ls-files | grep -F 'tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py' || true)"
if [ -z "$file" ]; then
echo "Referenced tutorial file not found"
exit 0
fi
echo "== file =="
echo "$file"
echo "== outline =="
ast-grep outline "$file" --view compact || true
echo "== relevant lines 90-190 =="
sed -n '90,190p' "$file" | nl -ba -v90
# Search repository for related identifiers/datasets without assuming matches.
echo "== repo references =="
rg -n --hidden -C2 'duke|Duke|heart|distance_map|_labelmap\.nii\.gz|_landmark\.mrk\.json|4DLabelmaps|derived_dir|case_dirs|frames_for_case' . || true
# Inspect referenced class code if present in repository.
echo "== class/model references =="
rg -n --hidden -C3 'class RegisterModelsDistanceMaps|def _create_masks_from_models|distance_squared_max|itk\.GetArrayViewFromImage\(distance_map\)' . || trueRepository: Project-MONAI/physiotwin4d
Length of output: 471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py"
echo "== tutorial lines 90-190 =="
python3 - <<'PY'
from pathlib import Path
p = Path("tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py")
text = p.read_text()
lines = text.splitlines()
for i, line in enumerate(lines, start=1):
if 90 <= i <= 190:
print(f"{i:04d}: {line}")
PY
echo "== cache path/source references =="
rg -n --hidden -C2 'distance_map_for|derived_dir|labelmap_file|_labelmap\.nii\.gz|case_dirs|frames_for_case|companion|labelmap' "$file" . || true
echo "== related tutorial references =="
rg -n --hidden -C2 'distance_map|DistanceMap|distance_squared_max|itk\.GetArrayViewFromImage\(distance_map\)|_labelmap\.nii\.gz' tutorials . || trueRepository: Project-MONAI/physiotwin4d
Length of output: 50383
Key the distance-map cache by case, not by frame stem only.
derived_dir is flat, but each case directory uses the same frame labels (frame_*.nii.gz). A shared stem for a gated frame collides across cases and writes into the same *_distance_map.mha file. Include the case directory name in distance_map_file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py` around lines
139 - 151, The distance-map cache in distance_map_for currently keys files only
by frame stem, causing collisions across case directories. Include the case
directory name when constructing distance_map_file, while preserving the
existing derived_dir location and cache lookup behavior.
| for case_dir in training_dirs: | ||
| frames = frames_for_case(case_dir) | ||
| if len(frames) < 2: | ||
| reporter.log_warning( | ||
| "Case %s has %d frame(s); skipping (paired training needs 2+)", | ||
| case_dir.name, | ||
| len(frames), | ||
| ) | ||
| continue | ||
| subject_ids.append(case_dir.name) | ||
| subject_distance_map_files.append( | ||
| [str(distance_map_for(frame)) for frame in frames] | ||
| ) | ||
| subject_labelmap_files.append([str(frame) for frame in frames]) | ||
| reporter.log_info( | ||
| "Finetuning cohort: %d cases, %d frames (held out %s)", | ||
| len(subject_ids), | ||
| sum(len(files) for files in subject_distance_map_files), | ||
| held_out_dir.name, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fail early when no training case survives the frame filter.
The guard at line 124 only requires two case directories. If every training case has fewer than two frames, the loop skips them all and subject_distance_map_files is empty. WorkflowFinetuneICONRegistration then receives an empty cohort and fails later, inside the finetuning subprocess. Add a check after the loop that mirrors the held-out check at lines 185-189.
🛡️ Proposed fix
+ if not subject_ids:
+ raise FileNotFoundError(
+ f"No training case under {data_dir} has the 2+ frames paired "
+ "finetuning needs."
+ )
reporter.log_info(
"Finetuning cohort: %d cases, %d frames (held out %s)",📝 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.
| for case_dir in training_dirs: | |
| frames = frames_for_case(case_dir) | |
| if len(frames) < 2: | |
| reporter.log_warning( | |
| "Case %s has %d frame(s); skipping (paired training needs 2+)", | |
| case_dir.name, | |
| len(frames), | |
| ) | |
| continue | |
| subject_ids.append(case_dir.name) | |
| subject_distance_map_files.append( | |
| [str(distance_map_for(frame)) for frame in frames] | |
| ) | |
| subject_labelmap_files.append([str(frame) for frame in frames]) | |
| reporter.log_info( | |
| "Finetuning cohort: %d cases, %d frames (held out %s)", | |
| len(subject_ids), | |
| sum(len(files) for files in subject_distance_map_files), | |
| held_out_dir.name, | |
| ) | |
| for case_dir in training_dirs: | |
| frames = frames_for_case(case_dir) | |
| if len(frames) < 2: | |
| reporter.log_warning( | |
| "Case %s has %d frame(s); skipping (paired training needs 2+)", | |
| case_dir.name, | |
| len(frames), | |
| ) | |
| continue | |
| subject_ids.append(case_dir.name) | |
| subject_distance_map_files.append( | |
| [str(distance_map_for(frame)) for frame in frames] | |
| ) | |
| subject_labelmap_files.append([str(frame) for frame in frames]) | |
| if not subject_ids: | |
| raise FileNotFoundError( | |
| f"No training case under {data_dir} has the 2+ frames paired " | |
| "finetuning needs." | |
| ) | |
| reporter.log_info( | |
| "Finetuning cohort: %d cases, %d frames (held out %s)", | |
| len(subject_ids), | |
| sum(len(files) for files in subject_distance_map_files), | |
| held_out_dir.name, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py` around lines
198 - 217, After the training-case loop, validate that the collected cohort is
non-empty before the existing “Finetuning cohort” log and before constructing
WorkflowFinetuneICONRegistration. Mirror the held-out validation behavior and
fail immediately when subject_distance_map_files (and corresponding subject
IDs/labels) contains no surviving cases.
| def read_landmarks(labelmap_file: Path) -> dict[str, np.ndarray]: | ||
| """Read a frame's Slicer markups file as ``{label: LPS point}``. | ||
|
|
||
| The markups files declare ``coordinateSystem: LPS``, the frame this | ||
| project works in, so the control points are used as written. | ||
| """ | ||
| landmark_file = companion(labelmap_file, "_landmark.mrk.json") | ||
| with landmark_file.open(encoding="utf-8") as f: | ||
| markups = json.load(f)["markups"] | ||
| return { | ||
| point["label"]: np.asarray(point["position"], dtype=np.float64) | ||
| for markup in markups | ||
| for point in markup["controlPoints"] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the LPS assumption instead of documenting it.
The docstring states that the markups files declare coordinateSystem: LPS, and the code uses the control points as written. The code never reads that field. 3D Slicer writes RAS in its default configuration. If one frame's file is RAS, the X and Y signs flip and every TRE number in the summary is wrong, with no error. Assert the field.
🛡️ Proposed fix
landmark_file = companion(labelmap_file, "_landmark.mrk.json")
with landmark_file.open(encoding="utf-8") as f:
markups = json.load(f)["markups"]
+ for markup in markups:
+ if markup.get("coordinateSystem") != "LPS":
+ raise ValueError(
+ f"{landmark_file} declares coordinateSystem "
+ f"{markup.get('coordinateSystem')!r}; this project works "
+ "in LPS."
+ )
return {📝 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.
| def read_landmarks(labelmap_file: Path) -> dict[str, np.ndarray]: | |
| """Read a frame's Slicer markups file as ``{label: LPS point}``. | |
| The markups files declare ``coordinateSystem: LPS``, the frame this | |
| project works in, so the control points are used as written. | |
| """ | |
| landmark_file = companion(labelmap_file, "_landmark.mrk.json") | |
| with landmark_file.open(encoding="utf-8") as f: | |
| markups = json.load(f)["markups"] | |
| return { | |
| point["label"]: np.asarray(point["position"], dtype=np.float64) | |
| for markup in markups | |
| for point in markup["controlPoints"] | |
| } | |
| def read_landmarks(labelmap_file: Path) -> dict[str, np.ndarray]: | |
| """Read a frame's Slicer markups file as ``{label: LPS point}``. | |
| The markups files declare ``coordinateSystem: LPS``, the frame this | |
| project works in, so the control points are used as written. | |
| """ | |
| landmark_file = companion(labelmap_file, "_landmark.mrk.json") | |
| with landmark_file.open(encoding="utf-8") as f: | |
| markups = json.load(f)["markups"] | |
| for markup in markups: | |
| if markup.get("coordinateSystem") != "LPS": | |
| raise ValueError( | |
| f"{landmark_file} declares coordinateSystem " | |
| f"{markup.get('coordinateSystem')!r}; this project works " | |
| "in LPS." | |
| ) | |
| return { | |
| point["label"]: np.asarray(point["position"], dtype=np.float64) | |
| for markup in markups | |
| for point in markup["controlPoints"] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tutorials/tutorial_02_duke_heart_distancemap_finetune_icon.py` around lines
276 - 289, Update read_landmarks to validate the loaded markups metadata before
using control points: assert that the file’s coordinateSystem field is exactly
“LPS”, and fail clearly otherwise. Preserve the existing point extraction only
after this validation.
| if pca_model is not None: | ||
| workflow.set_use_pca_registration( | ||
| use_pca_registration=True, | ||
| pca_model=pca_model, | ||
| use_surface=False, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare PCA component wiring across the fit tutorials.
rg -n -C6 'set_use_pca_registration|pca_components\(|running_as_test' tutorials/Repository: Project-MONAI/physiotwin4d
Length of output: 28363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file imports and relevant sections =="
sed -n '1,80p' tutorials/tutorial_07_heart_fit_statistical_model_to_patient.py
sed -n '120,180p' tutorials/tutorial_07_heart_fit_statistical_model_to_patient.py
echo
echo "== search WorkflowFitStatisticalModelToPatient and set_use_pca_registration signatures =="
rg -n -C8 'class WorkflowFitStatisticalModelToPatient|def set_use_pca_registration|number_of_pca_components.*use_pca' examples src tests tutorials 2>/dev/null || true
echo
echo "== compare tutorial 07 heart with 07 lung top imports =="
sed -n '1,35p' tutorials/tutorial_07_heart_fit_statistical_model_to_patient.py
sed -n '1,35p' tutorials/tutorial_07_lung_fit_statistical_model_to_patient.pyRepository: Project-MONAI/physiotwin4d
Length of output: 11158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WorkflowFitStatisticalModelToPatient __init__ and set_use_pca_registration doc =="
sed -n '50,130p' src/physiotwin4d/workflow_fit_statistical_model_to_patient.py
sed -n '345,385p' src/physiotwin4d/workflow_fit_statistical_model_to_patient.py
echo
echo "== static Python check: absent argument gets default zero =="
python3 - <<'PY'
import ast
from pathlib import Path
node = ast.parse(Path("tutorials/tutorial_07_heart_fit_statistical_model_to_patient.py").read_text())
for func in [n for n in node.body if isinstance(n, ast.FunctionDef) and n.name == "__main__"]:
if isinstance(node.body[node.body.index(func)-1], ast.ImportFrom):
continue
print({
"imports_HEART_CT_KCL": any(isinstance(im, ast.ImportFrom) for im in node.body if isinstance(im, ast.ImportFrom) and im.module == "parameters_heart_ct_kcl"),
"imports_WorkflowFitStatisticalModelToPatient": any(isinstance(im, ast.ImportFrom) for im in node.body if isinstance(im, ast.ImportFrom) and any(t.name == "WorkflowFitStatisticalModelToPatient" for t in im.names)),
})
for node_ast in ast.walk(node):
if getattr(node_ast, "attr", None) == "set_use_pca_registration":
print("arguments_named_number_of_pca_components:", any(kw.arg == "number_of_pca_components" for kw in node_ast.keywords))
print("docstring_uses_all_default_zero:", "use all components" in (ast.get_docstring(getattr(ast, "find_keyword", lambda **kw: None)(node_ast)) or ""))
PYRepository: Project-MONAI/physiotwin4d
Length of output: 7116
Pass the configured PCA component count.
set_use_pca_registration() defaults number_of_pca_components to 0, which is documented as "use all components", so this heart fit ignores the HEART_CT_KCL.pca_components(test_mode) value used by the heart PCA builder and the lung fit tutorial.
🐛 Proposed fix
if pca_model is not None:
workflow.set_use_pca_registration(
use_pca_registration=True,
pca_model=pca_model,
+ number_of_pca_components=HEART_CT_KCL.pca_components(test_mode),
use_surface=False,
)📝 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.
| if pca_model is not None: | |
| workflow.set_use_pca_registration( | |
| use_pca_registration=True, | |
| pca_model=pca_model, | |
| use_surface=False, | |
| ) | |
| if pca_model is not None: | |
| workflow.set_use_pca_registration( | |
| use_pca_registration=True, | |
| pca_model=pca_model, | |
| number_of_pca_components=HEART_CT_KCL.pca_components(test_mode), | |
| use_surface=False, | |
| ) |
🤖 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_heart_fit_statistical_model_to_patient.py` around lines
164 - 169, Update the set_use_pca_registration call in the pca_model block to
pass the configured HEART_CT_KCL.pca_components(test_mode) value as
number_of_pca_components, matching the heart PCA builder and lung fit tutorial
instead of relying on the default.
1 participant
Introduce tutorials/parameters_lung_ct_dirlab.py and tutorials/parameters_heart_ct_kcl.py as the single source for each use case's mask dilation, distance-map saturation radius, PCA component counts, Greedy iteration schedule, segmenter class, and (heart only) the interior chamber label ids. Every tutorial that rasterizes or registers a distance map now reads the same values, so the maps a network is finetuned on match the maps it later infers over. No paths live in these modules; each tutorial keeps its own inputs and outputs.
Add tutorials/tutorial_02_heart_distancemap_finetune_icon.py, which finetunes uniGradICON on heart distance maps built from the Duke-Heart-4DLabelmaps labelmaps with the chambers excluded. The heart needs its own run rather than reusing the lung weights: its registration mask is much tighter, so its distance maps saturate over a shorter radius and do not share an intensity distribution with the lung ones.
Library fixes:
Rename number_of_components / number_of_modes to
number_of_pca_components throughout the workflows and tutorials.
tutorial_02_lung_finetune_icon now writes difference images (fixed minus registered) instead of the resampled volumes, and reports the chain's Greedy-stage-only score as its own row. The chain remains unconditional: on DIR-Lab, ICON's 175^3 residual grid is about 1.4 mm over the FOV, coarser than the 1.10 mm Greedy already achieves, so it cannot refine and the tutorial reports that honestly.
tutorial_02_lung_distancemap_finetune_icon restricts its cached labelmaps to the lung labels. They previously held all 97 whole-body classes, and uniGradICON's Dice loss one-hots every shared class at 175^3 by batch 4, which saturated GPU memory.
Baselines for the slow and GPU buckets will need refreshing: the composition-order fix and the PCA field grid change alter registration output.
Summary by CodeRabbit
New Features
Documentation
Updates