Eliminate tubetk#76
Merged
Merged
Conversation
Add three new ImageTools methods that cover all TubeTK usage in the package and experiments: - make_isotropic_image: already existed (equivalent to ResampleImage.SetMakeHighResIso); now used in experiments too. - binary_dilate_image: replaces ImageMath.Dilate + GetOutputUChar/Short, using BinaryDilateImageFilter with a FlatStructuringElement ball kernel. - binary_erode_image: replaces ImageMath.Erode, using BinaryErodeImageFilter with a FlatStructuringElement ball kernel. - keep_largest_connected_component: replaces SegmentConnectedComponents with SetKeepOnlyLargestComponent(True), using ConnectedComponentImageFilter + RelabelComponentImageFilter + BinaryThresholdImageFilter. Callers updated: - src/physiomotion4d/segment_anatomy_base.py: hole-fill open in segment_connected_component() now uses ImageTools methods. - src/physiomotion4d/segment_heart_simpleware.py: myocardium dilate/erode in segmentation_method() now uses ImageTools methods. - src/physiomotion4d/segment_heart_simpleware_trimmed_branches.py: trim_branches() rewrites the multi-label ImageMath chain as per-label binary masks (avoiding inter-label bleed) and uses ImageTools for all morphology and largest-component selection. - experiments/Heart-Statistical_Model_To_Patient/*.py: ResampleImage replaced by ImageTools().make_isotropic_image(). - experiments/Lung-GatedCT_To_USD/0-register_dirlab_4dct.py: dilate_mask() helper replaced by ImageTools().binary_dilate_image(). Non-code changes: - pyproject.toml: drop itk-tubetk>=1.4.0 dependency. - docs/conf.py: remove sys.modules['itk.TubeTK'] mock (no longer imported). - tests/test_segment_heart_simpleware_trimmed_branches.py: remove the Python <3.12 skipif guard that existed solely because TubeTK's SegmentConnectedComponents segfaulted on CPython 3.11; trim_branches() is now pure ITK and runs on all supported Python versions.
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThe PR removes TubeTK references from docs, dependencies, and code, replaces TubeTK-based image morphology with new ITK/SimpleITK helpers, updates segmentation and registration APIs around ChangesCore API and workflow migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR removes the TubeTK dependency from PhysioMotion4D by replacing TubeTK-based image morphology / connected-components utilities with standard ITK filter implementations, and updates workflows, tests, docs, tutorials, and experiments to use the new paths.
Changes:
- Dropped
itk-tubetkfrom dependencies and replaced TubeTK operations with ITK-based helpers inImageTools. - Refactored segmentation “contrast enhanced” handling: removed
contrast_enhanced_studyfromSegmentAnatomyBase.segment()and addedSegmentChestTotalSegmentator.set_contrast_enhanced_study(), updating many call sites. - Refactored
WorkflowConvertImageToUSD’s inputs (now accepts in-memory time-series images + reference image) and adjusted registration/asset-writing behavior.
Reviewed changes
Copilot reviewed 48 out of 51 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_01_heart_gated_ct_to_usd.py | Updates tutorial to new WorkflowConvertImageToUSD constructor and removes direct ICON setup. |
| tests/test_segment_heart_simpleware.py | Updates Simpleware tests to call segment() without contrast_enhanced_study. |
| tests/test_segment_heart_simpleware_trimmed_branches.py | Removes old TubeTK/Python-version skip logic and updates calls to new segment() signature. |
| tests/test_segment_chest_total_segmentator.py | Updates TotalSegmentator tests to use set_contrast_enhanced_study() instead of passing a flag into segment(). |
| tests/test_register_time_series_images.py | Removes strict baseline assertions for Greedy time-series registration outputs; saves artifacts instead. |
| tests/test_experiments.py | Reduces per-script timeout for a reconstruction experiment test. |
| tests/test_convert_image_4d_to_3d.py | Consolidates/renames 4D→3D conversion tests and keeps basic output assertions. |
| tests/conftest.py | Updates TotalSegmentator labelmap fixture generation for new segment() signature. |
| statistics.md | Removes TubeTK mention from dependency list. |
| src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py | Removes Greedy+ICON default from docstring/example and tweaks default upsampling behavior. |
| src/physiomotion4d/workflow_fine_tune_icon_registration.py | Trims module docstring references to experiment scripts. |
| src/physiomotion4d/workflow_convert_image_to_vtk.py | Adds contrast-flag routing via set_contrast_enhanced_study() when supported. |
| src/physiomotion4d/workflow_convert_image_to_usd.py | Major API refactor: now takes images directly, changes asset-saving behavior, and rewrites dynamic/static registration logic. |
| src/physiomotion4d/vtk_to_usd/usd_utils.py | Replaces non-ASCII arrow in debug log with ASCII ->. |
| src/physiomotion4d/simpleware_medical/README.md | Updates usage example to call segment() without contrast_enhanced_study. |
| src/physiomotion4d/segment_heart_simpleware.py | Replaces TubeTK morphology with ImageTools dilate/erode helpers. |
| src/physiomotion4d/segment_heart_simpleware_trimmed_branches.py | Replaces TubeTK morphology/CC logic with ITK-based ImageTools helpers. |
| src/physiomotion4d/segment_chest_total_segmentator.py | Adds contrast-enhanced detection toggles and implements CC-based contrast labeling with ITK filters. |
| src/physiomotion4d/segment_anatomy_base.py | Removes TubeTK import and removes contrast_enhanced_study parameter from segment(); adds postprocess_after_labelmap() hook. |
| src/physiomotion4d/register_time_series_images.py | Switches the default backend to RegisterImagesGreedyICON with affine greedy stage. |
| src/physiomotion4d/register_models_pca.py | Removes non-ASCII “checkmark” glyphs from logs. |
| src/physiomotion4d/register_models_icp_itk.py | Removes non-ASCII “checkmark” glyphs from logs. |
| src/physiomotion4d/register_images_icon.py | Removes TubeTK-specific note from lazy-load docstring. |
| src/physiomotion4d/register_images_base.py | Adds fast_mode attribute and documents it. |
| src/physiomotion4d/register_images_ants.py | Uses fast_mode to reduce masking cost (mask_all_stages). |
| src/physiomotion4d/image_tools.py | Adds ITK-based binary dilate/erode and “keep largest component” helpers. |
| src/physiomotion4d/cli/convert_image_to_vtk.py | Replaces Unicode arrows in CLI output with ASCII ->. |
| src/physiomotion4d/cli/convert_image_to_usd.py | Updates CLI to build in-memory time series + reference image and configure TotalSegmentator contrast via setter. |
| README.md | Removes TubeTK from dependency list. |
| pyproject.toml | Removes itk-tubetk dependency and adjusts pytest timeout. |
| experiments/Reconstruct4DCT/reconstruct_4d_ct.py | Alters quick-run behavior for tests and switches experiment registration backend setup. |
| experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py | Updates experiment narrative and reconstruction defaults/outputs. |
| experiments/Lung-GatedCT_To_USD/0-register_dirlab_4dct.py | Replaces TubeTK mask dilation with ImageTools. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_model_registration_pca.py | Replaces TubeTK isotropic resampling with ImageTools.make_isotropic_image and removes non-ASCII glyphs. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_model_icp_itk.py | Replaces TubeTK isotropic resampling with ImageTools.make_isotropic_image and removes non-ASCII glyphs. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/setup.sh | Improves venv Python-path detection across platforms. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/registration_test.py | Adds a hard-coded-path registration benchmark script. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/registration_results_analysis.py | Fixes empty-data handling in occurrence splitting. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/composite_time_series_mid_slice.py | Makes tkinter optional and provides clearer runtime errors if unavailable. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/3-eval_icon.py | Adds basic cohort-size validation and reorganizes output initialization. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/2-finetune_icon.py | Adds a full fine-tuning driver script for uniGradICON. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/1-initial_registration.py | Fixes empty-mask crop handling and off-by-one slicing for crop ranges. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.py | Updates segmenter invocation to new segment() signature and adjusts landmark output naming. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/.gitignore | Ignores local uniGradICON clones and output artifacts. |
| experiments/Heart-GatedCT_To_USD/3-transform_dynamic_and_static_contours.py | Adjusts test-mode frame stepping. |
| experiments/Heart-GatedCT_To_USD/2-generate_segmentation.py | Adds fast_mode toggle (but still uses deprecated contrast_enhanced_study kwarg in segment() call). |
| experiments/Heart-GatedCT_To_USD/1-register_images.py | Adds fast_mode and test-mode shortcuts (but still uses deprecated contrast_enhanced_study kwarg in segment() calls). |
| experiments/Convert_VTK_To_USD/convert_vtk_to_usd_using_class.py | Removes non-ASCII glyphs from print output and clarifies failure text. |
| experiments/Colormap-VTK_To_USD/colormap_vtk_to_usd.py | Removes non-ASCII glyphs from print output. |
| docs/installation.rst | Removes TubeTK from dependency list. |
| docs/conf.py | Removes TubeTK mocking from Sphinx configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/composite_time_series_mid_slice.py (1)
15-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUncaught
RuntimeErrornow propagates throughmain()on headless/tkinter failure.
select_directory()previously returnedNoneontk.TclError, lettingmain()print"No directory selected"and cleanlyreturn 1. Now it raisesRuntimeErrorfor bothImportErrorandtk.TclError, butmain()(lines 159-164) still callsselect_directory()unguarded — the exception propagates uncaught, producing a raw traceback instead of the intended clean CLI error message. This regresses the "harden tkinter handling" goal for headless/no-display environments (e.g., CI without a display).🔧 Proposed fix
input_dir = args.directory if input_dir is None: - input_dir = select_directory() + try: + input_dir = select_directory() + except RuntimeError as exc: + print(f"Error: {exc}") + return 1 if input_dir is None: print("No directory selected") return 1Also applies to: 139-164
🤖 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-GatedCT-OptimizedLongitudinalRegistration/composite_time_series_mid_slice.py` around lines 15 - 42, select_directory() now raises RuntimeError for tkinter import/display failures, but main() still calls it without handling those cases, causing an uncaught traceback in headless environments. Update main() to catch the RuntimeError from select_directory() (or restore the None/clean-failure path) and keep the existing "No directory selected" style CLI exit behavior. Use the select_directory() and main() symbols to locate the control flow and preserve the current fallback handling for cancelled/failed directory selection.src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py (1)
292-303: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring still says
Default: False, but the signature default is nowTrue.📝 Proposed doc fix
upsample_to_fixed_resolution (bool, optional): If True, reconstructed images will be upsampled to isotropic resolution (mean of fixed image's X and Y spacing) while maintaining their original origin - and direction. Default: False + and direction. Default: True🤖 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_reconstruct_highres_4d_ct.py` around lines 292 - 303, Update the docstring for the reconstruction method so it matches the actual signature default for upsample_to_fixed_resolution. In the workflow_reconstruct_highres_4d_ct.py method that reconstructs the high-resolution time series, change the documented default from False to True and keep the argument description aligned with the current behavior.
🧹 Nitpick comments (5)
src/physiomotion4d/workflow_convert_image_to_usd.py (2)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid a mutable default argument.
dynamic_labelmap_ids: list[int] = []is a shared mutable default (ruffB006). It is only read here so it is currently harmless, but preferOptional[list[int]] = Nonewith normalization inside__init__to guard against future mutation.🤖 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_convert_image_to_usd.py` at line 53, The default value for dynamic_labelmap_ids in the __init__ signature is a shared mutable list, so update workflow_convert_image_to_usd.py to use a non-mutable default such as None and normalize it inside __init__. Keep the change localized to the constructor and any related handling of dynamic_labelmap_ids so the WorkflowConvertImageToUsd initialization remains safe against future mutation.
165-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn type is narrower than what
register()actually returns.
self.registrar.register(...)returnsdict[str, Union[itk.Transform, float]](it includes the"loss"float), but_register_with_maskis annotated-> dict[str, itk.Transform]and returns that dict directly. Under mypy strict this is an incompatible-return-type error. Also, the local namesinverse_transform_dynamic/forward_transform_dynamicare misleading since this helper serves both dynamic and static registration.As per coding guidelines: "Use full type hints with
mypystrict mode (disallow_untyped_defs = true)".🤖 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_convert_image_to_usd.py` around lines 165 - 198, `_register_with_mask` is annotated too narrowly for the value returned by `self.registrar.register(...)`, which also includes the `"loss"` float. Update the return type of `_register_with_mask` to match the actual `registration_results` shape, and keep the return value consistent with that broader mapping so mypy strict passes. While you are in this helper, rename the local transform variables away from `inverse_transform_dynamic` and `forward_transform_dynamic` to clearer names that reflect this shared registration path in `workflow_convert_image_to_usd`.tests/test_convert_image_4d_to_3d.py (1)
37-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStale output files can make the count assertion flaky.
output_diris reused across runs but never cleared beforesave_3d_imageswrites newtest_slice_*.mhafiles. If a prior run fails before the cleanup loop (line 50-51) runs, leftover files inflate the glob count on the next run, causingassert len(test_slice_files) == num_time_pointsto fail spuriously.🧹 Proposed fix
output_dir = test_directories["output"] / "convert_image_4d_to_3d" output_dir.mkdir(parents=True, exist_ok=True) + for stale_file in output_dir.glob("test_slice_*.mha"): + stale_file.unlink() conv.save_3d_images(output_dir, "test_slice")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_convert_image_4d_to_3d.py` around lines 37 - 51, The test in convert_image_4d_to_3d is counting stale `test_slice_*.mha` files from prior runs, which can make the glob-based assertion flaky. Before calling `conv.save_3d_images`, ensure the `output_dir` is cleaned of any existing matching files (or otherwise isolate the run) so `test_slice_files` only reflects the files created by this invocation. Update the cleanup logic around `output_dir`, `save_3d_images`, and the `test_slice_*.mha` glob so the count assertion stays deterministic even if a previous run exited early.src/physiomotion4d/image_tools.py (1)
345-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for
keep_largest_connected_component.This is new, non-trivial logic (connected components → relabel-by-size → threshold) with downstream consumers in
segment_heart_simpleware_trimmed_branches.pythat directly affect vessel-trimming correctness. A quick synthetic test (e.g. two disjoint blobs of different sizes) would confirm the largest-component selection and the "no components found" fallback behave as intended.Separately, note that the relabeled output uses a 16-bit
itk.USimage (max 65535 labels) due to ITK's Python-wrapping constraints forRelabelComponentImageFilter(already explained in the surrounding code comment) — unlikely to matter for the current heart/vessel mask use cases, but worth keeping in mind if this helper is reused on noisier masks with very many components.🧪 Suggested test sketch
def test_keep_largest_connected_component_keeps_largest_blob(): arr = np.zeros((10, 10, 10), dtype=np.uint8) arr[1:3, 1:3, 1:3] = 1 # small blob (8 voxels) arr[5:9, 5:9, 5:9] = 1 # large blob (64 voxels) image = itk.image_from_array(arr) result = ImageTools().keep_largest_connected_component(image) result_arr = itk.array_from_image(result) assert result_arr[1:3, 1:3, 1:3].sum() == 0 assert result_arr[5:9, 5:9, 5:9].sum() == arr[5:9, 5:9, 5:9].sum()🤖 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/image_tools.py` around lines 345 - 402, Add unit tests for keep_largest_connected_component in ImageTools to cover the new connected-component/relabel/threshold flow. Create a synthetic mask with two disjoint blobs of different sizes and assert the larger one is preserved while the smaller one is removed, and add a case for an empty mask to verify the no-components fallback returns an all-background image. Use the keep_largest_connected_component method directly so the tests are tied to the actual helper used by segment_heart_simpleware_trimmed_branches.py.experiments/Reconstruct4DCT/reconstruct_4d_ct.py (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment references removed import.
The commented-out alternative on line 29 references
RegisterImagesANTS(), but the import forRegisterImagesANTSwas removed from this file (line 8 now only importsRegisterImagesGreedy). Uncommenting this line as-is would raise aNameError.♻️ Suggested fix
reg_method_data = zip(["Greedy"], [RegisterImagesGreedy()], [[30, 15, 5]]) # reg_method_data = zip(["ICON"], [RegisterImagesICON()], [20]) -# reg_method_data = zip(["ICON","ANTs"], [RegisterImagesICON(), RegisterImagesANTS()], [20, [40, 20, 10]]) +# reg_method_data = zip(["ICON","ANTs"], [RegisterImagesICON(), RegisterImagesANTS()], [20, [40, 20, 10]]) # requires importing RegisterImagesANTS🤖 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/Reconstruct4DCT/reconstruct_4d_ct.py` around lines 27 - 29, The commented-out alternative in reconstruct_4d_ct.py still references RegisterImagesANTS() even though that symbol is no longer imported, so update the stale comment to match the current imports or remove the reference entirely. Check the reg_method_data setup near the RegisterImagesGreedy() usage and ensure any future example using RegisterImagesANTS() also restores the needed import before being uncommented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py`:
- Around line 55-58: The _build_registrar method now has a Default branch that
returns None, which conflicts with its declared RegisterImagesBase return type.
Update the type annotation on _build_registrar to allow the None return, or
change the Default handling so it returns a valid RegisterImagesBase instance;
keep the signature and return behavior consistent with the registrar selection
logic in Reconstruct4DCT.
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct.py`:
- Around line 20-29: The quick-run branch in reconstruct_4d_ct.py currently
exits immediately via TestTools.running_as_test(), which bypasses the
reconstruction flow during PHYSIOMOTION_RUNNING_AS_TEST. Update the quick_run
handling near the top-level setup so it keeps a reduced but real execution path
instead of calling exit(0), and preserve the reconstruction setup by continuing
through the logic that defines files_indx, reference_image_num, and
reg_method_data with a minimal workload.
In `@src/physiomotion4d/cli/convert_image_to_usd.py`:
- Around line 150-156: The CLI contract in convert_image_to_usd is still
advertising 4D volume/DICOM series support, but the current time_series_images
construction treats every input path as an independent frame via itk.imread.
Update the input handling in convert_image_to_usd/main so a single 4D file or
DICOM series is expanded into per-frame images before time_series_images is
built, or else narrow the argparse help/usage text to only accept explicit
per-frame 3D inputs. Keep the reference image selection logic consistent with
the final frame list.
In `@src/physiomotion4d/register_time_series_images.py`:
- Around line 96-97: Update the default registrar test to match the new default
in RegisterTimeSeriesImages: since register_time_series_images() now
instantiates RegisterImagesGreedyICON and configures
greedy.set_transform_type("Affine"), adjust the expectation in
tests/test_register_time_series_images.py so it asserts RegisterImagesGreedyICON
rather than RegisterImagesGreedy, using the RegisterTimeSeriesImages and
RegisterImagesGreedyICON symbols to locate the affected assertion.
In `@src/physiomotion4d/segment_anatomy_base.py`:
- Around line 71-73: The change in SegmentAnatomyBase removed the inherited
133/135 placeholder registrations, which breaks subclasses like
SegmentHeartSimpleware that still expect soft_tissue and contrast to be present.
Restore those default taxonomy registrations in SegmentAnatomyBase around the
self.taxonomy initialization, or explicitly add the same keys in every subclass
that depends on them, and make sure the shared taxonomy contract remains intact
for USDAnatomyTools and ConvertVTKToUSD.
In `@src/physiomotion4d/segment_chest_total_segmentator.py`:
- Line 368: Update the type annotation on the relevant parameter in
segment_chest_total_segmentator.py to use Optional[list[int]] instead of None |
list[int], and make sure Optional is imported from typing. The change should be
applied in the function signature that currently declares labelmap_ids so the
code matches the project typing guidelines.
In `@src/physiomotion4d/workflow_convert_image_to_usd.py`:
- Around line 358-392: `_transform_all_contours` and `_create_usd_files` build
`anatomy_types` incorrectly by always including "all", which causes a KeyError
in the dynamic path because `registration_results` and `reference_contours` are
stored as either "all" or "dynamic"/"static", not both. Update the selection
logic in both methods to use mutually exclusive anatomy type lists that match
how `_segment_and_register_frames` populates results, and reference the existing
symbols `dynamic_labelmap_ids`, `registration_results`, and `reference_contours`
to keep the lookup consistent. Also align `process()` with the actual outputs in
dynamic mode so it does not always return an `"all_painted"` USD filename when
only dynamic/static USDs are produced.
In `@src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py`:
- Line 153: The workflow default is inconsistent because
WorkflowReconstructHighres4DCT sets self.upsample_to_fixed_resolution in
__init__ but run_workflow still defaults to False and there is no
set_upsample_to_fixed_resolution wiring. Update run_workflow (and any related
entry-point/config path in WorkflowReconstructHighres4DCT) so the default
behavior reads from self.upsample_to_fixed_resolution, or add the missing setter
and use it consistently, ensuring callers get the class default without passing
True explicitly.
---
Outside diff comments:
In
`@experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/composite_time_series_mid_slice.py`:
- Around line 15-42: select_directory() now raises RuntimeError for tkinter
import/display failures, but main() still calls it without handling those cases,
causing an uncaught traceback in headless environments. Update main() to catch
the RuntimeError from select_directory() (or restore the None/clean-failure
path) and keep the existing "No directory selected" style CLI exit behavior. Use
the select_directory() and main() symbols to locate the control flow and
preserve the current fallback handling for cancelled/failed directory selection.
In `@src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py`:
- Around line 292-303: Update the docstring for the reconstruction method so it
matches the actual signature default for upsample_to_fixed_resolution. In the
workflow_reconstruct_highres_4d_ct.py method that reconstructs the
high-resolution time series, change the documented default from False to True
and keep the argument description aligned with the current behavior.
---
Nitpick comments:
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct.py`:
- Around line 27-29: The commented-out alternative in reconstruct_4d_ct.py still
references RegisterImagesANTS() even though that symbol is no longer imported,
so update the stale comment to match the current imports or remove the reference
entirely. Check the reg_method_data setup near the RegisterImagesGreedy() usage
and ensure any future example using RegisterImagesANTS() also restores the
needed import before being uncommented.
In `@src/physiomotion4d/image_tools.py`:
- Around line 345-402: Add unit tests for keep_largest_connected_component in
ImageTools to cover the new connected-component/relabel/threshold flow. Create a
synthetic mask with two disjoint blobs of different sizes and assert the larger
one is preserved while the smaller one is removed, and add a case for an empty
mask to verify the no-components fallback returns an all-background image. Use
the keep_largest_connected_component method directly so the tests are tied to
the actual helper used by segment_heart_simpleware_trimmed_branches.py.
In `@src/physiomotion4d/workflow_convert_image_to_usd.py`:
- Line 53: The default value for dynamic_labelmap_ids in the __init__ signature
is a shared mutable list, so update workflow_convert_image_to_usd.py to use a
non-mutable default such as None and normalize it inside __init__. Keep the
change localized to the constructor and any related handling of
dynamic_labelmap_ids so the WorkflowConvertImageToUsd initialization remains
safe against future mutation.
- Around line 165-198: `_register_with_mask` is annotated too narrowly for the
value returned by `self.registrar.register(...)`, which also includes the
`"loss"` float. Update the return type of `_register_with_mask` to match the
actual `registration_results` shape, and keep the return value consistent with
that broader mapping so mypy strict passes. While you are in this helper, rename
the local transform variables away from `inverse_transform_dynamic` and
`forward_transform_dynamic` to clearer names that reflect this shared
registration path in `workflow_convert_image_to_usd`.
In `@tests/test_convert_image_4d_to_3d.py`:
- Around line 37-51: The test in convert_image_4d_to_3d is counting stale
`test_slice_*.mha` files from prior runs, which can make the glob-based
assertion flaky. Before calling `conv.save_3d_images`, ensure the `output_dir`
is cleaned of any existing matching files (or otherwise isolate the run) so
`test_slice_files` only reflects the files created by this invocation. Update
the cleanup logic around `output_dir`, `save_3d_images`, and the
`test_slice_*.mha` glob so the count assertion stays deterministic even if a
previous run exited early.
🪄 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: 099b1608-c6e0-443f-8f56-5d200e094aa6
📒 Files selected for processing (51)
README.mddocs/conf.pydocs/installation.rstexperiments/Colormap-VTK_To_USD/colormap_vtk_to_usd.pyexperiments/Convert_VTK_To_USD/convert_vtk_to_usd_using_class.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/.gitignoreexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/0-cardiacGatedCT_segment_and_landmark.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/1-initial_registration.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/2-finetune_icon.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/3-eval_icon.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/composite_time_series_mid_slice.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/registration_results_analysis.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/registration_test.pyexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/setup.shexperiments/Heart-GatedCT_To_USD/1-register_images.pyexperiments/Heart-GatedCT_To_USD/2-generate_segmentation.pyexperiments/Heart-GatedCT_To_USD/3-transform_dynamic_and_static_contours.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_model_icp_itk.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_model_registration_pca.pyexperiments/Lung-GatedCT_To_USD/0-register_dirlab_4dct.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct_class.pypyproject.tomlsrc/physiomotion4d/cli/convert_image_to_usd.pysrc/physiomotion4d/cli/convert_image_to_vtk.pysrc/physiomotion4d/image_tools.pysrc/physiomotion4d/register_images_ants.pysrc/physiomotion4d/register_images_base.pysrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/register_models_icp_itk.pysrc/physiomotion4d/register_models_pca.pysrc/physiomotion4d/register_time_series_images.pysrc/physiomotion4d/segment_anatomy_base.pysrc/physiomotion4d/segment_chest_total_segmentator.pysrc/physiomotion4d/segment_heart_simpleware.pysrc/physiomotion4d/segment_heart_simpleware_trimmed_branches.pysrc/physiomotion4d/simpleware_medical/README.mdsrc/physiomotion4d/vtk_to_usd/usd_utils.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_convert_image_to_vtk.pysrc/physiomotion4d/workflow_fine_tune_icon_registration.pysrc/physiomotion4d/workflow_reconstruct_highres_4d_ct.pystatistics.mdtests/conftest.pytests/test_convert_image_4d_to_3d.pytests/test_experiments.pytests/test_register_time_series_images.pytests/test_segment_chest_total_segmentator.pytests/test_segment_heart_simpleware.pytests/test_segment_heart_simpleware_trimmed_branches.pytutorials/tutorial_01_heart_gated_ct_to_usd.py
💤 Files with no reviewable changes (1)
- docs/conf.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_anatomy_taxonomy.py`:
- Around line 116-122: The test name
test_segment_anatomy_base_default_taxonomy_seeded no longer matches its behavior
because it only instantiates SegmentAnatomyBase and does not verify seeded
taxonomy data. Update the test body to assert the default taxonomy seeding state
on the SegmentAnatomyBase instance, such as group membership or label_to_type
contents, so it still covers the intended regression. If that verification is
not available, rename the test to reflect instantiation-only behavior instead.
In `@tests/test_workflow_convert_image_to_usd.py`:
- Around line 18-76: Add back a fast, non-GPU unit test in
test_workflow_convert_image_to_usd.py that exercises WorkflowConvertImageToUSD
construction only, so default segmenter/registrar selection and type validation
are covered outside the slow pipeline. Use the WorkflowConvertImageToUSD
constructor and assert the defaults for segmenter and registrar (like in the
existing test’s isinstance checks) plus any invalid input/type cases that
previously validated construction behavior, keeping it separate from process()
and USD output assertions.
🪄 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: 44fba480-f309-409e-9c08-63da0a98c507
📒 Files selected for processing (7)
experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/setup.shsrc/physiomotion4d/cli/convert_image_to_usd.pysrc/physiomotion4d/convert_image_4d_to_3d.pysrc/physiomotion4d/workflow_convert_image_to_usd.pytests/test_anatomy_taxonomy.pytests/test_cli_smoke.pytests/test_workflow_convert_image_to_usd.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/physiomotion4d/cli/convert_image_to_usd.py
- src/physiomotion4d/workflow_convert_image_to_usd.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
+ Coverage 34.66% 36.05% +1.38%
==========================================
Files 57 57
Lines 7283 7275 -8
==========================================
+ Hits 2525 2623 +98
+ Misses 4758 4652 -106
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/physiomotion4d/cli/reconstruct_highres_4d_ct.py (1)
124-125: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
--upsamplecompatibility or remove it from docs The CLI no longer accepts--upsample, butdocs/cli_scripts/4dct_reconstruction.rststill advertises it. Keep a deprecated alias or update the docs/tests if the forced upsampling is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiomotion4d/cli/reconstruct_highres_4d_ct.py` around lines 124 - 125, The CLI option handling in reconstruct_highres_4d_ct.py no longer matches the documented `--upsample` flag, so either restore backward-compatible support for that alias in the argument parser or remove it consistently from the docs and tests. Update the CLI parsing logic around the reconstruction entrypoint to accept `--upsample` as a deprecated alias if forced upsampling is still intended, and make sure the docs in docs/cli_scripts/4dct_reconstruction.rst and any related tests reflect the chosen behavior.src/physiomotion4d/workflow_convert_image_to_usd.py (3)
45-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the missing
__init__return annotation.
__init__is changed but still lacks-> None, which violates strict typed-def expectations. As per coding guidelines, “Use full type hints withmypystrict mode (disallow_untyped_defs = true)” and “Add type hints to Python functions.”Proposed fix
- ): + ) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiomotion4d/workflow_convert_image_to_usd.py` around lines 45 - 58, The __init__ method in WorkflowConvertImageToUSD is missing an explicit return annotation, which conflicts with strict typing expectations. Update the __init__ signature to include a -> None return type while keeping the existing parameter type hints intact, so the class initializer remains compliant with mypy strict mode and typed-def guidelines.Source: Coding guidelines
180-192: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
save_assetsbehavior for the non-dynamic path.When
dynamic_labelmap_idsis empty, the"all"branch callsself.registrar.register()directly, sosave_assets=Truedoes not write the registered image or forward/inverse transform assets for those frames. Route the"all"branch through the same asset-writing helper.Proposed fix
def _register_with_mask( self, fixed_image: itk.Image, - fixed_mask: itk.Image, + fixed_mask: Optional[itk.Image], moving_image: itk.Image, - moving_mask: itk.Image, + moving_mask: Optional[itk.Image], filename_prefix: str = "", ) -> dict[str, Union[itk.Transform, float]]:else: - all_reg_results = self.registrar.register(moving_image) + all_reg_results = self._register_with_mask( + self.reference_image, + None, + moving_image, + None, + f"slice_{i:03d}_all", + ) self.registration_results.append( { "all": all_reg_results,Also applies to: 328-333
🤖 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_convert_image_to_usd.py` around lines 180 - 192, The non-dynamic "all" branch in `convert_image_to_usd` bypasses the asset-writing path, so `save_assets=True` never persists the registered image or forward/inverse transforms for those frames. Update the `"all"` handling to go through the same helper used by the dynamic-labelmap flow, namely `_register_with_mask` and the asset-saving logic it drives, instead of calling `self.registrar.register()` directly. Keep the existing non-dynamic behavior otherwise, but ensure the same `save_assets` side effects are applied consistently for both branches.
235-258: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject empty dynamic/static masks before registration.
If configured dynamic IDs are absent, or if they cover all foreground labels, this creates empty ROIs and still sends them into masked registration. Fail early with a clear
ValueErroror explicitly support skipping that anatomy branch.Proposed guard
labelmap_arr = itk.GetArrayFromImage(labelmap) is_dynamic_arr = np.isin(labelmap_arr, self.dynamic_labelmap_ids) + if not np.any(is_dynamic_arr): + raise ValueError( + "dynamic_labelmap_ids did not match any reference labels" + ) dynamic_labelmap_arr = np.where(is_dynamic_arr, 1, 0) @@ static_labelmap_arr = np.where((labelmap_arr > 0) & ~is_dynamic_arr, 1, 0) + if not np.any(static_labelmap_arr): + raise ValueError( + "dynamic_labelmap_ids leave no static reference anatomy" + )moving_is_dynamic_arr = np.isin( moving_labelmap_arr, self.dynamic_labelmap_ids ) + if not np.any(moving_is_dynamic_arr): + raise ValueError( + f"dynamic_labelmap_ids did not match labels in frame {i}" + ) moving_dynamic_labelmap_arr = np.where(moving_is_dynamic_arr, 1, 0) @@ static_labelmap_arr = np.where( (moving_labelmap_arr > 0) & ~moving_is_dynamic_arr, 1, 0 ) + if not np.any(static_labelmap_arr): + raise ValueError(f"dynamic_labelmap_ids leave no static anatomy in frame {i}")Also applies to: 278-314
🤖 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_convert_image_to_usd.py` around lines 235 - 258, The dynamic/static ROI creation in workflow_convert_image_to_usd.py can produce empty masks when self.dynamic_labelmap_ids are missing from the labelmap or when they consume all foreground labels, yet the code still proceeds into masked registration. Add a guard in the branch that builds reference_dynamic_mask and reference_static_mask to validate both masks after creation and before any registration use, and raise a clear ValueError or skip that anatomy branch explicitly when either ROI is empty. Apply the same check to the later registration path that consumes these masks so the workflow never sends empty masks into registration.src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py (1)
138-140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the default-registration documentation.
registration_method=Noneis now accepted and passed through, but the docs still say omission defaults toRegisterImagesGreedyICON/ combined Greedy+ICON. That misstates the new constructor contract.🤖 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_reconstruct_highres_4d_ct.py` around lines 138 - 140, The default-registration documentation in the constructor path for workflow_reconstruct_highres_4d_ct should be updated to match the new behavior where registration_method=None is accepted and passed through. Adjust the docstring or related constructor docs near the registration_method validation in the workflow builder so it no longer claims omission defaults to RegisterImagesGreedyICON or combined Greedy+ICON, and instead describes the new contract accurately.
🧹 Nitpick comments (1)
tests/test_register_time_series_images.py (1)
196-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExistence-only checks replace baseline comparisons.
All three tests now only assert output files exist, no longer validating registration correctness (transform sanity, image similarity, etc.). The non-reproducibility rationale is reasonable, but consider a weaker invariant check (e.g., transform is not identity, or image differs from moving image) instead of dropping verification entirely, to retain some regression signal.
Also applies to: 259-270, 435-442
🤖 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_time_series_images.py` around lines 196 - 209, The affected time-series registration tests now only check that artifacts are written, which removes meaningful regression coverage. In the relevant test methods that call write_result_transform and write_result_image, replace the existence-only assertions with weaker but still meaningful invariants such as validating the transform is not the identity / has expected structure and the registered image differs from the moving image or improves over it, so the tests still exercise registration correctness without relying on bitwise reproducibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/physiomotion4d/workflow_convert_image_to_usd.py`:
- Around line 380-383: `process()` is not idempotent because
`_transform_all_contours()` appends to the existing contour/time-sample
collections on repeat runs, causing duplicate USD data. Update the workflow in
`workflow_convert_image_to_usd.py` so the contour-related state is cleared or
reinitialized before each transformation pass, using the relevant fields touched
by `_transform_all_contours()` and `process()`, then rebuild the transformed
contours from scratch before appending new samples.
In `@src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py`:
- Line 303: The API examples are stale because they still show run_workflow
being called with upsample_to_fixed_resolution, which no longer exists on the
workflow entrypoint. Update the snippets in the docs to use the current workflow
method and its accepted arguments, matching the implementation in
reconstruct_time_series and run_workflow so the examples don’t raise TypeError
when copied.
---
Outside diff comments:
In `@src/physiomotion4d/cli/reconstruct_highres_4d_ct.py`:
- Around line 124-125: The CLI option handling in reconstruct_highres_4d_ct.py
no longer matches the documented `--upsample` flag, so either restore
backward-compatible support for that alias in the argument parser or remove it
consistently from the docs and tests. Update the CLI parsing logic around the
reconstruction entrypoint to accept `--upsample` as a deprecated alias if forced
upsampling is still intended, and make sure the docs in
docs/cli_scripts/4dct_reconstruction.rst and any related tests reflect the
chosen behavior.
In `@src/physiomotion4d/workflow_convert_image_to_usd.py`:
- Around line 45-58: The __init__ method in WorkflowConvertImageToUSD is missing
an explicit return annotation, which conflicts with strict typing expectations.
Update the __init__ signature to include a -> None return type while keeping the
existing parameter type hints intact, so the class initializer remains compliant
with mypy strict mode and typed-def guidelines.
- Around line 180-192: The non-dynamic "all" branch in `convert_image_to_usd`
bypasses the asset-writing path, so `save_assets=True` never persists the
registered image or forward/inverse transforms for those frames. Update the
`"all"` handling to go through the same helper used by the dynamic-labelmap
flow, namely `_register_with_mask` and the asset-saving logic it drives, instead
of calling `self.registrar.register()` directly. Keep the existing non-dynamic
behavior otherwise, but ensure the same `save_assets` side effects are applied
consistently for both branches.
- Around line 235-258: The dynamic/static ROI creation in
workflow_convert_image_to_usd.py can produce empty masks when
self.dynamic_labelmap_ids are missing from the labelmap or when they consume all
foreground labels, yet the code still proceeds into masked registration. Add a
guard in the branch that builds reference_dynamic_mask and reference_static_mask
to validate both masks after creation and before any registration use, and raise
a clear ValueError or skip that anatomy branch explicitly when either ROI is
empty. Apply the same check to the later registration path that consumes these
masks so the workflow never sends empty masks into registration.
In `@src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py`:
- Around line 138-140: The default-registration documentation in the constructor
path for workflow_reconstruct_highres_4d_ct should be updated to match the new
behavior where registration_method=None is accepted and passed through. Adjust
the docstring or related constructor docs near the registration_method
validation in the workflow builder so it no longer claims omission defaults to
RegisterImagesGreedyICON or combined Greedy+ICON, and instead describes the new
contract accurately.
---
Nitpick comments:
In `@tests/test_register_time_series_images.py`:
- Around line 196-209: The affected time-series registration tests now only
check that artifacts are written, which removes meaningful regression coverage.
In the relevant test methods that call write_result_transform and
write_result_image, replace the existence-only assertions with weaker but still
meaningful invariants such as validating the transform is not the identity / has
expected structure and the registered image differs from the moving image or
improves over it, so the tests still exercise registration correctness without
relying on bitwise reproducibility.
🪄 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: ddfa49ed-6d88-454b-931e-98a99273ee39
📒 Files selected for processing (18)
experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/composite_time_series_mid_slice.pyexperiments/Heart-GatedCT_To_USD/1-register_images.pyexperiments/Heart-GatedCT_To_USD/2-generate_segmentation.pyexperiments/Heart-Simpleware_Segmentation/simpleware_heart_segmentation.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct.pyexperiments/Reconstruct4DCT/reconstruct_4d_ct_class.pysrc/physiomotion4d/cli/reconstruct_highres_4d_ct.pysrc/physiomotion4d/image_tools.pysrc/physiomotion4d/segment_chest_total_segmentator.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_convert_image_to_vtk.pysrc/physiomotion4d/workflow_reconstruct_highres_4d_ct.pytests/test_anatomy_taxonomy.pytests/test_convert_image_4d_to_3d.pytests/test_image_tools.pytests/test_register_time_series_images.pytests/test_workflow_convert_image_to_usd.pytutorials/tutorial_01_heart_gated_ct_to_usd.py
🚧 Files skipped from review as they are similar to previous changes (9)
- experiments/Heart-GatedCT_To_USD/2-generate_segmentation.py
- tests/test_anatomy_taxonomy.py
- tests/test_convert_image_4d_to_3d.py
- src/physiomotion4d/image_tools.py
- experiments/Heart-GatedCT_To_USD/1-register_images.py
- tutorials/tutorial_01_heart_gated_ct_to_usd.py
- experiments/Reconstruct4DCT/reconstruct_4d_ct.py
- experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py
- src/physiomotion4d/segment_chest_total_segmentator.py
Comment on lines
+380
to
+383
| if len(self.dynamic_labelmap_ids) > 0: | ||
| anatomy_types = ["dynamic", "static"] | ||
| else: | ||
| anatomy_types = ["all"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset transformed contours before appending.
process() can be called more than once on the same workflow; _transform_all_contours() appends to existing lists, so reruns duplicate time samples in the USD output.
Proposed fix
if len(self.dynamic_labelmap_ids) > 0:
anatomy_types = ["dynamic", "static"]
else:
anatomy_types = ["all"]
+
+ for anatomy_type in anatomy_types:
+ self.transformed_contours[anatomy_type] = []
for i in range(self._num_time_points):📝 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
| if len(self.dynamic_labelmap_ids) > 0: | |
| anatomy_types = ["dynamic", "static"] | |
| else: | |
| anatomy_types = ["all"] | |
| if len(self.dynamic_labelmap_ids) > 0: | |
| anatomy_types = ["dynamic", "static"] | |
| else: | |
| anatomy_types = ["all"] | |
| for anatomy_type in anatomy_types: | |
| self.transformed_contours[anatomy_type] = [] | |
🤖 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_convert_image_to_usd.py` around lines 380 - 383,
`process()` is not idempotent because `_transform_all_contours()` appends to the
existing contour/time-sample collections on repeat runs, causing duplicate USD
data. Update the workflow in `workflow_convert_image_to_usd.py` so the
contour-related state is cleared or reinitialized before each transformation
pass, using the relevant fields touched by `_transform_all_contours()` and
`process()`, then rebuild the transformed contours from scratch before appending
new samples.
Comment on lines
+121
to
+123
| result_filename = workflow.process() | ||
|
|
||
| assert result_filename == "slicer_heart_small.all_painted.usd" |
Comment on lines
88
to
92
| print(f"Processing slice {i:03d}") | ||
| moving_image = itk.imread(str(data_dir / f"slice_{i:03d}.mha")) | ||
| result = seg.segment(moving_image, contrast_enhanced_study=True) | ||
| result = seg.segment(moving_image) | ||
| seg.set_contrast_enhanced_study(True) | ||
| labelmap_mask = result["labelmap"] |
Comment on lines
109
to
113
| if test_mode: | ||
| number_of_registration_iterations = 1 | ||
| else: | ||
| number_of_registration_iterations = 10 | ||
|
|
Comment on lines
423
to
431
| """ | ||
| Perform complete chest CT segmentation. | ||
|
|
||
| This is the main segmentation method that coordinates preprocessing, | ||
| segmentation, contrast agent detection (if applicable), postprocessing, | ||
| and anatomical group mask creation. | ||
| segmentation, subclass-specific labelmap refinement, and anatomical | ||
| group mask creation. | ||
|
|
||
| Args: | ||
| input_image (itk.image): The input 3D CT image to segment |
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
--upsampleoption (upsampling is always enabled).