docs: clarify solver integration docs - #3016
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDocumentation tables describing solver integration method support are corrected in both ChangesSolver Integration Classification Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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.
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 `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Line 3135: Add a CHANGELOG entry under the [Unreleased] section (preferably in
the "Fixed" subsection) noting the user-facing documentation correction to
supported integrator values for solver_mujoco.py; reference the integrator
parameter documentation change (integrator: Integrator type... Can be "euler",
"rk4", "implicit", or "implicitfast", or their corresponding MuJoCo integer
constants) and include a short line like "Fix: document supported integrator
values for solver_mujoco.integrator" so the docs change is tracked.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f6e621dc-396c-4613-bda9-8b5143fe3783
📒 Files selected for processing (2)
docs/api/newton_solvers.rstnewton/_src/solvers/mujoco/solver_mujoco.py
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Thanks for the contribution — these solver docs have been inconsistent for a while and it's great to see them cleaned up.
Two of the three changes are spot on:
SolverFeatherstone→ "Semi-implicit" matches the class docstring ("a semi-implicit integrator using symplectic Euler"). ✅SolverMuJoCo→ "Explicit, Semi-implicit, Implicit-in-velocity" is more accurate than the old "Implicit" — the defaultmujoco_warpbackend'simplicitfastis exactly "implicit in velocity." ✅
One request before merge (see inline comment): the constructor-docstring edit adds "implicit" as an integrator option, which pulls in the opposite direction from the table change. Could we revert that one line? With that, this is good to go.
Co-authored-by: Alain Denzler <adenzler@nvidia.com> Signed-off-by: Yahao Fan <108322378+fanyahao1@users.noreply.github.com>
You're right — that constructor docstring line should stay aligned with the table and the default mujoco_warp backend. I've reverted it. Thank you! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Head branch was pushed to by a user without write access
da7fae9
Description
Close #3015 without changing runtime behavior.
Fix two solver integration documentation inconsistencies.
SolverFeatherstoneis documented as semi-implicit, matching its class docstring and symplectic-Euler integration kernel.SolverMuJoCooverview entry asExplicit, Semi-implicit, Implicit-in-velocity, matching the MuJoCo integrator categories exposed by Newton.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
newton/_src/solvers/featherstone/solver_featherstone.pynewton/_src/solvers/featherstone/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pyBug fix
Steps to reproduce:
docs/api/newton_solvers.rst.SolverFeatherstoneintegration label against theSolverFeatherstoneclass docstring andintegrate_generalized_jointsimplementation.Minimal reproduction:
# Documentation-only issue; no runtime reproduction.Summary by CodeRabbit
SolverFeatherstoneandSolverMuJoCoin the solver documentation to accurately reflect their capabilities.