ENH: Reorg tutorials and create PhysicsNeMo workflows#88
Conversation
|
Warning Review limit reached
Next review available in: 43 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 (1)
WalkthroughAdds reusable PhysicsNeMo MGN/MLP training and inference workflows, shared data utilities, CLI entry points, BYOD tutorial drivers, segmentation updates, and revised tutorial documentation, numbering, scripts, and end-to-end tests. ChangesPhysicsNeMo pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Driver as BYOD tutorial driver
participant Train as PhysicsNeMo training workflow
participant Model as Checkpoint artifacts
participant Infer as PhysicsNeMo inference workflow
Driver->>Train: Build manifests and start training
Train->>Model: Save weights and metadata
Driver->>Infer: Evaluate held-out subjects
Infer->>Model: Load weights and normalization assets
Infer-->>Driver: Return predicted surfaces and statistics
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: 6
🧹 Nitpick comments (8)
tests/test_tutorials.py (1)
83-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign
_class_namewith the script'sTestToolsinstantiation.
TestTutorial01aHeartGatedCTToUSDuses"tutorial_01a", while the tutorial script instantiates itsTestToolswith"tutorial_01a_heart_gated_ct_to_usd". Although this works if the baseline directory is strictly namedtutorial_01a, the other tests (liketutorial_03_vtk_to_usd) use the full script name for their_class_name. Consider aligning them for consistency (and ensure the baseline directory name matches the update).💡 Proposed refactor
- _class_name = "tutorial_01a" + _class_name = "tutorial_01a_heart_gated_ct_to_usd"🤖 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` at line 83, Update the _class_name in TestTutorial01aHeartGatedCTToUSD to match the tutorial script’s TestTools identifier, “tutorial_01a_heart_gated_ct_to_usd”, and rename or align the corresponding baseline directory with this value.docs/tutorials.rst (1)
329-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder sections to maintain numerical sequence.
The tutorial renaming effectively reassigned the identifiers for the PCA workflows and the VTK-to-USD conversion, leaving "Tutorial 3" physically positioned after "Tutorial 5a" in both the documentation and the test suite. This makes reading the files top-to-bottom somewhat confusing.
docs/tutorials.rst#L329-L333: Move the Tutorial 3 documentation block up to appear before Tutorial 4a (above line 235).tests/test_tutorials.py#L196-L205: Move theTestTutorial03VTKToUSDtest class up to appear beforeTestTutorial04aCreateStatisticalModel(above line 126).🤖 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 329 - 333, Reorder the Tutorial 3 documentation block in docs/tutorials.rst to appear before Tutorial 4a, and move the TestTutorial03VTKToUSD class in tests/test_tutorials.py before TestTutorial04aCreateStatisticalModel, preserving each block’s contents unchanged.tutorials/tutorial_10c_byod_eval_physicsnemo_mgn.py (1)
54-85: 📐 Maintainability & Code Quality | 🔵 Trivial
_gating_stage_from_filenameand_write_subject_manifestare duplicated verbatim between the MGN and MLP eval tutorials. Same root cause: both files independently implement identical gating-stage parsing and manifest-building logic.
tutorials/tutorial_10c_byod_eval_physicsnemo_mgn.py#L54-L85: extract these two helpers intophysicsnemo_tools.py(already imported elsewhere aspnt) and import from there.tutorials/tutorial_10d_byod_eval_physicsnemo_mlp.py#L54-L85: import the same shared helpers instead of redefining them.🤖 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_10c_byod_eval_physicsnemo_mgn.py` around lines 54 - 85, Extract _gating_stage_from_filename and _write_subject_manifest from tutorials/tutorial_10c_byod_eval_physicsnemo_mgn.py lines 54-85 into the shared physicsnemo_tools.py module, preserving their behavior and required dependencies; update the MGN tutorial to use the shared helpers through its existing pnt import. In tutorials/tutorial_10d_byod_eval_physicsnemo_mlp.py lines 54-85, remove the duplicate helper definitions and import/use the same shared helpers instead.Source: Coding guidelines
src/physiotwin4d/cli/train_physicsnemo.py (1)
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
workflow: objectforces# type: ignore[attr-defined]on every call.Both
WorkflowTrainPhysicsNeMoMGNandWorkflowTrainPhysicsNeMoMLPinherit the same setters fromWorkflowTrainPhysicsNeMo; typing the parameter as that shared base removes the need for the ignore comments and gives mypy real checking here, better matching the "full type hints compatible with strict mypy" convention.As per coding guidelines, "use full type hints compatible with strict mypy."♻️ Proposed fix
+from ..workflow_train_physicsnemo import WorkflowTrainPhysicsNeMo + -def _apply_common(workflow: object, args: argparse.Namespace) -> None: +def _apply_common(workflow: WorkflowTrainPhysicsNeMo, args: argparse.Namespace) -> None: """Apply the shared tuning setters when supplied on the command line.""" if args.epochs is not None: - workflow.set_epochs(args.epochs) # type: ignore[attr-defined] + workflow.set_epochs(args.epochs) if args.batch_size is not None: - workflow.set_batch_size(args.batch_size) # type: ignore[attr-defined] + workflow.set_batch_size(args.batch_size) if args.learning_rate is not None: - workflow.set_learning_rate(args.learning_rate) # type: ignore[attr-defined] + workflow.set_learning_rate(args.learning_rate) if args.cache_size is not None: - workflow.set_cache_size(args.cache_size) # type: ignore[attr-defined] + workflow.set_cache_size(args.cache_size)🤖 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/train_physicsnemo.py` around lines 13 - 22, Update the _apply_common workflow parameter from object to the shared WorkflowTrainPhysicsNeMo base type, importing that class from the established module. Remove the # type: ignore[attr-defined] comments so mypy checks the inherited setter calls directly.Source: Coding guidelines
src/physiotwin4d/workflow_train_physicsnemo.py (3)
148-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
< 1/<= 0validation pattern across setters.
set_epochs,set_batch_size,set_cache_size(and the subclass setters forprocessor_size,hidden_dim,num_layers,layer_size) all repeat the same "raise ValueError if below threshold" pattern. A small_require_at_least(name, value, minimum)helper would remove the repetition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/workflow_train_physicsnemo.py` around lines 148 - 172, Introduce a shared _require_at_least(name, value, minimum) validation helper and update set_epochs, set_batch_size, set_cache_size, and the subclass setters for processor_size, hidden_dim, num_layers, and layer_size to use it, preserving each setter’s existing thresholds, error conditions, and assignments.
50-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImporting the private
_Sampleacross module boundaries.
_Sampleis underscore-prefixed inphysicsnemo_tools.py, signaling internal use, yet it's imported and used directly here as a public-facing type (list[_Sample]). Consider exposing it (drop the leading underscore) or havingphysicsnemo_toolsprovide a factory so the private-name convention isn't broken across files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/workflow_train_physicsnemo.py` around lines 50 - 53, Stop importing and exposing the private _Sample type from workflow_train_physicsnemo.py; rename or otherwise expose the sample type through physicsnemo_tools.py, then update its definition and all references, including the list[_Sample] annotation, to use the public symbol consistently.
496-509: 🚀 Performance & Scalability | 🔵 TrivialUnbounded accumulation of intermittent checkpoint files.
A full
_stage_model_epoch_{n}.ptcheckpoint is written everyrmse_log_interval(default 100) epochs with no retention/cleanup. Forepochs=10000(as in the MLP tutorial) that's ~100 checkpoint files persisted for the life of the run. Consider keeping only the last N or the best-by-val-RMSE checkpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/workflow_train_physicsnemo.py` around lines 496 - 509, Limit retention of intermittent checkpoints created in the training loop around torch.save and the _stage_model_epoch naming. Track saved checkpoint paths and remove older files once the configured retention count is exceeded, keeping only the most recent N (or an explicitly selected best-by-val-RMSE checkpoint); preserve the existing checkpoint logging and final model-saving behavior.tutorials/tutorial_09c_byod_train_physicsnemo_mgn.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_gating_stage_from_filenameand_write_subject_manifestare duplicated verbatim across the two tutorials. As per coding guidelines, "**/*_tools.py: Refer to*_tools.pyfiles for commonly used routines" — these are prime candidates to move intophysicsnemo_tools.py(already imported aspntby the workflows) and call from both tutorials, avoiding drift if one copy is fixed and the other isn't.
tutorials/tutorial_09c_byod_train_physicsnemo_mgn.py#L49-85: replace the local_gating_stage_from_filename/_write_subject_manifestwith imports from a shared helper.tutorials/tutorial_09d_byod_train_physicsnemo_mlp.py#L50-86: same — replace the local copies with the same shared helper.🤖 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_09c_byod_train_physicsnemo_mgn.py` at line 1, Move the shared _gating_stage_from_filename and _write_subject_manifest implementations into physicsnemo_tools.py, exposing them through the existing pnt import. Remove both local helper definitions from tutorials tutorial_09c_byod_train_physicsnemo_mgn.py and tutorial_09d_byod_train_physicsnemo_mlp.py, and update each workflow to call the shared pnt helpers while preserving current behavior.Source: Coding guidelines
🤖 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/physiotwin4d/physicsnemo_tools.py`:
- Around line 161-190: Update mesh_to_edge_index to triangulate the input
PolyData before reading poly.faces, ensuring extract_surface outputs with quads
or other polygons are converted to a true triangle mesh. Preserve the existing
edge construction and undirected conversion after triangulation.
In `@src/physiotwin4d/workflow_train_physicsnemo.py`:
- Around line 263-290: Update _load_subjects to detect duplicate
manifest.subject_id values before assigning to subjects, including duplicates
within a split and across train and val manifests. Raise a clear ValueError
identifying the duplicated subject and its conflicting split instead of silently
overwriting the existing entry.
- Around line 687-716: Update the MeshGraphNet construction in _build_model to
pass self.num_layers as num_layers_edge_encoder, ensuring the configured layer
count applies to the edge encoder as well as the other encoder, processor, and
decoder components.
- Line 1: Make surface-point ordering explicit across the training workflow and
reconstruct_reference_points(): do not rely on
extract_surface(algorithm="dataset_surface") preserving PCA topology. Use
vtkOriginalPointIds to remap extracted points into the PCA basis order, or
consistently replace both extraction paths with an ordering-preserving method,
ensuring the shared mean-shape and PCA coordinates remain aligned.
In `@tutorials/tutorial_09c_byod_train_physicsnemo_mgn.py`:
- Around line 31-36: Update the “Extra Install Required” note in the tutorial
module docstring to state that Python 3.11 or newer is required for the
physiotwin4d[physicsnemo] installation, matching the corresponding note in the
sibling MLP tutorial.
In `@tutorials/tutorial_09d_byod_train_physicsnemo_mlp.py`:
- Line 93: Update the OUTPUT_DIR assignment in the tutorial to use a
tutorial-specific directory name, matching the sibling MGN tutorial’s naming
convention (for example, an output_physicsnemo_mlp-style directory) instead of
the generic "output" path, while preserving the existing TUTORIALS_DIR base.
---
Nitpick comments:
In `@docs/tutorials.rst`:
- Around line 329-333: Reorder the Tutorial 3 documentation block in
docs/tutorials.rst to appear before Tutorial 4a, and move the
TestTutorial03VTKToUSD class in tests/test_tutorials.py before
TestTutorial04aCreateStatisticalModel, preserving each block’s contents
unchanged.
In `@src/physiotwin4d/cli/train_physicsnemo.py`:
- Around line 13-22: Update the _apply_common workflow parameter from object to
the shared WorkflowTrainPhysicsNeMo base type, importing that class from the
established module. Remove the # type: ignore[attr-defined] comments so mypy
checks the inherited setter calls directly.
In `@src/physiotwin4d/workflow_train_physicsnemo.py`:
- Around line 148-172: Introduce a shared _require_at_least(name, value,
minimum) validation helper and update set_epochs, set_batch_size,
set_cache_size, and the subclass setters for processor_size, hidden_dim,
num_layers, and layer_size to use it, preserving each setter’s existing
thresholds, error conditions, and assignments.
- Around line 50-53: Stop importing and exposing the private _Sample type from
workflow_train_physicsnemo.py; rename or otherwise expose the sample type
through physicsnemo_tools.py, then update its definition and all references,
including the list[_Sample] annotation, to use the public symbol consistently.
- Around line 496-509: Limit retention of intermittent checkpoints created in
the training loop around torch.save and the _stage_model_epoch naming. Track
saved checkpoint paths and remove older files once the configured retention
count is exceeded, keeping only the most recent N (or an explicitly selected
best-by-val-RMSE checkpoint); preserve the existing checkpoint logging and final
model-saving behavior.
In `@tests/test_tutorials.py`:
- Line 83: Update the _class_name in TestTutorial01aHeartGatedCTToUSD to match
the tutorial script’s TestTools identifier,
“tutorial_01a_heart_gated_ct_to_usd”, and rename or align the corresponding
baseline directory with this value.
In `@tutorials/tutorial_09c_byod_train_physicsnemo_mgn.py`:
- Line 1: Move the shared _gating_stage_from_filename and
_write_subject_manifest implementations into physicsnemo_tools.py, exposing them
through the existing pnt import. Remove both local helper definitions from
tutorials tutorial_09c_byod_train_physicsnemo_mgn.py and
tutorial_09d_byod_train_physicsnemo_mlp.py, and update each workflow to call the
shared pnt helpers while preserving current behavior.
In `@tutorials/tutorial_10c_byod_eval_physicsnemo_mgn.py`:
- Around line 54-85: Extract _gating_stage_from_filename and
_write_subject_manifest from tutorials/tutorial_10c_byod_eval_physicsnemo_mgn.py
lines 54-85 into the shared physicsnemo_tools.py module, preserving their
behavior and required dependencies; update the MGN tutorial to use the shared
helpers through its existing pnt import. In
tutorials/tutorial_10d_byod_eval_physicsnemo_mlp.py lines 54-85, remove the
duplicate helper definitions and import/use the same shared helpers instead.
🪄 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: d2fd81c3-9c0c-495d-b993-42e9ae8a3795
📒 Files selected for processing (29)
docs/architecture.rstdocs/index.rstdocs/quickstart.rstdocs/tutorials.rstpyproject.tomlsrc/physiotwin4d/__init__.pysrc/physiotwin4d/cli/infer_physicsnemo.pysrc/physiotwin4d/cli/train_physicsnemo.pysrc/physiotwin4d/physicsnemo_tools.pysrc/physiotwin4d/workflow_infer_physicsnemo.pysrc/physiotwin4d/workflow_train_physicsnemo.pytests/test_tutorials.pytutorials/README.mdtutorials/tutorial_01a_heart_gated_ct_to_usd.pytutorials/tutorial_01b_lung_gated_ct_to_usd.pytutorials/tutorial_02_ct_to_vtk.pytutorials/tutorial_03_vtk_to_usd.pytutorials/tutorial_04a_heart_create_statistical_model.pytutorials/tutorial_05a_heart_fit_statistical_model_to_patient.pytutorials/tutorial_06_reconstruct_highres_4d_ct.pytutorials/tutorial_08cd_byod_fit_model_to_patients.pytutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.pytutorials/tutorial_09b_cardiac_train_physicsnemo_mlp.pytutorials/tutorial_09c_byod_train_physicsnemo_mgn.pytutorials/tutorial_09d_byod_train_physicsnemo_mlp.pytutorials/tutorial_10a_cardiac_eval_physicsnemo_mgn.pytutorials/tutorial_10b_cardiac_eval_physicsnemo_mlp.pytutorials/tutorial_10c_byod_eval_physicsnemo_mgn.pytutorials/tutorial_10d_byod_eval_physicsnemo_mlp.py
💤 Files with no reviewable changes (4)
- tutorials/tutorial_09b_cardiac_train_physicsnemo_mlp.py
- tutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.py
- tutorials/tutorial_10b_cardiac_eval_physicsnemo_mlp.py
- tutorials/tutorial_10a_cardiac_eval_physicsnemo_mgn.py
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 `@src/physiotwin4d/segment_chest_total_segmentator.py`:
- Line 307: Update the docstring at the start of segmentation_method to
accurately state that the method runs the total and body tasks, with body
providing the skin outline rather than filling gaps, and additionally runs
heartchambers_highres and tissue_4_types when has_academic_license is present.
Keep the description factual and consistent with the method’s current behavior.
🪄 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: b4b7a16f-2981-4c2e-b0db-58be2d8be95c
📒 Files selected for processing (9)
src/physiotwin4d/physicsnemo_tools.pysrc/physiotwin4d/segment_chest_total_segmentator.pysrc/physiotwin4d/workflow_infer_physicsnemo.pysrc/physiotwin4d/workflow_train_physicsnemo.pytutorials/tutorial_05a_heart_fit_statistical_model_to_patient.pytutorials/tutorial_09c_byod_train_physicsnemo_mgn.pytutorials/tutorial_09d_byod_train_physicsnemo_mlp.pytutorials/tutorial_10c_byod_eval_physicsnemo_mgn.pytutorials/tutorial_10d_byod_eval_physicsnemo_mlp.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tutorials/tutorial_09c_byod_train_physicsnemo_mgn.py
- tutorials/tutorial_10d_byod_eval_physicsnemo_mlp.py
- tutorials/tutorial_10c_byod_eval_physicsnemo_mgn.py
- tutorials/tutorial_09d_byod_train_physicsnemo_mlp.py
- src/physiotwin4d/physicsnemo_tools.py
- tutorials/tutorial_05a_heart_fit_statistical_model_to_patient.py
- src/physiotwin4d/workflow_train_physicsnemo.py
- src/physiotwin4d/workflow_infer_physicsnemo.py
Summary by CodeRabbit