ENH: Simplified naming of tutorials and updated their dirs#89
Open
aylward wants to merge 4 commits into
Open
Conversation
WalkthroughChangesTutorial workflow updates
BYOD statistical-model propagation
PhysicsNeMo BYOD training and evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PatientDataset
participant WorkflowFitStatisticalModelToPatient
participant WorkflowReconstructHighres4DCT
participant TransformTools
PatientDataset->>WorkflowFitStatisticalModelToPatient: provide reference image and labelmap
WorkflowFitStatisticalModelToPatient-->>TransformTools: fitted SSM mesh and surface
PatientDataset->>WorkflowReconstructHighres4DCT: provide gated phase images
WorkflowReconstructHighres4DCT-->>TransformTools: phase transforms
TransformTools-->>PatientDataset: per-phase mesh and surface outputs
Possibly related PRs
🚥 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
Comment on lines
+449
to
+450
| Tutorial 8: Fit the Cardiac SSM and Propagate Through Gated Phases | ||
| ================================================================= |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Sphinx title underline length.
The header underline is one character shorter than the title text (65 vs 66 characters). This will cause a Sphinx Title underline too short warning or build failure.
🐛 Proposed fix
Tutorial 8: Fit the Cardiac SSM and Propagate Through Gated Phases
-=================================================================
+==================================================================📝 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
| Tutorial 8: Fit the Cardiac SSM and Propagate Through Gated Phases | |
| ================================================================= | |
| Tutorial 8: Fit the Cardiac SSM and Propagate Through Gated Phases | |
| ================================================================== |
🤖 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 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.
Comment on lines
97
to
105
| parser.add_argument( | ||
| "--register-reference", | ||
| action="store_true", | ||
| default=False, | ||
| help="Register reference frame to fixed image (default: use identity)", | ||
| action=argparse.BooleanOptionalAction, | ||
| default=True, | ||
| help=( | ||
| "Register the reference time frame to the reference image " | ||
| "(default: enabled; use --no-register-reference for an identity transform)" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--register-reference default silently flips from disabled to enabled.
This switches --register-reference from store_true (default False) to BooleanOptionalAction with default=True. Existing invocations that omit this flag will now get different reconstruction behavior (reference time frame registered to the reference image, instead of an identity transform) without any change to their command line. This aligns the CLI with WorkflowReconstructHighres4DCT's own default, but is a breaking behavior change for anyone relying on the old implicit default — worth a callout in release notes/CHANGELOG.
🤖 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/cli/reconstruct_highres_4d_ct.py` around lines 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.
Comment on lines
+281
to
+282
| class TestTutorial08BYODFitModel: | ||
| """End-to-end test for tutorial_08_byod_fit_model_to_patients.py.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add missing pytest markers to the BYOD test class.
TestTutorial08BYODFitModel is missing the @pytest.mark.tutorial and @pytest.mark.slow markers used by the other tutorial tests in this file. Without these markers, this test will not be executed when the tutorial test suite is run (e.g., via pytest -m tutorial).
💻 Proposed fix
+@pytest.mark.tutorial
+@pytest.mark.slow
class TestTutorial08BYODFitModel:
"""End-to-end test for tutorial_08_byod_fit_model_to_patients.py."""📝 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
| class TestTutorial08BYODFitModel: | |
| """End-to-end test for tutorial_08_byod_fit_model_to_patients.py.""" | |
| `@pytest.mark.tutorial` | |
| `@pytest.mark.slow` | |
| class TestTutorial08BYODFitModel: | |
| """End-to-end test for tutorial_08_byod_fit_model_to_patients.py.""" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_tutorials.py` around lines 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.
Comment on lines
+181
to
211
| test_image_num = int(0.7 * len(input_filenames)) | ||
| test_image_path = output_dir / f"slice_{test_image_num:03d}_registered.mha" | ||
| if test_image_path.exists(): | ||
| test_image = itk.imread(str(test_image_path)) | ||
| screenshots.append( | ||
| tt.save_screenshot_image_slice( | ||
| test_image, | ||
| f"slice_{test_image_num:03d}_registered_test.png", | ||
| axis=0, | ||
| slice_fraction=0.5, | ||
| colormap="gray", | ||
| vmin=-200, | ||
| vmax=600, | ||
| ) | ||
| ) | ||
|
|
||
| test_labelmap_path = output_dir / f"slice_{test_image_num:03d}_labelmap.mha" | ||
| if test_labelmap_path.exists(): | ||
| test_labelmap = itk.imread(str(test_labelmap_path)) | ||
| screenshots.append( | ||
| tt.save_screenshot_image_slice( | ||
| test_image, | ||
| f"slice_{test_image_num:03d}_registered_test.png", | ||
| f"slice_{test_image_num:03d}_labelmap_test.png", | ||
| axis=0, | ||
| slice_fraction=0.5, | ||
| colormap="gray", | ||
| vmin=-200, | ||
| vmax=600, | ||
| overlay_mask=test_labelmap, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Screenshot check uses the wrong asset filename; will never trigger.
Since no dynamic_labelmap_ids is passed to WorkflowConvertImageToUSD here, the workflow runs the "all" registration path, and workflow_convert_image_to_usd.py's _register() writes slice_{i:03d}_all_registered.mha (via filename_prefix=f"slice_{i:03d}_all"), not slice_{i:03d}_registered.mha. test_image_path.exists() will therefore always be False, silently skipping the intended slice/labelmap screenshots.
Additionally, the nested labelmap check (line 197) is dead even if the filename above is fixed: per-frame slice_{i:03d}_labelmap.mha is only written inside the dynamic_labelmap_ids branch of _segment_and_register_frames (see workflow_convert_image_to_usd.py lines 271-284), which this tutorial never exercises.
🐛 Proposed fix for the registered-slice filename
-test_image_path = output_dir / f"slice_{test_image_num:03d}_registered.mha"
+test_image_path = output_dir / f"slice_{test_image_num:03d}_all_registered.mha"The nested test_labelmap_path block (lines 197-211) can be dropped for this tutorial (no dynamic/static split means no per-frame labelmap is ever exported), or kept only if you plan to pass dynamic_labelmap_ids in the future.
📝 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_image_num = int(0.7 * len(input_filenames)) | |
| test_image_path = output_dir / f"slice_{test_image_num:03d}_registered.mha" | |
| if test_image_path.exists(): | |
| test_image = itk.imread(str(test_image_path)) | |
| screenshots.append( | |
| tt.save_screenshot_image_slice( | |
| test_image, | |
| f"slice_{test_image_num:03d}_registered_test.png", | |
| axis=0, | |
| slice_fraction=0.5, | |
| colormap="gray", | |
| vmin=-200, | |
| vmax=600, | |
| ) | |
| ) | |
| test_labelmap_path = output_dir / f"slice_{test_image_num:03d}_labelmap.mha" | |
| if test_labelmap_path.exists(): | |
| test_labelmap = itk.imread(str(test_labelmap_path)) | |
| screenshots.append( | |
| tt.save_screenshot_image_slice( | |
| test_image, | |
| f"slice_{test_image_num:03d}_registered_test.png", | |
| f"slice_{test_image_num:03d}_labelmap_test.png", | |
| axis=0, | |
| slice_fraction=0.5, | |
| colormap="gray", | |
| vmin=-200, | |
| vmax=600, | |
| overlay_mask=test_labelmap, | |
| ) | |
| ) | |
| test_image_num = int(0.7 * len(input_filenames)) | |
| test_image_path = output_dir / f"slice_{test_image_num:03d}_all_registered.mha" | |
| if test_image_path.exists(): | |
| test_image = itk.imread(str(test_image_path)) | |
| screenshots.append( | |
| tt.save_screenshot_image_slice( | |
| test_image, | |
| f"slice_{test_image_num:03d}_registered_test.png", | |
| axis=0, | |
| slice_fraction=0.5, | |
| colormap="gray", | |
| vmin=-200, | |
| vmax=600, | |
| ) | |
| ) | |
| test_labelmap_path = output_dir / f"slice_{test_image_num:03d}_labelmap.mha" | |
| if test_labelmap_path.exists(): | |
| test_labelmap = itk.imread(str(test_labelmap_path)) | |
| screenshots.append( | |
| tt.save_screenshot_image_slice( | |
| test_image, | |
| f"slice_{test_image_num:03d}_labelmap_test.png", | |
| axis=0, | |
| slice_fraction=0.5, | |
| colormap="gray", | |
| vmin=-200, | |
| vmax=600, | |
| overlay_mask=test_labelmap, | |
| ) | |
| ) |
🤖 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 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.
Comment on lines
+141
to
+169
| try: | ||
| pv.start_xvfb() | ||
| except Exception: | ||
| pass | ||
|
|
||
| for mode_idx in range(mode_count): | ||
| sigma = float(np.sqrt(eigenvalues[mode_idx])) | ||
| mode_offsets = np.asarray(components[mode_idx]).reshape(-1, 3) | ||
|
|
||
| minus_mesh = mean_surface.copy() | ||
| minus_mesh.points = mean_points - 2.0 * sigma * mode_offsets | ||
| plus_mesh = mean_surface.copy() | ||
| plus_mesh.points = mean_points + 2.0 * sigma * mode_offsets | ||
|
|
||
| plotter = pv.Plotter(off_screen=True, window_size=[1200, 500], shape=(1, 3)) | ||
| plotter.subplot(0, 0) | ||
| plotter.add_mesh(minus_mesh, color="royalblue", opacity=0.9) | ||
| plotter.camera_position = "iso" | ||
| plotter.subplot(0, 1) | ||
| plotter.add_mesh(mean_surface, color="steelblue", opacity=0.9) | ||
| plotter.camera_position = "iso" | ||
| plotter.subplot(0, 2) | ||
| plotter.add_mesh(plus_mesh, color="coral", opacity=0.9) | ||
| plotter.camera_position = "iso" | ||
|
|
||
| png_path = output_dir / f"pca_mode_{mode_idx + 1:02d}.png" | ||
| plotter.screenshot(str(png_path)) | ||
| plotter.close() | ||
| screenshots.append(png_path) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Xvfb display started but never stopped — leaks a display process.
pv.start_xvfb() is called at line 142 with no matching pv.stop_xvfb(). Existing helpers in test_tools.py (save_screenshot_mesh, save_screenshot_openusd) always pair start_xvfb/stop_xvfb in a try/finally. Left unpaired here, this can leave an orphaned Xvfb process running, especially problematic when multiple tutorials run sequentially in one CI session.
🔧 Proposed fix to stop Xvfb after rendering
-try:
- pv.start_xvfb()
-except Exception:
- pass
-
-for mode_idx in range(mode_count):
- ...
- screenshots.append(png_path)
+xvfb_started = False
+try:
+ pv.start_xvfb()
+ xvfb_started = True
+except Exception:
+ pass
+
+try:
+ for mode_idx in range(mode_count):
+ ...
+ screenshots.append(png_path)
+finally:
+ if xvfb_started and hasattr(pv, "stop_xvfb"):
+ pv.stop_xvfb()📝 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
| try: | |
| pv.start_xvfb() | |
| except Exception: | |
| pass | |
| for mode_idx in range(mode_count): | |
| sigma = float(np.sqrt(eigenvalues[mode_idx])) | |
| mode_offsets = np.asarray(components[mode_idx]).reshape(-1, 3) | |
| minus_mesh = mean_surface.copy() | |
| minus_mesh.points = mean_points - 2.0 * sigma * mode_offsets | |
| plus_mesh = mean_surface.copy() | |
| plus_mesh.points = mean_points + 2.0 * sigma * mode_offsets | |
| plotter = pv.Plotter(off_screen=True, window_size=[1200, 500], shape=(1, 3)) | |
| plotter.subplot(0, 0) | |
| plotter.add_mesh(minus_mesh, color="royalblue", opacity=0.9) | |
| plotter.camera_position = "iso" | |
| plotter.subplot(0, 1) | |
| plotter.add_mesh(mean_surface, color="steelblue", opacity=0.9) | |
| plotter.camera_position = "iso" | |
| plotter.subplot(0, 2) | |
| plotter.add_mesh(plus_mesh, color="coral", opacity=0.9) | |
| plotter.camera_position = "iso" | |
| png_path = output_dir / f"pca_mode_{mode_idx + 1:02d}.png" | |
| plotter.screenshot(str(png_path)) | |
| plotter.close() | |
| screenshots.append(png_path) | |
| xvfb_started = False | |
| try: | |
| pv.start_xvfb() | |
| xvfb_started = True | |
| except Exception: | |
| pass | |
| try: | |
| for mode_idx in range(mode_count): | |
| sigma = float(np.sqrt(eigenvalues[mode_idx])) | |
| mode_offsets = np.asarray(components[mode_idx]).reshape(-1, 3) | |
| minus_mesh = mean_surface.copy() | |
| minus_mesh.points = mean_points - 2.0 * sigma * mode_offsets | |
| plus_mesh = mean_surface.copy() | |
| plus_mesh.points = mean_points + 2.0 * sigma * mode_offsets | |
| plotter = pv.Plotter(off_screen=True, window_size=[1200, 500], shape=(1, 3)) | |
| plotter.subplot(0, 0) | |
| plotter.add_mesh(minus_mesh, color="royalblue", opacity=0.9) | |
| plotter.camera_position = "iso" | |
| plotter.subplot(0, 1) | |
| plotter.add_mesh(mean_surface, color="steelblue", opacity=0.9) | |
| plotter.camera_position = "iso" | |
| plotter.subplot(0, 2) | |
| plotter.add_mesh(plus_mesh, color="coral", opacity=0.9) | |
| plotter.camera_position = "iso" | |
| png_path = output_dir / f"pca_mode_{mode_idx + 1:02d}.png" | |
| plotter.screenshot(str(png_path)) | |
| plotter.close() | |
| screenshots.append(png_path) | |
| finally: | |
| if xvfb_started and hasattr(pv, "stop_xvfb"): | |
| pv.stop_xvfb() |
🤖 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_04_heart_create_statistical_model.py` around lines 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.
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
New Features
Improvements
torch-scatter>=2.1.0.Tests