ENH: Simplified naming of tutorials and updated their dirs#89
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesTutorial workflow consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Tutorial08
participant WorkflowFitStatisticalModelToPatient
participant WorkflowReconstructHighres4DCT
participant WorkflowTrainPhysicsNeMo
participant WorkflowInferPhysicsNeMo
Tutorial08->>WorkflowFitStatisticalModelToPatient: fit the patient reference model
Tutorial08->>WorkflowReconstructHighres4DCT: reconstruct gated phases
WorkflowFitStatisticalModelToPatient-->>WorkflowTrainPhysicsNeMo: provide fitted mesh outputs
WorkflowTrainPhysicsNeMo->>WorkflowInferPhysicsNeMo: provide saved checkpoint
WorkflowInferPhysicsNeMo-->>Tutorial08: return held-out inference outputs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tutorials/tutorial_08_byod_fit_model_to_patients.py`:
- Around line 2-10: Update the tutorial title and downstream references in the
module documentation to use the renamed tutorial_08_* and tutorial_09_*
identifiers instead of the stale 08cd, 09c, and 09d names; apply the same
correction to the additional referenced documentation section while keeping the
pipeline description factual.
- Around line 194-215: Update the workflow construction in the RECOMPUTE block
to configure WorkflowReconstructHighres4DCT so the first gated phase is
registered to ref_image rather than treated as the identity frame. Preserve the
existing time_series and registration method setup while overriding the default
reference-frame behavior with the appropriate register_reference configuration.
In `@tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py`:
- Around line 43-51: Update tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py
lines 43-51: remove the “9c” reference from the run-directory comment and change
DEFAULT_OUT_DIR to use the tutorial_10_byod_mgn directory. Update
tutorials/tutorial_10_byod_eval_physicsnemo_mlp.py lines 43-51 by removing the
stale “9d” reference from its corresponding comment.
🪄 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: 62fc732b-d008-4e7e-a72f-a3bc51d0dcd1
📒 Files selected for processing (13)
pyproject.tomltutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_01_lung_gated_ct_to_usd.pytutorials/tutorial_02_heart_ct_to_vtk.pytutorials/tutorial_03_heart_vtk_to_usd.pytutorials/tutorial_04_heart_create_statistical_model.pytutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.pytutorials/tutorial_06_lung_reconstruct_highres_4d_ct.pytutorials/tutorial_08_byod_fit_model_to_patients.pytutorials/tutorial_09_byod_train_physicsnemo_mgn.pytutorials/tutorial_09_byod_train_physicsnemo_mlp.pytutorials/tutorial_10_byod_eval_physicsnemo_mgn.pytutorials/tutorial_10_byod_eval_physicsnemo_mlp.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@tutorials/tutorial_08_byod_fit_model_to_patients.py`:
- Around line 2-10: Update the tutorial title and downstream references in the
module documentation to use the renamed tutorial_08_* and tutorial_09_*
identifiers instead of the stale 08cd, 09c, and 09d names; apply the same
correction to the additional referenced documentation section while keeping the
pipeline description factual.
- Around line 194-215: Update the workflow construction in the RECOMPUTE block
to configure WorkflowReconstructHighres4DCT so the first gated phase is
registered to ref_image rather than treated as the identity frame. Preserve the
existing time_series and registration method setup while overriding the default
reference-frame behavior with the appropriate register_reference configuration.
In `@tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py`:
- Around line 43-51: Update tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py
lines 43-51: remove the “9c” reference from the run-directory comment and change
DEFAULT_OUT_DIR to use the tutorial_10_byod_mgn directory. Update
tutorials/tutorial_10_byod_eval_physicsnemo_mlp.py lines 43-51 by removing the
stale “9d” reference from its corresponding comment.
🪄 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: 62fc732b-d008-4e7e-a72f-a3bc51d0dcd1
📒 Files selected for processing (13)
pyproject.tomltutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_01_lung_gated_ct_to_usd.pytutorials/tutorial_02_heart_ct_to_vtk.pytutorials/tutorial_03_heart_vtk_to_usd.pytutorials/tutorial_04_heart_create_statistical_model.pytutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.pytutorials/tutorial_06_lung_reconstruct_highres_4d_ct.pytutorials/tutorial_08_byod_fit_model_to_patients.pytutorials/tutorial_09_byod_train_physicsnemo_mgn.pytutorials/tutorial_09_byod_train_physicsnemo_mlp.pytutorials/tutorial_10_byod_eval_physicsnemo_mgn.pytutorials/tutorial_10_byod_eval_physicsnemo_mlp.py
🛑 Comments failed to post (3)
tutorials/tutorial_08_byod_fit_model_to_patients.py (2)
2-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale tutorial names.
The title and downstream references still use the old
08cd/09c/09dnaming, while this PR renames them totutorial_08_*andtutorial_09_*.Proposed documentation update
-Tutorial 8cd: Fit the Cardiac SSM and Propagate It Through Gated Phases +Tutorial 08: Fit the Cardiac SSM and Propagate It Through Gated Phases -First stage of the cardiac 4D deep-learning pipeline (Tutorials 08cd -> 09c/09d --> 10c/10d). +First stage of the cardiac 4D deep-learning pipeline +(Tutorials 08 -> 09 -> 10). -``tutorial_09c_byod_train_physicsnemo_mgn.py`` / -``tutorial_09d_byod_train_physicsnemo_mlp.py``) +``tutorial_09_byod_train_physicsnemo_mgn.py`` / +``tutorial_09_byod_train_physicsnemo_mlp.py``) -# this is also the directory the Tutorial 9c/9d trainers read from. +# this is also the directory the Tutorial 09 trainers read from.As per coding guidelines, keep documentation claims factual.
Also applies to: 85-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_08_byod_fit_model_to_patients.py` around lines 2 - 10, Update the tutorial title and downstream references in the module documentation to use the renamed tutorial_08_* and tutorial_09_* identifiers instead of the stale 08cd, 09c, and 09d names; apply the same correction to the additional referenced documentation section while keeping the pipeline description factual.Source: Coding guidelines
194-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Register the first gated image instead of treating it as identity.
The reference image is excluded from
time_series, but the workflow defaults to
reference_frame=0, register_reference=False. Consequently, the first gated phase
receives an identity transform rather than registration toref_image.Proposed fix
reg_workflow = WorkflowReconstructHighres4DCT( time_series_images=time_series, fixed_image=ref_image, + register_reference=True, registration_method=icon_registration_method, )📝 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.gated_files = sorted( file for file in patient_dir.glob("*.nii.gz") if file != ref_image_file and "nop" not in file.name and "_g" in file.stem ) time_series = [] time_series_ids = [] for gated_file in gated_files: time_series.append(itk.imread(str(gated_file))) time_id = gated_file.name.split("_g")[1][:3] time_series_ids.append(time_id) if RECOMPUTE: icon_registration_method = RegisterImagesICON() icon_registration_method.set_weights_path(str(ICON_WEIGHTS_PATH)) icon_registration_method.set_number_of_iterations(None) reg_workflow = WorkflowReconstructHighres4DCT( time_series_images=time_series, fixed_image=ref_image, register_reference=True, registration_method=icon_registration_method, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_08_byod_fit_model_to_patients.py` around lines 194 - 215, Update the workflow construction in the RECOMPUTE block to configure WorkflowReconstructHighres4DCT so the first gated phase is registered to ref_image rather than treated as the identity frame. Preserve the existing time_series and registration method setup while overriding the default reference-frame behavior with the appropriate register_reference configuration.tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py (1)
43-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update stale tutorial references and standardize output directory naming.
The PR successfully simplified the tutorial names and output paths, but these comments still reference the old
9c/9dnumbering. Additionally, the MGN script's default output directory should include the_byod_prefix for consistency with the MLP script (tutorial_10_byod_mlp).
tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py#L43-L51: Update the comment to remove "9c", and standardizeDEFAULT_OUT_DIRto usetutorial_10_byod_mgn.tutorials/tutorial_10_byod_eval_physicsnemo_mlp.py#L43-L51: Update the comment to remove "9d".📍 Affects 2 files
tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py#L43-L51(this comment)tutorials/tutorial_10_byod_eval_physicsnemo_mlp.py#L43-L51🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py` around lines 43 - 51, Update tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py lines 43-51: remove the “9c” reference from the run-directory comment and change DEFAULT_OUT_DIR to use the tutorial_10_byod_mgn directory. Update tutorials/tutorial_10_byod_eval_physicsnemo_mlp.py lines 43-51 by removing the stale “9d” reference from its corresponding comment.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/quickstart.rst (1)
276-279: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the outdated tutorial numbering.
The
9a/9b/10a/10bvariant suffixes were removed in this PR. Please update this reference to match the consolidated tutorials to avoid confusing users.📝 Proposed fix
bring-your-own-data cardiac tutorials; see :doc:`tutorials` for their dataset -layout. Tutorials 9a/9b/10a/10b additionally require the optional +layout. Tutorials 9 and 10 additionally require the optional ``physicsnemo`` extra (``pip install "physiotwin4d[physicsnemo]"``); PhysicsNeMo itself requires Python >= 3.11.🤖 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/quickstart.rst` around lines 276 - 279, Update the tutorial reference in the quickstart documentation to use the consolidated tutorial numbering instead of the removed 9a/9b/10a/10b suffixes, while preserving the existing physicsnemo installation and Python version requirements.docs/tutorials.rst (1)
89-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the outdated tutorial numbering.
The
1a-5atutorial numbering scheme was removed in this PR. Please update this user-facing string to match the consolidated numbering.📝 Proposed fix
-This covers the data used by Tutorials 1a-5a. Run +This covers the data used by Tutorials 1-5. Run ``physiotwin4d-download-data --help`` for all options, and see🤖 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/tutorials.rst` around lines 89 - 90, Update the user-facing tutorial reference in the surrounding documentation text to use the consolidated numbering scheme instead of the removed “Tutorials 1a-5a” wording, while leaving the download command and surrounding guidance unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tutorials.rst`:
- Around line 449-450: Adjust the underline beneath “Tutorial 8: Fit the Cardiac
SSM and Propagate Through Gated Phases” to exactly match the title’s length,
preserving the existing section heading style.
In `@src/physiotwin4d/cli/reconstruct_highres_4d_ct.py`:
- Around line 97-105: Restore the --register-reference CLI default to disabled
so invocations that omit the flag preserve the previous identity-transform
behavior. Update the argparse definition for --register-reference, including its
help text, to accurately describe the disabled default and the explicit option
that enables registration; record the breaking default change in the release
notes or CHANGELOG.
In `@tests/test_tutorials.py`:
- Around line 281-282: Add `@pytest.mark.tutorial` and `@pytest.mark.slow`
decorators to the TestTutorial08BYODFitModel class, matching the marker pattern
used by the other tutorial test classes so it is selected by the tutorial and
slow test filters.
In `@tutorials/tutorial_01_lung_gated_ct_to_usd.py`:
- Around line 181-211: Update the screenshot asset lookup in the tutorial to use
the all-registration filename produced by WorkflowConvertImageToUSD, including
the “all” prefix in test_image_path so the registered slice screenshot executes.
Remove the nested test_labelmap_path screenshot block because this tutorial does
not pass dynamic_labelmap_ids and therefore does not produce per-frame labelmap
files.
In `@tutorials/tutorial_04_heart_create_statistical_model.py`:
- Around line 141-169: Pair the Xvfb startup in the tutorial’s rendering flow
with cleanup by wrapping the mode-rendering loop and screenshot generation in a
try/finally block, then call pv.stop_xvfb() in the finally clause. Preserve the
existing rendering behavior and ensure cleanup runs even when screenshot
creation or plotting raises an exception.
---
Outside diff comments:
In `@docs/quickstart.rst`:
- Around line 276-279: Update the tutorial reference in the quickstart
documentation to use the consolidated tutorial numbering instead of the removed
9a/9b/10a/10b suffixes, while preserving the existing physicsnemo installation
and Python version requirements.
In `@docs/tutorials.rst`:
- Around line 89-90: Update the user-facing tutorial reference in the
surrounding documentation text to use the consolidated numbering scheme instead
of the removed “Tutorials 1a-5a” wording, while leaving the download command and
surrounding guidance unchanged.
🪄 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: ad0d4285-eebf-4321-b095-f88a20d11b39
📒 Files selected for processing (23)
docs/architecture.rstdocs/index.rstdocs/quickstart.rstdocs/tutorials.rstpyproject.tomlsrc/physiotwin4d/cli/reconstruct_highres_4d_ct.pysrc/physiotwin4d/workflow_convert_image_to_usd.pysrc/physiotwin4d/workflow_reconstruct_highres_4d_ct.pytests/test_tutorials.pytests/test_workflow_reconstruct_highres_4d_ct.pytutorials/README.mdtutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_01_lung_gated_ct_to_usd.pytutorials/tutorial_02_heart_ct_to_vtk.pytutorials/tutorial_03_heart_vtk_to_usd.pytutorials/tutorial_04_heart_create_statistical_model.pytutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.pytutorials/tutorial_06_lung_reconstruct_highres_4d_ct.pytutorials/tutorial_08_byod_fit_model_to_patients.pytutorials/tutorial_09_byod_train_physicsnemo_mgn.pytutorials/tutorial_09_byod_train_physicsnemo_mlp.pytutorials/tutorial_10_byod_eval_physicsnemo_mgn.pytutorials/tutorial_10_byod_eval_physicsnemo_mlp.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tutorials/tutorial_06_lung_reconstruct_highres_4d_ct.py
- tutorials/tutorial_02_heart_ct_to_vtk.py
- tutorials/tutorial_09_byod_train_physicsnemo_mlp.py
- tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py
- tutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.py
- tutorials/tutorial_09_byod_train_physicsnemo_mgn.py
- tutorials/tutorial_01_heart_gated_ct_to_usd.py
- tutorials/tutorial_08_byod_fit_model_to_patients.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/physiotwin4d/segment_chest_total_segmentator.py (1)
296-300: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCentralize the
gpu:0device literal. It’s repeated across the five TotalSegmentator calls, so a shared constant or attribute would reduce drift if this value changes later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/segment_chest_total_segmentator.py` around lines 296 - 300, Define one shared constant or class attribute for the TotalSegmentator device value, then replace the repeated "gpu:0" literals in all five TotalSegmentator calls with that symbol. Keep the existing device behavior unchanged and apply the same symbol consistently across the calls.
🤖 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 `@tutorials/tutorial_01_lung_gated_ct_to_usd.py`:
- Around line 172-201: The screenshot checks use the wrong output filename and
include an unsupported labelmap branch. In
tutorials/tutorial_01_lung_gated_ct_to_usd.py lines 172-201 and
tutorials/tutorial_01_heart_gated_ct_to_usd.py lines 167-197, update
test_image_path to use the _all_registered.mha suffix, and remove the nested
test_labelmap_path existence check and its screenshot block.
In `@tutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.py`:
- Around line 59-64: Update the data directory selection in tutorial 05 to
always use repo_root / "data" / "DirLab-4DCT", removing the test_mode-dependent
data/test path while leaving patient_image_file construction unchanged.
---
Nitpick comments:
In `@src/physiotwin4d/segment_chest_total_segmentator.py`:
- Around line 296-300: Define one shared constant or class attribute for the
TotalSegmentator device value, then replace the repeated "gpu:0" literals in all
five TotalSegmentator calls with that symbol. Keep the existing device behavior
unchanged and apply the same symbol consistently across the calls.
🪄 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: 74afe6d5-9c60-454a-8e4b-181faf0adaa1
📒 Files selected for processing (13)
src/physiotwin4d/segment_chest_total_segmentator.pytutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_01_lung_gated_ct_to_usd.pytutorials/tutorial_02_heart_ct_to_vtk.pytutorials/tutorial_03_heart_vtk_to_usd.pytutorials/tutorial_04_heart_create_statistical_model.pytutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.pytutorials/tutorial_06_lung_reconstruct_highres_4d_ct.pytutorials/tutorial_08_byod_fit_model_to_patients.pytutorials/tutorial_09_byod_train_physicsnemo_mgn.pytutorials/tutorial_09_byod_train_physicsnemo_mlp.pytutorials/tutorial_10_byod_eval_physicsnemo_mgn.pytutorials/tutorial_10_byod_eval_physicsnemo_mlp.py
💤 Files with no reviewable changes (4)
- tutorials/tutorial_09_byod_train_physicsnemo_mgn.py
- tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py
- tutorials/tutorial_10_byod_eval_physicsnemo_mlp.py
- tutorials/tutorial_09_byod_train_physicsnemo_mlp.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tutorials/tutorial_04_heart_create_statistical_model.py
- tutorials/tutorial_03_heart_vtk_to_usd.py
- tutorials/tutorial_02_heart_ct_to_vtk.py
- tutorials/tutorial_08_byod_fit_model_to_patients.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/physiotwin4d/segment_chest_total_segmentator.py (1)
296-300: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCentralize the
gpu:0device literal. It’s repeated across the five TotalSegmentator calls, so a shared constant or attribute would reduce drift if this value changes later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/segment_chest_total_segmentator.py` around lines 296 - 300, Define one shared constant or class attribute for the TotalSegmentator device value, then replace the repeated "gpu:0" literals in all five TotalSegmentator calls with that symbol. Keep the existing device behavior unchanged and apply the same symbol consistently across the calls.
🤖 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 `@tutorials/tutorial_01_lung_gated_ct_to_usd.py`:
- Around line 172-201: The screenshot checks use the wrong output filename and
include an unsupported labelmap branch. In
tutorials/tutorial_01_lung_gated_ct_to_usd.py lines 172-201 and
tutorials/tutorial_01_heart_gated_ct_to_usd.py lines 167-197, update
test_image_path to use the _all_registered.mha suffix, and remove the nested
test_labelmap_path existence check and its screenshot block.
In `@tutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.py`:
- Around line 59-64: Update the data directory selection in tutorial 05 to
always use repo_root / "data" / "DirLab-4DCT", removing the test_mode-dependent
data/test path while leaving patient_image_file construction unchanged.
---
Nitpick comments:
In `@src/physiotwin4d/segment_chest_total_segmentator.py`:
- Around line 296-300: Define one shared constant or class attribute for the
TotalSegmentator device value, then replace the repeated "gpu:0" literals in all
five TotalSegmentator calls with that symbol. Keep the existing device behavior
unchanged and apply the same symbol consistently across the calls.
🪄 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: 74afe6d5-9c60-454a-8e4b-181faf0adaa1
📒 Files selected for processing (13)
src/physiotwin4d/segment_chest_total_segmentator.pytutorials/tutorial_01_heart_gated_ct_to_usd.pytutorials/tutorial_01_lung_gated_ct_to_usd.pytutorials/tutorial_02_heart_ct_to_vtk.pytutorials/tutorial_03_heart_vtk_to_usd.pytutorials/tutorial_04_heart_create_statistical_model.pytutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.pytutorials/tutorial_06_lung_reconstruct_highres_4d_ct.pytutorials/tutorial_08_byod_fit_model_to_patients.pytutorials/tutorial_09_byod_train_physicsnemo_mgn.pytutorials/tutorial_09_byod_train_physicsnemo_mlp.pytutorials/tutorial_10_byod_eval_physicsnemo_mgn.pytutorials/tutorial_10_byod_eval_physicsnemo_mlp.py
💤 Files with no reviewable changes (4)
- tutorials/tutorial_09_byod_train_physicsnemo_mgn.py
- tutorials/tutorial_10_byod_eval_physicsnemo_mgn.py
- tutorials/tutorial_10_byod_eval_physicsnemo_mlp.py
- tutorials/tutorial_09_byod_train_physicsnemo_mlp.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tutorials/tutorial_04_heart_create_statistical_model.py
- tutorials/tutorial_03_heart_vtk_to_usd.py
- tutorials/tutorial_02_heart_ct_to_vtk.py
- tutorials/tutorial_08_byod_fit_model_to_patients.py
🛑 Comments failed to post (2)
tutorials/tutorial_01_lung_gated_ct_to_usd.py (1)
172-201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Screenshot existence check targets the wrong filename in both Tutorial 1 scripts; root cause is the missing
_allsuffix. Neither script passesdynamic_labelmap_idstoWorkflowConvertImageToUSD, so the workflow's "all" path writesslice_{i:03d}_all_registered.mha, but both scripts check forslice_{i:03d}_registered.mha— alwaysFalse, silently skipping the registered-slice and nested labelmap screenshots.
tutorials/tutorial_01_lung_gated_ct_to_usd.py#L172-L201: append_alltotest_image_path(slice_{test_image_num:03d}_all_registered.mha) and drop the nestedtest_labelmap_pathblock (no per-frame labelmap is produced on this path).tutorials/tutorial_01_heart_gated_ct_to_usd.py#L167-L197: apply the same_allfilename fix and drop the nestedtest_labelmap_pathblock.📍 Affects 2 files
tutorials/tutorial_01_lung_gated_ct_to_usd.py#L172-L201(this comment)tutorials/tutorial_01_heart_gated_ct_to_usd.py#L167-L197🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_01_lung_gated_ct_to_usd.py` around lines 172 - 201, The screenshot checks use the wrong output filename and include an unsupported labelmap branch. In tutorials/tutorial_01_lung_gated_ct_to_usd.py lines 172-201 and tutorials/tutorial_01_heart_gated_ct_to_usd.py lines 167-197, update test_image_path to use the _all_registered.mha suffix, and remove the nested test_labelmap_path existence check and its screenshot block.tutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.py (1)
59-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: List phase files present in the DirLab-4DCT test fixture. fd -t f "Case1Pack_T" data/test/DirLab-4DCT 2>/dev/null || echo "test data dir not present in this sandbox"Repository: Project-MONAI/physiotwin4d
Length of output: 206
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n[git ls-files matching DirLab-4DCT]\n' git ls-files | rg '^data/.*/DirLab-4DCT|DirLab-4DCT' printf '\n[files around the tutorial]\n' git ls-files tutorials | rg 'tutorial_05_heart_to_lung_fit_statistical_model_to_patient\.py|tutorial_01|DirLab' printf '\n[inspect tutorial 05]\n' sed -n '1,140p' tutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.py printf '\n[search for Case1Pack_T70 references]\n' rg -n 'Case1Pack_T70|DirLab-4DCT|running_as_test\(\)' tutorials data . 2>/dev/null || trueRepository: Project-MONAI/physiotwin4d
Length of output: 27860
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n[data/DirLab-4DCT/README.md]\n' sed -n '1,140p' data/DirLab-4DCT/README.md printf '\n[tutorial 01 excerpt]\n' sed -n '90,150p' tutorials/tutorial_01_lung_gated_ct_to_usd.py printf '\n[tests/test_tutorials.py excerpt around DirLab]\n' sed -n '360,410p' tests/test_tutorials.py printf '\n[check whether data/test/DirLab-4DCT is tracked]\n' git ls-files data/test 2>/dev/null || trueRepository: Project-MONAI/physiotwin4d
Length of output: 9899
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n[data/test/README.md]\n' sed -n '1,200p' data/test/README.md printf '\n[tests/test_tutorials.py tutorial 05 references]\n' rg -n 'tutorial_05_heart_to_lung_fit_statistical_model_to_patient|tutorial 05|Heart to Lung' tests/test_tutorials.py tutorials -A 6 -B 6 printf '\n[data/test tree entries if present]\n' find data/test -maxdepth 3 -type f | sort | sed -n '1,200p'Repository: Project-MONAI/physiotwin4d
Length of output: 7394
Keep tutorial 05 on
data/DirLab-4DCT
data/testis a pytest cache for Slicer-Heart data, not a tutorial dataset, so switching todata/test/DirLab-4DCTwill make this tutorial fail in test mode. If a smaller fixture is needed, add one explicitly instead of reusingdata/test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_05_heart_to_lung_fit_statistical_model_to_patient.py` around lines 59 - 64, Update the data directory selection in tutorial 05 to always use repo_root / "data" / "DirLab-4DCT", removing the test_mode-dependent data/test path while leaving patient_image_file construction unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tutorials/tutorial_09_byod_train_physicsnemo_mgn.py`:
- Around line 93-94: Update the RESUME_FROM assignment to verify that the
checkpoint file exists before using it; otherwise set RESUME_FROM to None so
_load_resume_checkpoint() does not call torch.load() on a missing file during a
fresh run.
🪄 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: a20b3d7d-c068-445c-81c0-919a9723d661
📒 Files selected for processing (4)
src/physiotwin4d/segment_chest_total_segmentator.pysrc/physiotwin4d/workflow_infer_physicsnemo.pysrc/physiotwin4d/workflow_train_physicsnemo.pytutorials/tutorial_09_byod_train_physicsnemo_mgn.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/physiotwin4d/segment_chest_total_segmentator.py
Comment on lines
+93
to
94
| RESUME_FROM = str(OUTPUT_DIR / "mgn_stage_model_epoch_00100.pt") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent FileNotFoundError when running the tutorial from scratch.
Hardcoding RESUME_FROM to an output checkpoint inside OUTPUT_DIR will cause the tutorial to crash on the first run if the file does not yet exist. Because torch.load() is executed blindly inside _load_resume_checkpoint() when a path is supplied, missing files throw a FileNotFoundError.
Ensure the script checks for the file's existence before assigning the path, or safely set it to None.
🛠️ Proposed fix
- RESUME_FROM = str(OUTPUT_DIR / "mgn_stage_model_epoch_00100.pt")
+ _resume_ckpt = OUTPUT_DIR / "mgn_stage_model_epoch_00100.pt"
+ RESUME_FROM = str(_resume_ckpt) if _resume_ckpt.exists() else None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RESUME_FROM = str(OUTPUT_DIR / "mgn_stage_model_epoch_00100.pt") | |
| _resume_ckpt = OUTPUT_DIR / "mgn_stage_model_epoch_00100.pt" | |
| RESUME_FROM = str(_resume_ckpt) if _resume_ckpt.exists() else None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tutorials/tutorial_09_byod_train_physicsnemo_mgn.py` around lines 93 - 94,
Update the RESUME_FROM assignment to verify that the checkpoint file exists
before using it; otherwise set RESUME_FROM to None so _load_resume_checkpoint()
does not call torch.load() on a missing file during a fresh run.
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