solverMuJoCo: change nefc_per_env to njmax#663
Conversation
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
📝 WalkthroughWalkthroughRenamed the SolverMuJoCo parameter Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Solver as SolverMuJoCo
participant Converter as convert_to_mjc
participant MJW as mujoco_warp.put_data
Note over User,Solver: Instantiate solver with njmax
User->>Solver: __init__(..., njmax)
Solver->>Converter: convert_to_mjc(..., njmax)
Note over Converter: final_njmax = max(njmax, mj_data.nefc)
Converter->>MJW: put_data(..., njmax=final_njmax)
MJW-->>Converter: ack
Converter-->>Solver: (MjWarpModel, MjWarpData, MjModel, MjData)
Solver-->>User: solver instance ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
1162-1162: Consider keeping a backwards-compatible alias for nefc_per_env.To avoid breaking downstream users immediately, accept nefc_per_env as a deprecated alias and map it to njmax, emitting a DeprecationWarning.
Apply within the signature:
- njmax: int = 100, + njmax: int = 100, + nefc_per_env: int | None = None,Then just after argument parsing:
# Back-compat shim for deprecated argument if nefc_per_env is not None: import warnings warnings.warn("`nefc_per_env` is deprecated; use `njmax`.", DeprecationWarning, stacklevel=2) njmax = nefc_per_env if njmax == 100 else njmax # preserve explicit njmax if set
1188-1189: Doc clarity: “per environment (world)” can be ambiguous when separate_envs_to_worlds=False.Suggest: “Maximum number of constraints per world (when environments are not separated, world == all envs).”
2356-2359: Heuristic njmax = max(njmax, mj_data.nefc) is okay, but won’t catch later spikes.If constraints can exceed the initial budget at runtime (e.g., more contacts later), consider detecting allocation overflow and reinitializing mjw_data with a larger njmax, or expose guidance in docs to set njmax conservatively.
Would you like a follow-up patch that grows mjw_data on demand when MuJoCo reports allocation errors?
newton/tests/test_anymal_reset.py (1)
100-104: Avoid magic number for njmax (optional).Define a constant to improve readability and reuse across tests.
class TestAnymalReset(unittest.TestCase): + NJMAX = 200 @@ - self.solver = newton.solvers.SolverMuJoCo( - self.model, solver=2, cone=cone_type, impratio=impratio, iterations=100, ls_iterations=50, njmax=200 - ) + self.solver = newton.solvers.SolverMuJoCo( + self.model, + solver=2, + cone=cone_type, + impratio=impratio, + iterations=100, + ls_iterations=50, + njmax=self.NJMAX, + )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
newton/_src/solvers/mujoco/solver_mujoco.py(5 hunks)newton/examples/example_mujoco.py(1 hunks)newton/examples/robot/example_robot_g1.py(1 hunks)newton/tests/test_anymal_reset.py(1 hunks)newton/tests/test_equality_constraints.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: preist-nvidia
PR: newton-physics/newton#579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
PR: newton-physics/newton#579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/tests/test_mujoco_solver.pynewton/examples/robot/example_robot_g1.pynewton/examples/example_mujoco.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_equality_constraints.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
PR: newton-physics/newton#579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/tests/test_mujoco_solver.pynewton/examples/robot/example_robot_g1.pynewton/examples/example_mujoco.pynewton/_src/solvers/mujoco/solver_mujoco.py
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (8)
newton/examples/example_mujoco.py (1)
246-246: Rename applied correctly (njmax passed into SolverMuJoCo).Constructor call now matches the updated API.
newton/examples/robot/example_robot_g1.py (1)
80-80: Rename applied correctly (njmax=300).Call site updated and consistent with solver API.
newton/tests/test_equality_constraints.py (1)
48-48: Test updated to new API (njmax).Looks consistent with the rename; no logic changes.
newton/tests/test_mujoco_solver.py (1)
318-318: Rename applied correctly (njmax=1).Note: runtime will upsize to max(njmax, nefc) at convert time, so this won’t under-allocate.
newton/_src/solvers/mujoco/solver_mujoco.py (3)
1253-1265: Propagation of njmax into convert_to_mjc is correct.No issues spotted.
1647-1648: Signature rename to njmax is consistent.Doc comment “number of constraints per world” matches MuJoCo terminology.
1155-1181: Allnefc_per_envreferences have been removed. No further updates required.newton/tests/test_anymal_reset.py (1)
102-104: Rename usage to njmax looks correct.Constructor now passes njmax explicitly; aligns with the SolverMuJoCo API rename.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Alain Denzler <adenzler@nvidia.com> Co-authored-by: Eric Heiden <eheiden@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com> Co-authored-by: Eric Heiden <eheiden@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com> Co-authored-by: Eric Heiden <eheiden@nvidia.com>
This makes it more aligned with the MuJoCo language and makes the error messages useful
Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Refactor
Bug Fixes
Documentation
Tests