ENH: Upaded AI agents for consistency and make physicsnemo optional#60
Merged
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds Greedy and combined greedy_icon registration paths with iteration configuration and tests; updates many docs, agent skills, CI workflows, pytest run-buckets, convention checks, and tutorial PhysicsNeMo install/guarding. ChangesRegistration Greedy support
Agent, CI, docs, and skills updates
Sequence Diagram(s) sequenceDiagram
participant User
participant RegisterTimeSeriesImages
participant GreedyRegistrar
participant ANTsRegistrar
participant ICONRegistrar
participant TransformStore
User->>RegisterTimeSeriesImages: call register_time_series(registration_method="greedy"|"ants_icon"|"greedy_icon")
RegisterTimeSeriesImages->>GreedyRegistrar: configure & run initial stage (if greedy or greedy_icon)
RegisterTimeSeriesImages->>ANTsRegistrar: configure & run initial stage (if ants or ants_icon)
GreedyRegistrar->>TransformStore: produce initial_forward_transform
ANTsRegistrar->>TransformStore: produce initial_forward_transform
RegisterTimeSeriesImages->>ICONRegistrar: call ICON refinement with initial_forward_transform
ICONRegistrar->>TransformStore: produce refined forward/inverse transforms
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 updates repository automation/docs to make PhysicsNeMo an optional dependency (used only for Tutorial 9), and extends time-series registration to support Greedy and Greedy→ICON refinement while standardizing test-bucket gating via a --run-all convenience flag.
Changes:
- Make PhysicsNeMo opt-in via a new
[physicsnemo]extra and add runtime import guards + documentation updates for Tutorial 9. - Add Greedy (and
greedy_icon) support toRegisterTimeSeriesImages, plus unit-test coverage for Greedy initialization/iteration configuration. - Add
--run-physicsnemoand--run-allpytest flags and update CI/workflows/docs to use the new buckets.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/ai_agent_github_reviews.py | Tightens AI review “hard rules” (coords/LPS vs RAS, emoji ban, Windows mp guard reminder). |
| tutorials/tutorial_09_physicsnemo_mesh_stage_model.py | Adds PhysicsNeMo optional-import guard and documents extra install requirement. |
| tutorials/README.md | Notes Tutorial 9 requires the [physicsnemo] extra. |
| tests/test_register_time_series_images.py | Adds Greedy init test + Greedy iteration setter coverage. |
| tests/README.md | Documents --run-physicsnemo and --run-all buckets. |
| tests/conftest.py | Implements --run-physicsnemo and --run-all bucket enablement + marker registration. |
| src/physiomotion4d/register_time_series_images.py | Adds Greedy and greedy_icon support + Greedy iteration configuration API. |
| README.md | Documents PhysicsNeMo as an optional extra and clarifies Python version constraints. |
| pyproject.toml | Drops base Python requirement to 3.10; moves PhysicsNeMo to optional deps. |
| docs/tutorials.rst | Adds “extra install” note for Tutorial 9 PhysicsNeMo requirement. |
| docs/testing.rst | Documents --run-physicsnemo and --run-all; updates CI runner guidance. |
| docs/quickstart.rst | Clarifies Tutorial 9 needs optional physicsnemo extra (Python >= 3.11). |
| docs/developer/core.rst | Updates contributor test guidance to include --run-physicsnemo / --run-all. |
| docs/contributing.rst | Updates testing section with new buckets / --run-all. |
| docs/api/registration/time_series.rst | Updates time-series registration docs to include Greedy and greedy_icon. |
| docs/API_MAP.md | Regenerated API map reflecting public API/doc shifts. |
| CLAUDE.md | Updates agent commands/docs (adds --run-all, regen API map guidance, convention checks). |
| AGENTS.md | Updates contributor/agent rules and adds environment notes. |
| .gitignore | Ignores .claude AI tool state directory. |
| .github/workflows/test-slow.yml | Switches GPU slow workflow to install physicsnemo extra and use --run-all. |
| .github/workflows/README.md | Updates workflow docs/commands to match new buckets and extras. |
| .github/workflows/nightly-health.yml | Switches nightly GPU health run to install physicsnemo extra and use --run-all. |
| .github/workflows/ci.yml | Switches GPU workflow to install physicsnemo extra and use --run-all. |
| .agents/skills/review-pr/SKILL.md | Adds a “review-pr” skill doc for invoking the repo PR-review driver. |
| .agents/skills/regen-api-map/SKILL.md | Adds a “regen-api-map” skill doc for regenerating docs/API_MAP.md. |
| .agents/skills/check-conventions/SKILL.md | Adds a “check-conventions” skill doc encoding hard project rules. |
| .agents/agents/testing.md | Updates testing agent guidance (prefers real downloaded fixtures; clarifies commands). |
| .agents/agents/implementation.md | Updates implementation agent rules (LPS convention, no-emoji, ConvertVTKToUSD, etc.). |
| .agents/agents/architecture.md | Updates architecture invariants (LPS internal frame; USD export conversion). |
💡 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 #60 +/- ##
==========================================
- Coverage 31.22% 31.13% -0.10%
==========================================
Files 50 50
Lines 6828 6861 +33
==========================================
+ Hits 2132 2136 +4
- Misses 4696 4725 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (3)
tests/test_register_time_series_images.py (1)
57-72: ⚡ Quick winAdd one behavior test for combined-mode initial-transform propagation
The new tests validate construction and iteration state, but they don’t cover the new combined execution path. Add a focused test for
greedy_icon/ants_iconasserting that aninitial_forward_transformis propagated through the initial stage.Also applies to: 109-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_register_time_series_images.py` around lines 57 - 72, Add a new behavior test that constructs RegisterTimeSeriesImages with the combined modes "greedy_icon" and "ants_icon", set a sentinel value on the initial-stage registrar's initial_forward_transform (e.g., registrar.registrar_greedy.initial_forward_transform or registrar.registrar_ants.initial_forward_transform), and assert that after initialization the downstream ICON registrar (registrar.registrar_icon.initial_forward_transform) equals that sentinel; this verifies the initial_forward_transform is propagated from the initial registrar to the ICON stage in combined-mode..agents/skills/check-conventions/SKILL.md (1)
67-69: ⚡ Quick winAdd language specifier to fenced code block.
The example output format block should specify a language for proper rendering. Since this is showing example terminal output, use
textorconsole.📝 Proposed fix
-``` +```text <path>:<line> <rule short name> <one-line excerpt></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In @.agents/skills/check-conventions/SKILL.md around lines 67 - 69, The fenced
code block example in .agents/skills/check-conventions/SKILL.md that shows
": " is missing a language
specifier; update the triple-backtick block to include a language like text
(e.g., changetotext) so the example terminal output renders correctly.</details> </blockquote></details> <details> <summary>AGENTS.md (1)</summary><blockquote> `98-108`: _⚡ Quick win_ **Consider whether this Codex-specific troubleshooting belongs in shared contributor guidance.** The "Codex Sandbox" section documents a workaround for a single user's environment (`C:\Users\saylward\...`). This is very specific and may not apply to other contributors. Consider either: - Generalizing this section to cover common sandbox path issues without hardcoded user paths, or - Moving this guidance to a Codex-specific troubleshooting doc, or - Removing it if it's a temporary workaround The current wording makes `AGENTS.md` read like a personal troubleshooting log rather than universal contributor guidance. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 98 - 108, The "Codex Sandbox" section in AGENTS.md contains a user-specific workaround (hardcoded paths like the temporary venv path `C:\Users\saylward\AppData\Local\Temp\pm4d-uv-env\Scripts\python.exe`) that doesn't belong in shared contributor guidance; either generalize the guidance to describe symptoms and a generic workaround for broken launcher/venv paths without personal paths, or move this content into a Codex-specific troubleshooting doc (or remove it if temporary), and update the section title "Codex Sandbox" and its bullet points to reflect the chosen option. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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 @.github/workflows/ci.yml:
- Around line 305-311: The GPU job is creating a venv with Python 3.10 but then
tries to install the physicsnemo extra which requires Python >=3.11; update the
workflow so the interpreter matches the package requirement by changing the
runner/venv Python version to 3.11 (or, alternatively, remove the physicsnemo
extra from the pip install command) — specifically update the job setup that
creates the venv and the pip invocation that runs python -m uv pip install -e
".[test,cuda13,physicsnemo]" so the Python used for python -m uv is 3.11 (or
drop ",physicsnemo" from the extras if you prefer not to require 3.11).In @.github/workflows/README.md:
- Around line 147-153: The README currently shows the install command uv pip
install -e ".[test,cuda13,physicsnemo]" but omits that the physicsnemo extra
pulls in nvidia-physicsnemo which requires Python >= 3.11; update both
occurrences of uv pip install -e ".[test,cuda13,physicsnemo]" to include a short
note like “(requires Python >= 3.11)” immediately adjacent to the command so
users on Python 3.10 see the prerequisite before attempting installation.In
@docs/contributing.rst:
- Around line 270-273: Update the comment next to the pytest command that
mentions the physicsnemo extra to specify the Python minimum, i.e., change the
comment on the line containing "pytest tests/ --run-physicsnemo" (the pytest
command shown in the diff) to read that it needs the [physicsnemo] extra and
Python >= 3.11 (for example: "# needs [physicsnemo] extra installed; requires
Python >= 3.11").In
@src/physiomotion4d/register_time_series_images.py:
- Around line 798-809: The forwarded initial_forward_transform argument is being
dropped when delegating to the internal registrars: when
self.registration_method_name is "ants_icon" or "greedy_icon" you call
registrar_initial.registration_method(...) but do not pass through the
initial_forward_transform; likewise pass it through in the later call that
invokes self.registrar_ants/registrar_greedy (the other block around lines
816-817). Fix by adding the initial_forward_transform=initial_forward_transform
(or the public parameter name) to the keyword args when calling
registrar_initial.registration_method and to the subsequent
registrar_*.registration_method calls so the initial transform is preserved
(references: self.registration_method_name, registrar_initial,
self.registrar_ants.registration_method,
self.registrar_greedy.registration_method, initial_res).- Line 22: The module-level constant REGISTRATION_METHODS lacks an explicit type
annotation; add a precise type (e.g., List[str] or Sequence[str]) to its
declaration and import the corresponding typing symbol (from typing import List
or Sequence) at the top of the module so the constant is declared with an
explicit type like REGISTRATION_METHODS: List[str] = [...] to satisfy strict
typing.In
@tests/test_register_time_series_images.py:
- Line 71: Remove the noisy print statement print("\nTime series registrar
initialized with Greedy") from tests/test_register_time_series_images.py; if you
need to record that information for debugging replace it with the logging module
(e.g., logger.debug/ info) or rely on pytest's caplog fixture, ensuring no
direct print() calls remain in the test.
Nitpick comments:
In @.agents/skills/check-conventions/SKILL.md:
- Around line 67-69: The fenced code block example in
.agents/skills/check-conventions/SKILL.md that shows ": " is missing a language specifier; update the
triple-backtick block to include a language like text (e.g., change ``` toIn `@AGENTS.md`: - Around line 98-108: The "Codex Sandbox" section in AGENTS.md contains a user-specific workaround (hardcoded paths like the temporary venv path `C:\Users\saylward\AppData\Local\Temp\pm4d-uv-env\Scripts\python.exe`) that doesn't belong in shared contributor guidance; either generalize the guidance to describe symptoms and a generic workaround for broken launcher/venv paths without personal paths, or move this content into a Codex-specific troubleshooting doc (or remove it if temporary), and update the section title "Codex Sandbox" and its bullet points to reflect the chosen option. In `@tests/test_register_time_series_images.py`: - Around line 57-72: Add a new behavior test that constructs RegisterTimeSeriesImages with the combined modes "greedy_icon" and "ants_icon", set a sentinel value on the initial-stage registrar's initial_forward_transform (e.g., registrar.registrar_greedy.initial_forward_transform or registrar.registrar_ants.initial_forward_transform), and assert that after initialization the downstream ICON registrar (registrar.registrar_icon.initial_forward_transform) equals that sentinel; this verifies the initial_forward_transform is propagated from the initial registrar to the ICON stage in combined-mode.🪄 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:
84682737-b7ae-40bf-b169-d209baa88ce2📒 Files selected for processing (29)
.agents/agents/architecture.md.agents/agents/implementation.md.agents/agents/testing.md.agents/skills/check-conventions/SKILL.md.agents/skills/regen-api-map/SKILL.md.agents/skills/review-pr/SKILL.md.github/workflows/README.md.github/workflows/ci.yml.github/workflows/nightly-health.yml.github/workflows/test-slow.yml.gitignoreAGENTS.mdCLAUDE.mdREADME.mddocs/API_MAP.mddocs/api/registration/time_series.rstdocs/contributing.rstdocs/developer/core.rstdocs/quickstart.rstdocs/testing.rstdocs/tutorials.rstpyproject.tomlsrc/physiomotion4d/register_time_series_images.pytests/README.mdtests/conftest.pytests/test_register_time_series_images.pytutorials/README.mdtutorials/tutorial_09_physicsnemo_mesh_stage_model.pyutils/ai_agent_github_reviews.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 @.gitignore:
- Around line 20-22: The .gitignore contains a misspelled pattern
".github_reveiw_prompt.txt" which doesn't match the actual generated file;
update the .gitignore to replace the incorrect entry with the correct
".github_review_prompt.txt" (remove or fix the misspelled
".github_reveiw_prompt.txt") so the intermediate prompt produced by the script
is properly ignored.
🪄 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: 379612e1-22b2-4ca0-8c8d-5f7aba80e1f0
📒 Files selected for processing (11)
.agents/skills/check-conventions/SKILL.md.agents/skills/review-pr/SKILL.md.github/workflows/README.md.github/workflows/ci.yml.github/workflows/nightly-health.yml.github/workflows/test-slow.yml.gitignoredocs/API_MAP.mddocs/contributing.rstsrc/physiomotion4d/register_time_series_images.pyutils/ai_agent_github_reviews.py
✅ Files skipped from review due to trivial changes (2)
- docs/contributing.rst
- docs/API_MAP.md
| "--run-all", | ||
| action="store_true", | ||
| default=False, | ||
| help=("Enable every --run-* bucket: " + ", ".join(_RUN_BUCKET_FLAGS) + "."), |
| ) | ||
|
|
||
| print("\nTime series registrar initialized with Greedy") | ||
|
|
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
Documentation
Testing
Chores