ENH: Update physicsnemo tutorials to use cardiac data.#70
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThis PR replaces the removed lung-lobe PCA and PhysicsNeMo tutorials with a cardiac tutorial pipeline covering SSM fitting, PhysicsNeMo training, and checkpoint-based evaluation. It also updates tutorial docs, tests, mypy coverage, pre-commit/mypy config, and one workflow API to accept optional ICON iteration settings. ChangesCardiac Tutorial Pipeline Replacement
Estimated code review effort: 4 (Complex) | ~75 minutes 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 |
There was a problem hiding this comment.
Pull request overview
This PR replaces the prior DirLab lung-lobe PCA + PhysicsNeMo stage-model tutorials with a new cardiac “mesh stage-prediction” pipeline (Tutorials 8–10) built around KCL cardiac PCA assets and a local bring-your-own D:/PhysioMotion4D/ dataset layout, and updates docs/tests/tooling accordingly.
Changes:
- Added new cardiac Tutorials 8–10: fit/propagate cardiac SSM meshes, train PhysicsNeMo models (MGN + MLP), and evaluate/predict per-stage surfaces with optional error stats.
- Removed the legacy DirLab tutorial chain (Tutorials 7–9) and updated tutorial indices/docs to reflect the new pipeline.
- Updated tutorial test suite to optionally run the new bring-your-own-data tutorials when local data and checkpoints are present; refreshed mypy tooling configuration.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tutorials/tutorial_10b_cardiac_eval_physicsnemo_mlp.py |
New MLP evaluation script for predicting cardiac stage meshes from a Tutorial 9b checkpoint. |
tutorials/tutorial_10a_cardiac_eval_physicsnemo_mgn.py |
New MeshGraphNet evaluation script for predicting/scoring cardiac stage meshes from a Tutorial 9a checkpoint. |
tutorials/tutorial_09b_cardiac_train_physicsnemo_mlp.py |
New MLP trainer across subjects using fitted cardiac SSM surfaces from Tutorial 8 (Option B displacement convention). |
tutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.py |
New MeshGraphNet trainer using shared mesh topology/edge features plus PCA coefficients and stage. |
tutorials/tutorial_08_cardiac_fit_model.py |
New bring-your-own-data tutorial to fit the KCL cardiac PCA model and propagate through gated phases. |
tutorials/tutorial_09_physicsnemo_mesh_stage_model.py |
Removed old DirLab stage-model tutorial. |
tutorials/tutorial_08_dirlab_pca_time_series.py |
Removed old DirLab propagation tutorial. |
tutorials/tutorial_07_dirlab_pca_model.py |
Removed old DirLab PCA model + fit tutorial. |
tutorials/tutorial_04_fit_statistical_model_to_patient.py |
Minor typing cleanup around extracting surfaces and passing pv.DataSet lists. |
tutorials/README.md |
Updated tutorial index/ordering and clarified that cardiac Tutorials 8–10 are bring-your-own-data. |
tests/test_tutorials.py |
Added optional/skip-gated tests for cardiac Tutorials 8–10 and helper to avoid argparse/pytest argv conflicts. |
src/physiomotion4d/workflow_reconstruct_highres_4d_ct.py |
Allows disabling ICON fine-tuning by accepting None iterations; updated docstring accordingly. |
README.md |
Updated top-level tutorial table to reflect the new cardiac pipeline and numbering. |
pyproject.toml |
Bumped mypy minimum version and updated mypy module targets to include new tutorials. |
experiments/LongitudinalRegistration/.gitignore |
Ignores *.txt artifacts in the experiment directory. |
docs/tutorials.rst |
Updated tutorial documentation to describe the new cardiac Tutorials 8–10 and remove old DirLab 7–9. |
.pre-commit-config.yaml |
Updated mypy pre-commit mirror revision. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 32.36% 32.38% +0.01%
==========================================
Files 53 53
Lines 7257 7257
==========================================
+ Hits 2349 2350 +1
+ Misses 4908 4907 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
tutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.py (1)
259-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
mean_surf.n_faceswithmean_surf.n_faces_strict. PyVista 0.47.0 deprecatesPolyData.n_faces, and the strict API avoids legacy cell-count semantics in this tutorial path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.py` around lines 259 - 272, The graph logging in the tutorial still uses the deprecated PolyData.n_faces property; update the mean-shape mesh handling in this block to use mean_surf.n_faces_strict instead. Keep the existing logging and edge-count logic in the shared mesh graph section, but replace every reference to mean_surf.n_faces so the tutorial path uses the strict cell-count API consistently.tutorials/tutorial_10a_cardiac_eval_physicsnemo_mgn.py (1)
168-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Optional[list[float]]instead oflist[float] | None.As per coding guidelines,
**/*.pyfiles should useOptional[X]instead ofX | None(ruffUP007suppressed).♻️ Proposed fix
+from typing import Any, Optional, cast -from typing import Any, cast ... def predict( subject: str, epoch: int, out_dir: Path, - stages: list[float] | None = None, + stages: Optional[list[float]] = None, ) -> dict[str, Any]:🤖 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_10a_cardiac_eval_physicsnemo_mgn.py` at line 168, The type annotation in the tutorial function signature uses the PEP 604 union form for an optional value; update the parameter on the relevant function (the one taking stages) to use Optional[list[float]] instead of list[float] | None, and ensure the file follows the project’s Python typing style consistently.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 `@docs/tutorials.rst`:
- Around line 218-227: Update the outdated navigation text in tutorials.rst so
it matches the renamed/removed tutorials: the Hero card-grid links and the
“Recommended Run Order” section still reference the deleted DirLab lung-lobe
Tutorial 7 and the old Tutorial 8/9 anchors. Replace those references with the
current cardiac pipeline numbering and headings for Tutorials 8-10b, and remove
any guidance that tells users to run Tutorial 7 or depend on it so the links and
ordering stay consistent with the new structure.
In `@tutorials/tutorial_08_cardiac_fit_model.py`:
- Around line 263-269: The warped labelmap write in the image-saving section is
missing compression, unlike the nearby reference image save. Update the
itk.imwrite call used for labelmap output in the patient export logic to pass
compression=True, keeping the existing labelmap path and data flow unchanged.
Use the nearby itk.imwrite call for the warped reference image as the pattern
and ensure the labelmap write follows the same compressed storage guideline.
- Around line 242-282: The SSM warping in the phase export path is using the
wrong transform direction: `TransformTools().transform_image` already applies
`inv_tfm` for the reference labelmap, but the subsequent `transform_pvcontour`
calls for `ssm_mesh_fitted` and `ssm_surface_fitted` still use `fwd_tfm`. Update
the phase-mapping logic in this block to use the inverse transform for the
reference-to-phase deformation, keeping the saved `forward_tfm`/`inverse_tfm`
files intact and only swapping the transform passed into the mesh and surface
warps.
- Around line 196-205: The gated-file collection in the time-series setup should
exclude non-phase files like the reference volume before parsing `_g`, since
`gated_file.name.split("_g")[1]` in the `time_series` loop can fail on files
without that token. Update the `patient_dir.glob("*.nii.gz")` filtering logic to
keep only gated phase inputs, and sort the resulting `gated_files`
deterministically before building `time_series` and `time_series_ids` so
`itk.imread` and time ID extraction in `tutorial_08_cardiac_fit_model.py` always
operate on valid phase files.
In `@tutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.py`:
- Around line 422-486: The validation tensor preparation in the training flow
can crash when `VAL_SUBJECTS` is empty because `val_samples` becomes empty
before `torch.stack` is called. Update the sample-building and GPU pre-stacking
logic around `val_samples`, `train_samples`, and the later validation RMSE setup
to either require a non-empty validation split up front or branch to skip all
validation tensor creation and evaluation when no validation subjects are
provided.
- Around line 356-359: The checkpoint loading in the resume path should use the
safer tensor-only deserialization mode. Update the `resume_ckpt` load inside the
`resume_from_weights` branch in `tutorial_09a_cardiac_train_physicsnemo_mgn.py`
so the `torch.load` call includes `weights_only=True` along with
`map_location="cpu"`, keeping the existing `resume_from_weights` behavior and
format unchanged.
- Around line 466-470: The checkpoint save path is using the compiled wrapper’s
state_dict, which adds _orig_mod. keys and breaks RESUME_FROM_WEIGHTS when
loading into an uncompiled model. In
tutorial_09a_cardiac_train_physicsnemo_mgn.py, update the save logic around the
torch.compile block and the checkpoint write path to use getattr(model,
"_orig_mod", model).state_dict() so both compiled and uncompiled models save the
base weights consistently.
In `@tutorials/tutorial_09b_cardiac_train_physicsnemo_mlp.py`:
- Around line 403-431: The validation split handling in the training data
assembly still assumes `val_sids` is non-empty, so `np.vstack(val_inputs)` and
`np.vstack(val_targets)` will fail when `VAL_SUBJECTS` is `None`. Update the
logic around the validation build in the training routine (the block that fills
`val_inputs`/`val_targets` and computes `val_inputs_array`/`val_targets_array`)
to either reject an empty validation split up front with a clear error or skip
validation dataset/RMSE creation entirely when `val_sids` is empty.
- Around line 328-331: The resume checkpoint loading in the training script is
using the default pickle-enabled torch.load behavior, which should be tightened
for this plain dict checkpoint. Update the resume path in the block that sets
resume_ckpt and calls torch.load so it passes weights_only=True when loading
RESUME_FROM_WEIGHTS. Keep the existing map_location="cpu" behavior and preserve
the surrounding resume_from_weights logic.
- Around line 457-460: The checkpoint save logic in the training flow should
persist the underlying base model parameters, not the compiled wrapper state.
Update the epoch and final checkpoint writes in the same area where
torch.compile is applied so they use model._orig_mod.state_dict() when the
compiled module exposes _orig_mod, and fall back to model.state_dict()
otherwise. Keep the change localized around the model checkpointing path used
after torch.compile.
In `@tutorials/tutorial_10a_cardiac_eval_physicsnemo_mgn.py`:
- Line 196: The metadata checkpoint load in the tutorial should use the safe
weights-only path instead of full pickle deserialization. Update the torch.load
call for meta_ckpt in the metadata loading section to pass weights_only=True,
matching the other checkpoint reads and keeping the load limited to tensors and
primitive values.
- Around line 377-403: The ALL-row aggregation in
tutorial_10a_cardiac_eval_physicsnemo_mgn.py mislabels a mean of per-phase
medians as "median_error_mm". Update the stats_rows.append block that builds the
ALL entry so "median_error_mm" is computed from the pooled error values across
all results (consistent with how rms_error_mm and max_error_mm use pooled data),
rather than averaging existing "median_error_mm" values from stats_rows. Use the
ALL-row construction in the stats aggregation section to locate and correct this
calculation.
In `@tutorials/tutorial_10b_cardiac_eval_physicsnemo_mlp.py`:
- Around line 82-84: The default checkpoint selection in
run_tutorial()/DEFAULT_EPOCH is inconsistent with the test-mode training limit,
so the tutorial points to a nonexistent checkpoint. Update the
tutorial_10b_cardiac_eval_physicsnemo_mlp.py logic around DEFAULT_EPOCH,
DEFAULT_OUT_DIR, and run_tutorial() to use epoch 2 in test mode or dynamically
pick the latest available checkpoint before evaluation.
- Around line 139-145: The checkpoint loading in
tutorial_10b_cardiac_eval_physicsnemo_mlp.py uses the unsafe default
deserialization path even though physicsnemo_stage_model.pt only contains a
state_dict and simple metadata. Update the torch.load call in the main
evaluation flow to use the safe loader by enabling weights_only=True, keeping
the existing existence check and metadata handling intact.
---
Nitpick comments:
In `@tutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.py`:
- Around line 259-272: The graph logging in the tutorial still uses the
deprecated PolyData.n_faces property; update the mean-shape mesh handling in
this block to use mean_surf.n_faces_strict instead. Keep the existing logging
and edge-count logic in the shared mesh graph section, but replace every
reference to mean_surf.n_faces so the tutorial path uses the strict cell-count
API consistently.
In `@tutorials/tutorial_10a_cardiac_eval_physicsnemo_mgn.py`:
- Line 168: The type annotation in the tutorial function signature uses the PEP
604 union form for an optional value; update the parameter on the relevant
function (the one taking stages) to use Optional[list[float]] instead of
list[float] | None, and ensure the file follows the project’s Python typing
style consistently.
🪄 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: 705d8523-1a6f-4cca-b742-a3da162ba33f
📒 Files selected for processing (17)
.pre-commit-config.yamlREADME.mddocs/tutorials.rstexperiments/LongitudinalRegistration/.gitignorepyproject.tomlsrc/physiomotion4d/workflow_reconstruct_highres_4d_ct.pytests/test_tutorials.pytutorials/README.mdtutorials/tutorial_04_fit_statistical_model_to_patient.pytutorials/tutorial_07_dirlab_pca_model.pytutorials/tutorial_08_cardiac_fit_model.pytutorials/tutorial_08_dirlab_pca_time_series.pytutorials/tutorial_09_physicsnemo_mesh_stage_model.pytutorials/tutorial_09a_cardiac_train_physicsnemo_mgn.pytutorials/tutorial_09b_cardiac_train_physicsnemo_mlp.pytutorials/tutorial_10a_cardiac_eval_physicsnemo_mgn.pytutorials/tutorial_10b_cardiac_eval_physicsnemo_mlp.py
💤 Files with no reviewable changes (3)
- tutorials/tutorial_08_dirlab_pca_time_series.py
- tutorials/tutorial_09_physicsnemo_mesh_stage_model.py
- tutorials/tutorial_07_dirlab_pca_model.py
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Summary by CodeRabbit