ENH: Longitudinal registration - ICON fine tuning#62
Merged
Conversation
WalkthroughThis PR standardizes naming conventions across registration APIs and introduces new longitudinal registration capabilities. The core change replaces lowercase naming ( ChangesNaming Standardization
Workflow Updates
Longitudinal Registration Experiments
Documentation and Configuration
🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR adds an ICON fine-tuning workflow (wrapping the uniGradICON finetuning subprocess) and supporting utilities/tests, while also standardizing registration method strings and related API names to uppercase forms (e.g., ANTS, ICON, ANTS_ICON) across tutorials, docs, CLIs, and workflows.
Changes:
- Introduces
WorkflowFineTuneICONRegistration(dataset/config generation + fine-tuning subprocess + apply-with-finetuned-weights path) and a newLandmarkToolsutility module. - Adds unit tests for ICON fine-tuning workflow behaviors (dataset/config generation, mask derivation, subprocess invocation, validation guards).
- Renames/standardizes ANTs/ICON-related identifiers across code/docs/tests (e.g.,
RegisterImagesANTS,registrar_ANTS,set_number_of_iterations_ANTS,registration_method="ANTS_ICON"), and updates various experiment scripts accordingly.
Reviewed changes
Copilot reviewed 55 out of 56 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_08_dirlab_pca_time_series.py | Updates tutorial to use ANTS_ICON registration method string. |
| tutorials/tutorial_06_reconstruct_highres_4d_ct.py | Updates tutorial constants and iteration setter naming to ANTS/ANTS-prefixed forms. |
| tutorials/tutorial_01_heart_gated_ct_to_usd.py | Updates tutorial text references for ANTs registrar naming. |
| tests/test_workflow_fine_tune_icon_registration.py | Adds comprehensive unit tests for the new fine-tuning workflow and mask creation helper. |
| tests/test_transform_tools.py | Updates documentation string references to renamed ANTs tests. |
| tests/test_register_time_series_images.py | Updates tests to new registration method strings and renamed registrar attributes/setters. |
| tests/test_register_images_icon.py | Renames fixtures/paths to ICON-style names in tests. |
| tests/test_register_images_greedy.py | Updates documentation string references to renamed ANTs tests. |
| tests/test_register_images_ants.py | Renames RegisterImagesANTs usage to RegisterImagesANTS and related identifiers. |
| tests/README.md | Updates test module references (currently to uppercase filenames). |
| tests/conftest.py | Updates fixtures/imports to RegisterImagesANTS and renames registrar fixtures. |
| src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py | Standardizes method strings and iteration setter names; enforces uppercase method validation. |
| src/physiomotion4d/workflow_fit_statistical_model_to_patient.py | Renames internal registrar attributes/parameters to *_ANTS/*_ICON and use_ICON_*. |
| src/physiomotion4d/workflow_fine_tune_icon_registration.py | New workflow implementing file-based fine-tuning + in-memory apply path. |
| src/physiomotion4d/workflow_create_statistical_model.py | Updates RegisterModelsDistanceMaps.register(... use_ICON=...) callsite. |
| src/physiomotion4d/workflow_convert_image_to_usd.py | Updates ANTs registrar class name to RegisterImagesANTS. |
| src/physiomotion4d/segment_heart_simpleware.py | Refactors mask handling and adds orientation/landmark flipping logic (includes new debug prints). |
| src/physiomotion4d/register_time_series_images.py | Standardizes registration method handling, registrar attribute names, and iteration setters. |
| src/physiomotion4d/register_models_distance_maps.py | Renames ANTs/ICON registrar attributes and use_ICON parameter; updates examples. |
| src/physiomotion4d/register_images_icon.py | Adds RegisterImagesICON.create_mask and removes the prior in-class finetune method. |
| src/physiomotion4d/register_images_ants.py | Renames class to RegisterImagesANTS and adjusts helper method names to *_ANTS*. |
| src/physiomotion4d/landmark_tools.py | New LandmarkTools module for reading/writing landmarks (3D Slicer Markups JSON + CSV). |
| src/physiomotion4d/cli/reconstruct_highres_4d_ct.py | Updates CLI choices and flag names to uppercase variants (examples partially inconsistent). |
| src/physiomotion4d/cli/fit_statistical_model_to_patient.py | Renames CLI flag to --use-ICON-refinement and updates parameter plumbing. |
| src/physiomotion4d/init.py | Exports LandmarkTools and WorkflowFineTuneICONRegistration; renames ANTs registrar export. |
| README.md | Updates public API docs and testing commands to new RegisterImagesANTS naming and method strings. |
| pyproject.toml | Updates keywords casing and mypy ignore list to include yaml. |
| experiments/Reconstruct4DCT/reconstruct_4d_ct.py | Updates ANTs registrar naming and output filename tokens to ANTS. |
| experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py | Updates registration method strings and iteration setters to uppercase naming. |
| experiments/LongitudinalRegistration/setup.sh | Adds setup script to clone uniGradICON and install dependencies. |
| experiments/LongitudinalRegistration/experiment_recon_4d.py | Adds reconstructed 4D experiment script using new registration method strings. |
| experiments/LongitudinalRegistration/3-run_registration_method_comparison.py | Adds full longitudinal method comparison script (landmarks + optional resegmentation). |
| experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py | Adds script to compare default vs finetuned ICON weights with landmark error metrics. |
| experiments/LongitudinalRegistration/1-finetune_icon.py | Adds script that builds train cohort and runs new fine-tuning workflow. |
| experiments/LongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.py | Adds script to segment and write labelmaps + landmarks for the Duke gated CT cohort. |
| experiments/LongitudinalRegistration/.gitignore | Ignores local uniGradICON checkout folder. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.py | Renames call arg to use_ICON_refinement. |
| experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py | Adds file chooser UX (tkinter) and writes landmarks via LandmarkTools. |
| experiments/Heart-GatedCT_To_USD/test_compare_registration_speed.py | Updates ANTs registrar naming. |
| experiments/Heart-GatedCT_To_USD/1-register_images.py | Updates ANTs registrar naming. |
| experiments/Heart-Create_Statistical_Model/3-registration_based_correspondence.py | Renames use_icon references to use_ICON in examples/comments. |
| docs/tutorials.rst | Updates docs to use ANTS_ICON method string. |
| docs/examples.rst | Updates examples to use ANTS and RegisterImagesANTS. |
| docs/developer/registration_images.rst | Updates developer docs for RegisterImagesANTS and registration_method="ANTS". |
| docs/conf.py | Updates mocked imports list (currently includes ANTS). |
| docs/cli_scripts/fit_statistical_model_to_patient.rst | Updates CLI docs to --use-ICON-refinement. |
| docs/cli_scripts/4dct_reconstruction.rst | Updates CLI docs flag --ANTS-iterations (method example still lowercase). |
| docs/architecture.rst | Updates component naming to RegisterImagesANTS. |
| docs/api/workflows.rst | Updates workflow API examples to registration_method="ANTS". |
| docs/api/registration/time_series.rst | Updates time series docs to ANTS and ANTS_ICON. |
| docs/api/registration/index.rst | Updates docs to refer to RegisterImagesANTS. |
| docs/api/registration/ants.rst | Renames ANTs registration class references to RegisterImagesANTS. |
| docs/api/index.rst | Updates API index to point to RegisterImagesANTS. |
| docs/API_MAP.md | Updates generated API map for new/renamed classes/functions. |
| .gitignore | Adds *.raw ignore pattern. |
| .github/workflows/ci.yml | Updates comments referencing renamed test module names. |
Comments suppressed due to low confidence (1)
src/physiomotion4d/register_time_series_images.py:121
registration_methodis now case-sensitive (ANTS/ICON/ANTS_ICON), and the prior normalization (.lower()) was removed. This is a breaking API change: callers passing the previously accepted values ('ants','icon','ants_icon', etc.) will now raiseValueError. Consider normalizing input (e.g., map legacy lowercase names to the canonical forms) or accepting both variants inREGISTRATION_METHODSto preserve backwards compatibility.
💡 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 #62 +/- ##
==========================================
+ Coverage 31.11% 32.60% +1.48%
==========================================
Files 50 52 +2
Lines 6861 7156 +295
==========================================
+ Hits 2135 2333 +198
- Misses 4726 4823 +97
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (2)
experiments/LongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.py (1)
43-43: ⚡ Quick winReplace
print()with structured logging.Use the logging module for runtime status instead of
print()so experiment logs are consistent and controllable.As per coding guidelines, "Use logging module instead of print statements in Python code".Suggested refactor
+import logging import os from pathlib import Path @@ +logger = logging.getLogger(__name__) @@ -print(f"Found {len(ref_files)} reference images") +logger.info('Found %d reference images', len(ref_files)) @@ -print(f"Found {len(src_data_dirs)} source data directories") +logger.info('Found %d source data directories', len(src_data_dirs)) for d, fs in zip(src_data_dirs, src_data_files, strict=True): - print(f" {d}: {len(fs)} files") + logger.debug('%s: %d files', d, len(fs)) @@ - print(f"Segmenting {src_dir}...") + logger.info('Segmenting %s...', src_dir) @@ - print(f" Segmenting {f}...") + logger.debug('Segmenting %s...', f) @@ -print("\nSegmenting images...") +logger.info('Segmenting images...') image_to_labelmap = segment_images(src_data_dirs, src_data_files) -print(f"Segmentation complete. {len(image_to_labelmap)} labelmaps available.\n") +logger.info('Segmentation complete. %d labelmaps available.', len(image_to_labelmap))Also applies to: 55-57, 84-84, 89-89, 112-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/LongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.py` at line 43, Replace all direct print(...) calls with the logging module: import logging at the top, configure a logger (e.g., logger = logging.getLogger(__name__)), and change print(f"Found {len(ref_files)} reference images") to logger.info("Found %d reference images", len(ref_files)); similarly update the other print calls (the ones that print status and counts around ref_files and landmark/segment status) to use logger.info/debug/warning as appropriate and use structured formatting (logger.info(msg, vars) or logger.debug) instead of f-strings so logs are consistent and configurable.docs/API_MAP.md (1)
413-432: ⚡ Quick winExclude
results/artifacts from the generated API map.
results/...entries are run artifacts and should not be part of the public API index. Please exclude that tree inutils/generate_api_map.pyand regenerate this file to keep API docs stable.🤖 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_MAP.md` around lines 413 - 432, The API map includes run artifacts under results/, so update utils/generate_api_map.py to exclude any file tree whose path starts with "results/" (or add "results" to the excluded directories list used when walking the codebase) before collecting symbols; ensure the filter is applied where files are discovered (e.g., in the directory-walking or glob logic) so symbols like loss_to_dict/augment/finetune_multi from results/* are never indexed, then re-run the generator to regenerate docs/API_MAP.md without the results/ entries.
🤖 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 @.github/workflows/ci.yml:
- Around line 410-411: The excluded-test example filenames in the CI config use
uppercase names (tests/test_register_images_ANTS.py and
tests/test_register_images_ICON.py) that don't match the actual lowercase test
files; update these occurrences (including the additional occurrence noted
around the later reference) to the real lowercase filenames
(tests/test_register_images_ants.py and tests/test_register_images_icon.py) so
examples match repository paths and won't mislead or break copy/paste.
In `@experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py`:
- Around line 27-28: The unconditional top-level imports "import tkinter as tk"
and "from tkinter import filedialog" can raise ImportError in headless/minimal
environments; move these imports into the branch that actually uses them (the
TestTools.running_as_test() guarded interactive/file dialog path) or perform a
lazy import inside the function that calls filedialog, and wrap it in a
try/except ImportError to provide a fallback (e.g., skip the dialog, log a clear
message, or raise a controlled error). Ensure you reference and update the code
that calls filedialog to import tkinter/filedialog at use-time rather than at
module import-time.
In
`@experiments/LongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.py`:
- Around line 16-17: The doc comment listing "<stem>_landmark.csv" is
inconsistent with the actual write call that produces "<stem>_landmark.mrk.json"
(search for the literal ".mrk.json" in the file); update the documentation line
to match the real output filename or change the code that writes the landmarks
to emit ".csv" instead so they match—ensure the symbol references (the comment
entry "<stem>_landmark.csv" and the write call producing ".mrk.json") are
synchronized and use descriptive names for any variables created for the
landmark filename.
In `@experiments/LongitudinalRegistration/1-finetune_icon.py`:
- Line 50: The variable annotation for unigradicon_src_path uses "Path | None";
change it to use Optional[Path] per repo typing rules by updating the
declaration of unigradicon_src_path to use Optional[Path] and ensure Optional is
imported from typing (or added to existing typing imports) while keeping the
assignment to Path(__file__).parent / "uniGradICON" / "src" intact.
In `@experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py`:
- Line 116: The list comprehension assigning timepoints uses
timepoint_re.search(p.name).group("timepoint") which will raise AttributeError
if a filename in image_files doesn't match the regex; change it to handle
non-matches defensively by checking the match object before calling .group
(e.g., for each p in image_files call m = timepoint_re.search(p.name) and if m
is None either skip the file, raise a clear ValueError mentioning p.name, or
provide a default), updating the variable assignment at the timepoints creation
site to use that safe logic and ensure downstream code expects the possibly
filtered list or raised error.
- Around line 224-227: The current write block assumes summary_rows has at least
one element and uses summary_rows[0].keys(), which will raise IndexError when no
rows exist; update the logic around summary_file, summary_rows and the
csv.DictWriter call so you only access summary_rows[0] when summary_rows is
non-empty (e.g., if summary_rows: open file, create DictWriter using
list(summary_rows[0].keys()), write header and rows; else either skip file
creation or write an empty file with known fieldnames), making the guard around
the writer creation and writerows to avoid indexing an empty list.
In
`@experiments/LongitudinalRegistration/3-run_registration_method_comparison.py`:
- Around line 602-604: The iteration flags were added without explicit dest
names so argparse created attributes like ANTS_iterations and ICON_iterations,
but the code expects snake_case attributes (args.ants_iterations and
args.icon_iterations) when calling parse_iterations and elsewhere; update the
parser.add_argument calls for "--ANTS-iterations", "--greedy-iterations", and
"--ICON-iterations" to include dest="ants_iterations", dest="greedy_iterations",
and dest="icon_iterations" respectively so the parsed Namespace provides the
expected attributes used by parse_iterations and the rest of the script.
In `@experiments/LongitudinalRegistration/experiment_recon_4d.py`:
- Line 46: The print statement accesses ref_files[0] without checking for an
empty list; change it to first verify ref_files is non-empty (e.g., if not
ref_files: handle or log and return/continue) before slicing basename, so guard
the access to ref_files[0] and avoid IndexError when no reference images are
found (update the logic around the print call that uses
os.path.basename(ref_files[0])[:6]).
- Around line 105-109: The reference matching fails because reference_image_file
is a full path while source_image_files contains only basenames, so the if check
never succeeds and reference_image_num stays at 7 (possibly out of range); fix
by comparing normalized names (e.g., use os.path.basename(reference_image_file)
or os.path.normpath/os.path.abspath on both) when testing membership and
obtaining the index for reference_image_num, and after computing the index
ensure reference_image_num is within range (0..len(source_image_files)-1) and
set register_start_to_reference accordingly (use reference_image_num =
source_image_files.index(basename) and if index >= len(...) clamp to last
index).
In `@experiments/LongitudinalRegistration/setup.sh`:
- Around line 20-24: The venv interpreter check uses "venv/Scripts/python" which
is wrong on Windows; update the detection in the setup script to test for the
Windows executables (e.g., "venv/Scripts/python.exe" and optionally
"venv/Scripts/python3.exe") before falling back to the Unix path, and set PYTHON
to the first existing interpreter found (e.g., check -f
"venv/Scripts/python.exe" || -f "venv/Scripts/python3.exe" then set
PYTHON="venv/Scripts/python.exe" or the matching file; otherwise keep
PYTHON="venv/bin/python").
- Line 28: The script runs "$PYTHON" -m uv pip install -e . inside
experiments/LongitudinalRegistration which tries to install an editable package
from that subdirectory (likely not an installable project); change the install
target so the repo's main package is installed instead — either run the editable
install from the repository root or invoke pip with the correct path (e.g., -e
.. or the repo root package path) so the main package is installed; update the
setup.sh command that currently references "$PYTHON" -m uv pip install -e . to
point to the correct install location.
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct.py`:
- Line 35: The bug is a casing mismatch between the registration prefix used
when writing and when reading transforms—reg_method_data is initialized with
"ANTs" (and filenames emitted as slice_ANTs_*) while the reload stage expects
"ANTS" (slice_ANTS_*), causing reloads to fail; fix by making the prefix
consistent across write and read paths (either change the reg_method_data entry
from "ANTs" to "ANTS" or update the reload/read code to expect "ANTs")—search
for reg_method_data, RegisterImagesANTS, and the filename patterns "slice_ANTs_"
and "slice_ANTS_" and update all occurrences so they use the same exact casing.
In `@src/physiomotion4d/cli/reconstruct_highres_4d_ct.py`:
- Around line 109-116: The CLI args use uppercase option names
"--ANTS-iterations" and "--ICON-iterations" so argparse will create
args.ANTS_iterations and args.ICON_iterations, breaking code that expects
args.ants_iterations and args.icon_iterations; update the parser.add_argument
calls for "--ANTS-iterations" and "--ICON-iterations" to include explicit
dest="ants_iterations" and dest="icon_iterations" respectively (keep existing
nargs/type/help settings) so subsequent accesses like args.ants_iterations and
args.icon_iterations succeed.
In `@src/physiomotion4d/landmark_tools.py`:
- Around line 51-57: The __init__ method on LandmarkTools lacks an explicit
return annotation; update the LandmarkTools.__init__ signature to include "->
None" (i.e., def __init__(self, log_level: int | str = logging.INFO) -> None:)
so it complies with the repository mypy rule disallow_untyped_defs; ensure you
modify the method declaration only (in the LandmarkTools class) and run type
checks to confirm the fix.
In `@src/physiomotion4d/register_models_distance_maps.py`:
- Line 40: The example calls to registrar.register() use incorrect keyword
names; update the example kwargs to match the register() signature by replacing
mode with transform_type and use_ANTS (or use_ICON inconsistency) with use_ICON
so the call reads something like registrar.register(transform_type='deformable',
use_ICON=True, icon_iterations=50); apply the same change for the other example
instance that mirrors this call (the one referenced around the second example)
and ensure all keyword names exactly match the register() parameters
(transform_type, use_ICON, icon_iterations).
In `@src/physiomotion4d/segment_heart_simpleware.py`:
- Around line 372-377: The loop that adjusts point signs currently negates
point[i] for i<2 when in_direction[i,i] >= 0, which combined with the later
unconditional point[0] = -point[0]; point[1] = -point[1] double-negates X/Y;
modify the loop in the function using in_direction and point so it only negates
when in_direction[i,i] < 0 (e.g., change the branch that does point[i] =
-point[i] to check in_direction[i,i] < 0), leaving the later unconditional
RAS→LPS conversion of point[0] and point[1] intact.
- Around line 350-353: The computed physical-edge coordinate uses
mask_image.TransformIndexToPhysicalPoint(mask_image.GetLargestPossibleRegion().GetSize()),
which is off-by-one for ITK indices; call TransformIndexToPhysicalPoint with the
region size minus one (size-1) for each dimension when constructing edge so the
z-axis reflection (in_direction[2,2] < 0) uses the correct boundary. Also fix
the landmark x/y negation: remove the unconditional point[0] = -point[0] and
point[1] = -point[1] and instead negate each coordinate only when its
corresponding in_direction[i,i] indicates a sign flip (keep the existing
conditional block that does elif i < 2: point[i] = -point[i] when
in_direction[i,i] >= 0, or invert that logic if intended), ensuring negation
happens exactly once; refer to mask_image.TransformIndexToPhysicalPoint,
mask_image.GetLargestPossibleRegion().GetSize(), in_direction, and the point
array handling in the loop to locate and correct the code.
In `@src/physiomotion4d/workflow_fine_tune_icon_registration.py`:
- Around line 412-490: The code currently continues to write the dataset JSON
and launch fine-tuning even if dataset_entries is empty; add a fail-fast check
after building dataset_entries and before writing dataset_json_path: if
len(dataset_entries) == 0, call self.log_error (or raise a
RuntimeError/ValueError) with a clear message including self.fine_tune_name and
abort (raise an exception) so downstream steps are not run on an empty dataset;
reference the local variable dataset_entries, the dataset_json_path write block,
and the attribute self._dataset_json_path to locate where to insert the check.
In `@tests/README.md`:
- Around line 188-189: Update the dependency diagram entries that reference
test_register_images_ANTS and test_register_images_ICON to use the actual
lowercase filenames (test_register_images_ants and test_register_images_icon) so
the diagram matches the real test file names; locate the lines that contain
"test_register_images_ANTS" and "test_register_images_ICON" in the README
diagram and replace them with "test_register_images_ants" and
"test_register_images_icon" respectively to ensure case-sensitive navigation
works.
In `@tests/test_register_images_greedy.py`:
- Line 5: Update the module docstring in test_register_images_greedy.py to
reference the correct lowercase filename "test_register_images_ants.py" instead
of "test_register_images_ANTS"; locate the docstring at the top of the file and
change the referenced module name, and while here ensure the module/test
function names (e.g., any top-level string mentioning test_register_images_ANTS
or functions like test_register_images_greedy) use meaningful, lowercase names
consistent with the repo convention.
---
Nitpick comments:
In `@docs/API_MAP.md`:
- Around line 413-432: The API map includes run artifacts under results/, so
update utils/generate_api_map.py to exclude any file tree whose path starts with
"results/" (or add "results" to the excluded directories list used when walking
the codebase) before collecting symbols; ensure the filter is applied where
files are discovered (e.g., in the directory-walking or glob logic) so symbols
like loss_to_dict/augment/finetune_multi from results/* are never indexed, then
re-run the generator to regenerate docs/API_MAP.md without the results/ entries.
In
`@experiments/LongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.py`:
- Line 43: Replace all direct print(...) calls with the logging module: import
logging at the top, configure a logger (e.g., logger =
logging.getLogger(__name__)), and change print(f"Found {len(ref_files)}
reference images") to logger.info("Found %d reference images", len(ref_files));
similarly update the other print calls (the ones that print status and counts
around ref_files and landmark/segment status) to use logger.info/debug/warning
as appropriate and use structured formatting (logger.info(msg, vars) or
logger.debug) instead of f-strings so logs are consistent and configurable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04d7ae61-6043-4a50-b191-3712e4b9f0fa
📒 Files selected for processing (55)
.github/workflows/ci.yml.gitignoreREADME.mddocs/API_MAP.mddocs/api/index.rstdocs/api/registration/ants.rstdocs/api/registration/index.rstdocs/api/registration/time_series.rstdocs/api/workflows.rstdocs/architecture.rstdocs/cli_scripts/4dct_reconstruction.rstdocs/cli_scripts/fit_statistical_model_to_patient.rstdocs/developer/registration_images.rstdocs/examples.rstdocs/tutorials.rstexperiments/Heart-Create_Statistical_Model/3-registration_based_correspondence.pyexperiments/Heart-GatedCT_To_USD/1-register_images.pyexperiments/Heart-GatedCT_To_USD/test_compare_registration_speed.pyexperiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.pyexperiments/LongitudinalRegistration/.gitignoreexperiments/LongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.pyexperiments/LongitudinalRegistration/1-finetune_icon.pyexperiments/LongitudinalRegistration/2-recon_4d_icon_eval.pyexperiments/LongitudinalRegistration/3-run_registration_method_comparison.pyexperiments/LongitudinalRegistration/experiment_recon_4d.pyexperiments/LongitudinalRegistration/setup.shexperiments/Reconstruct4DCT/reconstruct_4d_ct.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct_class.pypyproject.tomlsrc/physiomotion4d/__init__.pysrc/physiomotion4d/cli/fit_statistical_model_to_patient.pysrc/physiomotion4d/cli/reconstruct_highres_4d_ct.pysrc/physiomotion4d/landmark_tools.pysrc/physiomotion4d/register_images_ants.pysrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/register_models_distance_maps.pysrc/physiomotion4d/register_time_series_images.pysrc/physiomotion4d/segment_heart_simpleware.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_create_statistical_model.pysrc/physiomotion4d/workflow_fine_tune_icon_registration.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.pysrc/physiomotion4d/workflow_reconstruct_highres_4d_ct.pytests/README.mdtests/conftest.pytests/test_register_images_ants.pytests/test_register_images_greedy.pytests/test_register_images_icon.pytests/test_register_time_series_images.pytests/test_transform_tools.pytests/test_workflow_fine_tune_icon_registration.pytutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_06_reconstruct_highres_4d_ct.pytutorials/tutorial_08_dirlab_pca_time_series.py
Comment on lines
+410
to
+411
| # - tests/test_register_images_ANTS.py (slow, computationally intensive) | ||
| # - tests/test_register_images_ICON.py (requires CUDA for ICON) |
There was a problem hiding this comment.
Fix excluded-test note filenames to match real paths.
Line 410, Line 411, and Line 424 reference uppercase test filenames. The repository test files are lowercase, so these examples are misleading and can fail when copied.
Suggested fix
-# - tests/test_register_images_ANTS.py (slow, computationally intensive)
-# - tests/test_register_images_ICON.py (requires CUDA for ICON)
+# - tests/test_register_images_ants.py (slow, computationally intensive)
+# - tests/test_register_images_icon.py (requires CUDA for ICON)
...
-# pytest tests/test_register_images_ANTS.py -v --run-slow
+# pytest tests/test_register_images_ants.py -v --run-slowAlso applies to: 424-424
🤖 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 @.github/workflows/ci.yml around lines 410 - 411, The excluded-test example
filenames in the CI config use uppercase names
(tests/test_register_images_ANTS.py and tests/test_register_images_ICON.py) that
don't match the actual lowercase test files; update these occurrences (including
the additional occurrence noted around the later reference) to the real
lowercase filenames (tests/test_register_images_ants.py and
tests/test_register_images_icon.py) so examples match repository paths and won't
mislead or break copy/paste.
Comment on lines
+27
to
+28
| import tkinter as tk | ||
| from tkinter import filedialog |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py"
echo "== File head (first 120 lines) =="
nl -ba "$FILE" | sed -n '1,160p'
echo
echo "== Search for tkinter usage =="
rg -n "tkinter|filedialog|tk\." "$FILE" || true
echo
echo "== Check for CLI / guard conditions =="
rg -n "__main__|if .*dialog|file path|predefined|args|argparse" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py"
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE" >&2
exit 1
fi
echo "== File head (first 220 lines) =="
nl -ba "$FILE" | sed -n '1,220p'
echo
echo "== Search for tkinter usage =="
rg -n "tkinter|filedialog|tk\." "$FILE" || true
echo
echo "== Check for CLI / guard conditions =="
rg -n "__main__|if .*dialog|file path|predefined|args|argparse" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py"
nl -ba "$FILE" | sed -n '1,220p'
rg -n "tkinter|filedialog|tk\." "$FILE" || true
rg -n "__main__|if .*dialog|file path|predefined|args|argparse" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py"
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE" >&2
exit 1
fi
echo "== File head (first 240 lines) =="
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,240p'
echo
echo "== Search for tkinter usage =="
rg -n "tkinter|filedialog|tk\." "$FILE" || true
echo
echo "== Check for CLI / guard conditions =="
rg -n "__main__|if .*dialog|file path|predefined|args|argparse" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 10334
Lazy-import tkinter to prevent headless/minimal environments from failing at module import time.
tkinter is imported unconditionally (lines 27-28), so ImportError can occur even though the interactive filedialog branch is guarded by TestTools.running_as_test().
Suggested fix
-import tkinter as tk
-from tkinter import filedialog
@@
else:
- root = tk.Tk()
+ try:
+ import tkinter as tk
+ from tkinter import filedialog
+ except ImportError as e:
+ raise RuntimeError(
+ 'tkinter is required for interactive file selection. '
+ 'Install Tk support or run with a predefined input path.'
+ ) from e
+ root = tk.Tk()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.py`
around lines 27 - 28, The unconditional top-level imports "import tkinter as tk"
and "from tkinter import filedialog" can raise ImportError in headless/minimal
environments; move these imports into the branch that actually uses them (the
TestTools.running_as_test() guarded interactive/file dialog path) or perform a
lazy import inside the function that calls filedialog, and wrap it in a
try/except ImportError to provide a fallback (e.g., skip the dialog, log a clear
message, or raise a controlled error). Ensure you reference and update the code
that calls filedialog to import tkinter/filedialog at use-time rather than at
module import-time.
Comment on lines
+16
to
+17
| # - `<stem>_labelmap.nii.gz` | ||
| # - `<stem>_landmark.csv` |
There was a problem hiding this comment.
Synchronize documented landmark filename with actual output.
Lines 16–17 say landmarks are written as .csv, but Line 91 writes .mrk.json. Update either the comment or the output filename so users don’t look for the wrong artifact.
Suggested fix (doc text)
-- `<stem>_landmark.csv`
+- `<stem>_landmark.mrk.json`Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@experiments/LongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.py`
around lines 16 - 17, The doc comment listing "<stem>_landmark.csv" is
inconsistent with the actual write call that produces "<stem>_landmark.mrk.json"
(search for the literal ".mrk.json" in the file); update the documentation line
to match the real output filename or change the code that writes the landmarks
to emit ".csv" instead so they match—ensure the symbol references (the comment
entry "<stem>_landmark.csv" and the write call producing ".mrk.json") are
synchronized and use descriptive names for any variables created for the
landmark filename.
| # Local clone of uniGradICON (feat-add-finetuning branch) — prepended to | ||
| # PYTHONPATH so the subprocess picks up the local source instead of the | ||
| # installed package. Set to ``None`` to use the pip-installed unigradicon. | ||
| unigradicon_src_path: Path | None = Path(__file__).parent / "uniGradICON" / "src" |
There was a problem hiding this comment.
Use Optional[Path] instead of Path | None for type hints.
Please align this annotation with repository typing rules.
Suggested fix
+from typing import Optional
@@
-unigradicon_src_path: Path | None = Path(__file__).parent / "uniGradICON" / "src"
+unigradicon_src_path: Optional[Path] = (
+ Path(__file__).parent / "uniGradICON" / "src"
+)As per coding guidelines, "Use Optional[X] instead of X | None for type hints (ruff UP007 suppressed)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| unigradicon_src_path: Path | None = Path(__file__).parent / "uniGradICON" / "src" | |
| from typing import Optional | |
| unigradicon_src_path: Optional[Path] = ( | |
| Path(__file__).parent / "uniGradICON" / "src" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/LongitudinalRegistration/1-finetune_icon.py` at line 50, The
variable annotation for unigradicon_src_path uses "Path | None"; change it to
use Optional[Path] per repo typing rules by updating the declaration of
unigradicon_src_path to use Optional[Path] and ensure Optional is imported from
typing (or added to existing typing imports) while keeping the assignment to
Path(__file__).parent / "uniGradICON" / "src" intact.
| stems = [p.name[:-7] for p in image_files] | ||
| labelmap_files = [seg_dir / f"{s}_labelmap.nii.gz" for s in stems] | ||
| landmark_files = [seg_dir / f"{s}_landmark.mrk.json" for s in stems] | ||
| timepoints = [timepoint_re.search(p.name).group("timepoint") for p in image_files] |
There was a problem hiding this comment.
Handle unmatched timepoint filenames defensively.
timepoint_re.search(...).group(...) can crash with AttributeError when a filename does not match _gNNN.
Proposed fix
- timepoints = [timepoint_re.search(p.name).group("timepoint") for p in image_files]
+ timepoints: list[str] = []
+ for p in image_files:
+ match = timepoint_re.search(p.name)
+ if match is None:
+ raise ValueError(f'Could not extract timepoint from filename: {p.name}')
+ timepoints.append(match.group('timepoint'))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/LongitudinalRegistration/2-recon_4d_icon_eval.py` at line 116,
The list comprehension assigning timepoints uses
timepoint_re.search(p.name).group("timepoint") which will raise AttributeError
if a filename in image_files doesn't match the regex; change it to handle
non-matches defensively by checking the match object before calling .group
(e.g., for each p in image_files call m = timepoint_re.search(p.name) and if m
is None either skip the file, raise a clear ValueError mentioning p.name, or
provide a default), updating the variable assignment at the timepoints creation
site to use that safe logic and ensure downstream code expects the possibly
filtered list or raised error.
Comment on lines
+350
to
+353
| edge = np.array( | ||
| mask_image.TransformIndexToPhysicalPoint( | ||
| mask_image.GetLargestPossibleRegion().GetSize() | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
# Show the exact lines around the reported snippet and the surrounding logic
nl -ba "$FILE" | sed -n '320,410p'
# Narrow search for any use of GetSize() / TransformIndexToPhysicalPoint() in the file
rg -n "TransformIndexToPhysicalPoint|GetLargestPossibleRegion\\(\\)\\.GetSize\\(\\)" "$FILE"
# Search for "negate" patterns / conditional flips / RAS<->LPS mentions
rg -n "negat|LPS|RAS|flip|landmark|reflection|TransformIndexToPhysicalPoint" "$FILE"Repository: Project-MONAI/physiomotion4d
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
# Show the exact lines around the reported snippet and surrounding logic (using awk instead of nl)
awk 'NR>=320 && NR<=410 {printf "%d\t%s\n", NR, $0}' "$FILE"
# Narrow search for ITK usage patterns in the file
rg -n "TransformIndexToPhysicalPoint|GetLargestPossibleRegion\\(\\)\\.GetSize\\(\\)" "$FILE" || true
# Search for any coordinate conversion/flip/remapping cues
rg -n "negat|LPS|RAS|flip|landmark|reflection" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 6293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
# Show the section where landmarks.csv is read and any comments about coordinate frames
awk 'NR>=240 && NR<=330 {printf "%d\t%s\n", NR, $0}' "$FILE"
# Also show the file header imports for any project-level coordinate convention references
awk 'NR<=120 {printf "%d\t%s\n", NR, $0}' "$FILE"
# Search the file for "ras to lps" and "landmarks.csv" references for context
rg -n "ras to lps|RAS|LPS|landmarks\\.csv|landmark" "$FILE"Repository: Project-MONAI/physiomotion4d
Length of output: 11136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# File currently inspected
FILE="src/physiomotion4d/segment_heart_simpleware.py"
# 1) Find all uses of TransformIndexToPhysicalPoint in the repo and inspect nearby context.
rg -n "TransformIndexToPhysicalPoint\\(" -S .
# 2) Specifically find any GetLargestPossibleRegion().GetSize() passed into TransformIndexToPhysicalPoint.
rg -n "TransformIndexToPhysicalPoint\\([^\\n]*GetLargestPossibleRegion\\(\\)\\.GetSize\\(\\)" -S .
# 3) Inspect if in_direction/out_direction are used elsewhere in this file.
rg -n "in_direction|out_direction|point\\[i\\] = -point\\[i\\]|edge\\[i\\]" "$FILE"
# 4) Show a bit more context around the loop computing landmark point reflections.
awk 'NR>=330 && NR<=390 {printf "%d\t%s\n", NR, $0}' "$FILE"Repository: Project-MONAI/physiomotion4d
Length of output: 451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/transform_tools.py"
awk 'NR>=940 && NR<=1045 {printf "%d\t%s\n", NR, $0}' "$FILE"
awk 'NR>=1080 && NR<=1165 {printf "%d\t%s\n", NR, $0}' "$FILE"Repository: Project-MONAI/physiomotion4d
Length of output: 8456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/transform_tools.py"
# Continue inspection around the bounds checking in _trace_streamline
awk 'NR>=1140 && NR<=1235 {printf "%d\t%s\n", NR, $0}' "$FILE"
# Search within transform_tools.py for "size - 1" and for "GetSize()" bounds comparisons
rg -n "size\\s*-\\s*1|GetLargestPossibleRegion\\(\\)\\.GetSize\\(\\)" "$FILE"Repository: Project-MONAI/physiomotion4d
Length of output: 3952
Fix off-by-one and double-negation in landmark coordinate handling
-
edgeis computed viamask_image.TransformIndexToPhysicalPoint(mask_image.GetLargestPossibleRegion().GetSize()), but ITK indices are valid only up tosize - 1; passingsizeshifts the physical coordinate used for the z-axis reflection (whenin_direction[2, 2] < 0).Suggested fix
-
edge = np.array( -
mask_image.TransformIndexToPhysicalPoint( -
mask_image.GetLargestPossibleRegion().GetSize() -
) -
)
-
size = np.array(mask_image.GetLargestPossibleRegion().GetSize()) -
last_index = (size - 1).tolist() -
edge = np.array(mask_image.TransformIndexToPhysicalPoint(last_index))</details>
- Landmark
x/yare negated inside the loop (elif i < 2: point[i] = -point[i]whenin_direction[i, i] >= 0) and then negated again unconditionally (point[0] = -point[0],point[1] = -point[1]). For the common case of positive directions, this cancels the intended RAS→LPS conversion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/physiomotion4d/segment_heart_simpleware.py` around lines 350 - 353, The
computed physical-edge coordinate uses
mask_image.TransformIndexToPhysicalPoint(mask_image.GetLargestPossibleRegion().GetSize()),
which is off-by-one for ITK indices; call TransformIndexToPhysicalPoint with the
region size minus one (size-1) for each dimension when constructing edge so the
z-axis reflection (in_direction[2,2] < 0) uses the correct boundary. Also fix
the landmark x/y negation: remove the unconditional point[0] = -point[0] and
point[1] = -point[1] and instead negate each coordinate only when its
corresponding in_direction[i,i] indicates a sign flip (keep the existing
conditional block that does elif i < 2: point[i] = -point[i] when
in_direction[i,i] >= 0, or invert that logic if intended), ensuring negation
happens exactly once; refer to mask_image.TransformIndexToPhysicalPoint,
mask_image.GetLargestPossibleRegion().GetSize(), in_direction, and the point
array handling in the loop to locate and correct the code.
Comment on lines
+372
to
+377
| elif i < 2: | ||
| point[i] = -point[i] | ||
| self.log_debug(f" New point {point}") | ||
| # convert ras to lps as used by this project | ||
| point[0] = -point[0] | ||
| point[1] = -point[1] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
# Show surrounding lines around 350-390
nl -ba "$FILE" | sed -n '330,410p'
# Search within file for "ras" or "lps" or "point[0]" occurrences
rg -n "ras|lps|Convert|point\[0\]|point\[1\]" "$FILE"
# Show entire function or loop if smaller: try to extract nearest def/class
# Grab function boundaries around the snippet region
# Use awk to find nearest "def " above and below within some range
python3 - <<'PY'
import re, itertools, os
file_path="src/physiomotion4d/segment_heart_simpleware.py"
with open(file_path,'r',encoding='utf-8') as f:
lines=f.readlines()
start=330
end=410
for i in range(start-1, end):
if i<0 or i>=len(lines): continue
if re.match(r'^\s*def\s', lines[i]):
defline=i+1
# find next def or class after this
break
else:
defline=None
print("First def before/within region:", defline)
PYRepository: Project-MONAI/physiomotion4d
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
# Show a wider range to see loop and i usage
nl -ba "$FILE" | sed -n '300,430p'Repository: Project-MONAI/physiomotion4d
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
# Also find occurrences of "elif i < 2" in repo
rg -n "elif i < 2" -S .Repository: Project-MONAI/physiomotion4d
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "segment_heart_simpleware" -S src/physiomotion4d || trueRepository: Project-MONAI/physiomotion4d
Length of output: 1069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -laRepository: Project-MONAI/physiomotion4d
Length of output: 1721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
echo "=== Lines 300-430 ==="
nl -ba "$FILE" | sed -n '300,430p'
echo
echo "=== RG ras/lps/point[0]/point[1] ==="
rg -n "ras|lps|Convert|point\[0\]|point\[1\]" "$FILE" || true
echo
echo "=== RG 'elif i < 2' across repo ==="
rg -n "elif i < 2" -S . || trueRepository: Project-MONAI/physiomotion4d
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_heart_simpleware.py"
echo "=== Excerpt lines 350-390 (no nl available) ==="
sed -n '350,390p' "$FILE"
echo
echo "=== Lines containing ras/lps/convert/point[0]/point[1] in this file ==="
rg -n "ras|lps|Convert|convert ras|point\[0\]|point\[1\]" "$FILE" || true
echo
echo "=== Lines containing 'elif i < 2' in this file (with a bit of context) ==="
rg -n -C 5 "elif i < 2" "$FILE" || trueRepository: Project-MONAI/physiomotion4d
Length of output: 3174
Avoid double-negating X/Y when converting RAS→LPS
When in_direction[i, i] >= 0 for i < 2, the loop applies point[i] = -point[i] (line 373) and then the code unconditionally negates X/Y again (lines 376-377). This makes X/Y effectively unchanged instead of converting RAS→LPS (the in_direction[i, i] < 0 branch uses a different formula).
Suggested fix
- elif i < 2:
- point[i] = -point[i]
+ # keep orientation-handling here only
@@
- # convert ras to lps as used by this project
+ # Convert RAS -> LPS exactly once.
point[0] = -point[0]
point[1] = -point[1]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/physiomotion4d/segment_heart_simpleware.py` around lines 372 - 377, The
loop that adjusts point signs currently negates point[i] for i<2 when
in_direction[i,i] >= 0, which combined with the later unconditional point[0] =
-point[0]; point[1] = -point[1] double-negates X/Y; modify the loop in the
function using in_direction and point so it only negates when in_direction[i,i]
< 0 (e.g., change the branch that does point[i] = -point[i] to check
in_direction[i,i] < 0), leaving the later unconditional RAS→LPS conversion of
point[0] and point[1] intact.
Comment on lines
+412
to
+490
| dataset_entries: list[dict[str, str]] = [] | ||
| for subject_index, image_files in enumerate(self.subject_image_files): | ||
| subject_id = ( | ||
| self.subject_ids[subject_index] | ||
| if self.subject_ids is not None | ||
| else f"subject_{subject_index:04d}" | ||
| ) | ||
| seg_list = ( | ||
| self.subject_segmentation_files[subject_index] | ||
| if self.subject_segmentation_files is not None | ||
| else [None] * len(image_files) | ||
| ) | ||
| mask_list = ( | ||
| self.subject_mask_files[subject_index] | ||
| if self.subject_mask_files is not None | ||
| else [None] * len(image_files) | ||
| ) | ||
| landmark_list = ( | ||
| self.subject_landmark_files[subject_index] | ||
| if self.subject_landmark_files is not None | ||
| else [None] * len(image_files) | ||
| ) | ||
|
|
||
| for image_file, seg_file, mask_file, landmark_file in zip( | ||
| image_files, seg_list, mask_list, landmark_list, strict=True | ||
| ): | ||
| image_path = Path(image_file) | ||
| if not image_path.exists(): | ||
| raise FileNotFoundError(f"Image not found: {image_path}") | ||
|
|
||
| entry: dict[str, str] = { | ||
| "image": self._posix(image_path), | ||
| "subject_id": subject_id, | ||
| } | ||
|
|
||
| if use_seg: | ||
| if seg_file is None or not Path(seg_file).exists(): | ||
| self.log_warning( | ||
| "Skipping %s: segmentation missing for paired-with-seg " | ||
| "training (seg=%s)", | ||
| image_path, | ||
| seg_file, | ||
| ) | ||
| continue | ||
| entry["segmentation"] = self._posix(seg_file) | ||
|
|
||
| if use_mask: | ||
| if mask_file is not None and Path(mask_file).exists(): | ||
| resolved_mask: Path = Path(mask_file) | ||
| elif seg_file is not None and Path(seg_file).exists(): | ||
| resolved_mask = self._derive_mask(seg_file) | ||
| else: | ||
| self.log_warning( | ||
| "Skipping %s: neither explicit mask nor segmentation " | ||
| "available to derive a loss-function mask " | ||
| "(mask=%s, seg=%s)", | ||
| image_path, | ||
| mask_file, | ||
| seg_file, | ||
| ) | ||
| continue | ||
| entry["mask"] = self._posix(resolved_mask) | ||
|
|
||
| if landmark_file is not None: | ||
| entry["landmarks"] = self._posix(landmark_file) | ||
|
|
||
| dataset_entries.append(entry) | ||
|
|
||
| dataset_json_path = self.experiment_dir / f"{self.fine_tune_name}_dataset.json" | ||
| with dataset_json_path.open("w") as fh: | ||
| json.dump({"data": dataset_entries}, fh, indent=2) | ||
|
|
||
| self.log_info( | ||
| "Wrote dataset JSON %s with %d entries", | ||
| dataset_json_path, | ||
| len(dataset_entries), | ||
| ) | ||
| self._dataset_json_path = dataset_json_path | ||
| return dataset_json_path |
There was a problem hiding this comment.
Fail fast when dataset resolves to zero training entries.
If all frames are filtered out, the workflow still writes JSON/YAML and launches fine-tuning; failure then occurs downstream with a less actionable error.
Suggested fix
dataset_json_path = self.experiment_dir / f"{self.fine_tune_name}_dataset.json"
+ if not dataset_entries:
+ raise ValueError(
+ 'No valid training entries were generated. '
+ 'Check image/segmentation/mask availability.'
+ )
with dataset_json_path.open("w") as fh:
json.dump({"data": dataset_entries}, fh, indent=2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dataset_entries: list[dict[str, str]] = [] | |
| for subject_index, image_files in enumerate(self.subject_image_files): | |
| subject_id = ( | |
| self.subject_ids[subject_index] | |
| if self.subject_ids is not None | |
| else f"subject_{subject_index:04d}" | |
| ) | |
| seg_list = ( | |
| self.subject_segmentation_files[subject_index] | |
| if self.subject_segmentation_files is not None | |
| else [None] * len(image_files) | |
| ) | |
| mask_list = ( | |
| self.subject_mask_files[subject_index] | |
| if self.subject_mask_files is not None | |
| else [None] * len(image_files) | |
| ) | |
| landmark_list = ( | |
| self.subject_landmark_files[subject_index] | |
| if self.subject_landmark_files is not None | |
| else [None] * len(image_files) | |
| ) | |
| for image_file, seg_file, mask_file, landmark_file in zip( | |
| image_files, seg_list, mask_list, landmark_list, strict=True | |
| ): | |
| image_path = Path(image_file) | |
| if not image_path.exists(): | |
| raise FileNotFoundError(f"Image not found: {image_path}") | |
| entry: dict[str, str] = { | |
| "image": self._posix(image_path), | |
| "subject_id": subject_id, | |
| } | |
| if use_seg: | |
| if seg_file is None or not Path(seg_file).exists(): | |
| self.log_warning( | |
| "Skipping %s: segmentation missing for paired-with-seg " | |
| "training (seg=%s)", | |
| image_path, | |
| seg_file, | |
| ) | |
| continue | |
| entry["segmentation"] = self._posix(seg_file) | |
| if use_mask: | |
| if mask_file is not None and Path(mask_file).exists(): | |
| resolved_mask: Path = Path(mask_file) | |
| elif seg_file is not None and Path(seg_file).exists(): | |
| resolved_mask = self._derive_mask(seg_file) | |
| else: | |
| self.log_warning( | |
| "Skipping %s: neither explicit mask nor segmentation " | |
| "available to derive a loss-function mask " | |
| "(mask=%s, seg=%s)", | |
| image_path, | |
| mask_file, | |
| seg_file, | |
| ) | |
| continue | |
| entry["mask"] = self._posix(resolved_mask) | |
| if landmark_file is not None: | |
| entry["landmarks"] = self._posix(landmark_file) | |
| dataset_entries.append(entry) | |
| dataset_json_path = self.experiment_dir / f"{self.fine_tune_name}_dataset.json" | |
| with dataset_json_path.open("w") as fh: | |
| json.dump({"data": dataset_entries}, fh, indent=2) | |
| self.log_info( | |
| "Wrote dataset JSON %s with %d entries", | |
| dataset_json_path, | |
| len(dataset_entries), | |
| ) | |
| self._dataset_json_path = dataset_json_path | |
| return dataset_json_path | |
| dataset_entries: list[dict[str, str]] = [] | |
| for subject_index, image_files in enumerate(self.subject_image_files): | |
| subject_id = ( | |
| self.subject_ids[subject_index] | |
| if self.subject_ids is not None | |
| else f"subject_{subject_index:04d}" | |
| ) | |
| seg_list = ( | |
| self.subject_segmentation_files[subject_index] | |
| if self.subject_segmentation_files is not None | |
| else [None] * len(image_files) | |
| ) | |
| mask_list = ( | |
| self.subject_mask_files[subject_index] | |
| if self.subject_mask_files is not None | |
| else [None] * len(image_files) | |
| ) | |
| landmark_list = ( | |
| self.subject_landmark_files[subject_index] | |
| if self.subject_landmark_files is not None | |
| else [None] * len(image_files) | |
| ) | |
| for image_file, seg_file, mask_file, landmark_file in zip( | |
| image_files, seg_list, mask_list, landmark_list, strict=True | |
| ): | |
| image_path = Path(image_file) | |
| if not image_path.exists(): | |
| raise FileNotFoundError(f"Image not found: {image_path}") | |
| entry: dict[str, str] = { | |
| "image": self._posix(image_path), | |
| "subject_id": subject_id, | |
| } | |
| if use_seg: | |
| if seg_file is None or not Path(seg_file).exists(): | |
| self.log_warning( | |
| "Skipping %s: segmentation missing for paired-with-seg " | |
| "training (seg=%s)", | |
| image_path, | |
| seg_file, | |
| ) | |
| continue | |
| entry["segmentation"] = self._posix(seg_file) | |
| if use_mask: | |
| if mask_file is not None and Path(mask_file).exists(): | |
| resolved_mask: Path = Path(mask_file) | |
| elif seg_file is not None and Path(seg_file).exists(): | |
| resolved_mask = self._derive_mask(seg_file) | |
| else: | |
| self.log_warning( | |
| "Skipping %s: neither explicit mask nor segmentation " | |
| "available to derive a loss-function mask " | |
| "(mask=%s, seg=%s)", | |
| image_path, | |
| mask_file, | |
| seg_file, | |
| ) | |
| continue | |
| entry["mask"] = self._posix(resolved_mask) | |
| if landmark_file is not None: | |
| entry["landmarks"] = self._posix(landmark_file) | |
| dataset_entries.append(entry) | |
| dataset_json_path = self.experiment_dir / f"{self.fine_tune_name}_dataset.json" | |
| if not dataset_entries: | |
| raise ValueError( | |
| 'No valid training entries were generated. ' | |
| 'Check image/segmentation/mask availability.' | |
| ) | |
| with dataset_json_path.open("w") as fh: | |
| json.dump({"data": dataset_entries}, fh, indent=2) | |
| self.log_info( | |
| "Wrote dataset JSON %s with %d entries", | |
| dataset_json_path, | |
| len(dataset_entries), | |
| ) | |
| self._dataset_json_path = dataset_json_path | |
| return dataset_json_path |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/physiomotion4d/workflow_fine_tune_icon_registration.py` around lines 412
- 490, The code currently continues to write the dataset JSON and launch
fine-tuning even if dataset_entries is empty; add a fail-fast check after
building dataset_entries and before writing dataset_json_path: if
len(dataset_entries) == 0, call self.log_error (or raise a
RuntimeError/ValueError) with a clear message including self.fine_tune_name and
abort (raise an exception) so downstream steps are not run on an empty dataset;
reference the local variable dataset_entries, the dataset_json_path write block,
and the attribute self._dataset_json_path to locate where to insert the check.
Comment on lines
+188
to
+189
| ↓ ├─→ test_register_images_ANTS ──→ test_transform_tools | ||
| ↓ ├─→ test_register_images_ICON |
There was a problem hiding this comment.
Use the actual lowercase test filenames in the dependency diagram.
Line 188 and Line 189 now reference uppercase filenames, but the test files are lowercase. This makes the diagram inconsistent and copy/paste navigation can fail on case-sensitive systems.
Suggested fix
- ↓ ├─→ test_register_images_ANTS ──→ test_transform_tools
- ↓ ├─→ test_register_images_ICON
+ ↓ ├─→ test_register_images_ants ──→ test_transform_tools
+ ↓ ├─→ test_register_images_icon📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ↓ ├─→ test_register_images_ANTS ──→ test_transform_tools | |
| ↓ ├─→ test_register_images_ICON | |
| ↓ ├─→ test_register_images_ants ──→ test_transform_tools | |
| ↓ ├─→ test_register_images_icon |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/README.md` around lines 188 - 189, Update the dependency diagram
entries that reference test_register_images_ANTS and test_register_images_ICON
to use the actual lowercase filenames (test_register_images_ants and
test_register_images_icon) so the diagram matches the real test file names;
locate the lines that contain "test_register_images_ANTS" and
"test_register_images_ICON" in the README diagram and replace them with
"test_register_images_ants" and "test_register_images_icon" respectively to
ensure case-sensitive navigation works.
| Tests for Greedy-based image registration. | ||
|
|
||
| Uses the same fixtures as test_register_images_ants (converted 3D CT images). | ||
| Uses the same fixtures as test_register_images_ANTS (converted 3D CT images). |
There was a problem hiding this comment.
Fix inaccurate referenced test module name.
Line 5 points to test_register_images_ANTS, but the test module in this repo is
test_register_images_ants.py (lowercase). Please align the docstring to the actual file name.
Suggested fix
-Uses the same fixtures as test_register_images_ANTS (converted 3D CT images).
+Uses the same fixtures as test_register_images_ants (converted 3D CT images).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Uses the same fixtures as test_register_images_ANTS (converted 3D CT images). | |
| Uses the same fixtures as test_register_images_ants (converted 3D CT images). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_register_images_greedy.py` at line 5, Update the module docstring
in test_register_images_greedy.py to reference the correct lowercase filename
"test_register_images_ants.py" instead of "test_register_images_ANTS"; locate
the docstring at the top of the file and change the referenced module name, and
while here ensure the module/test function names (e.g., any top-level string
mentioning test_register_images_ANTS or functions like
test_register_images_greedy) use meaningful, lowercase names consistent with the
repo convention.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements
--ANTS-iterations,--ICON-iterations,--use-ICON-refinement(previously lowercase variants).