SolverMuJoCo: add custom attribute for jnt_actgravcomp#1174
Conversation
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
📝 WalkthroughWalkthroughAdds a new MuJoCo per-joint custom attribute Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1690-1762: Consider tilingjnt_actgravcompinexpand_model_fieldsfor multi‑world configs.
expand_model_fieldscurrently tiles severaljnt_*arrays (jnt_stiffness,jnt_range,jnt_margin, etc.) acrossnworld, but does not includejnt_actgravcomp. IfMjWarpModelexposesjnt_actgravcompwith a world dimension (similar to the other joint fields), you likely want it replicated as well so multi‑world simulations see consistent gravity‑comp flags.If that’s the case, adding
"jnt_actgravcomp"tomodel_fields_to_expandwould keep behavior aligned with the other joint properties:model_fields_to_expand = { # "qpos0", # "qpos_spring", @@ "jnt_stiffness", "jnt_range", + "jnt_actgravcomp",If
jnt_actgravcompis intentionally world‑invariant in mjwarp (no world dimension), then no change is needed.newton/tests/test_import_usd.py (2)
1651-1745: USDjnt_actgravcompparsing test is well‑targeted; optionally assert per‑joint mapping for extra robustness.The test cleanly exercises USD →
Model.mujoco.jnt_actgravcompwiring and matches the style of the existing MuJoCo custom‑attribute tests. If you want it to be slightly more robust, you could explicitly check which DOF corresponds toJoint1vsJoint2usingmodel.joint_keyandmodel.joint_qd_start, for example:joint_names = model.joint_key qd_start = model.joint_qd_start.numpy() jnt_actgravcomp = model.mujoco.jnt_actgravcomp.numpy() idx1 = joint_names.index("/Articulation/Joint1") idx2 = joint_names.index("/Articulation/Joint2") self.assertTrue(jnt_actgravcomp[qd_start[idx1]]) self.assertFalse(jnt_actgravcomp[qd_start[idx2]])Not required, but this would guard against any future changes in joint ordering or extra DOFs.
1624-1624: Remove redundant innerimport math # noqa: PLC0415to satisfy Ruff.Ruff reports an unused
noqadirective (PLC0415not enabled), andmathis already imported at the top of the file. You can drop this inner import (and the# noqa) and rely on the module‑level import:- import math # noqa: PLC0415 - - angular_gain_unit_scale = math.degrees(1.0) + angular_gain_unit_scale = math.degrees(1.0)This keeps the test behavior identical while resolving the RUF100 warning.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 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/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 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/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (3)
newton/tests/test_import_usd.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(151-238)
newton/tests/test_import_mjcf.py (2)
newton/tests/test_import_usd.py (1)
test_jnt_actgravcomp_parsing(1652-1744)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(77-2056)register_custom_attributes(151-238)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(623-668)ModelBuilder(70-5751)CustomAttribute(308-398)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-70)ModelAttributeAssignment(31-46)
🪛 Ruff (0.14.7)
newton/tests/test_import_usd.py
1654-1654: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ 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). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
227-238: End-to-endjnt_actgravcompwiring looks correct and consistent with existing MuJoCo attributes.The new custom attribute registration, retrieval, and per-DOF application into
joint_params["actgravcomp"]all line up with how other JOINT_DOF MuJoCo attributes (limit margins, stiffness, damping) are handled, and the per-DOF indexing matches the existing Newton‑DOF ↔ MuJoCo‑joint mapping design. I don't see functional or API issues here.Also applies to: 1014-1014, 1361-1362, 1436-1437
newton/tests/test_mujoco_solver.py (1)
1503-1542: LGTM! Test properly validates jnt_actgravcomp attribute propagation.The test correctly verifies that the custom attribute is:
- Properly stored in the Newton model with boolean values
- Correctly converted to MuJoCo's native format (1/0 integers)
The test scope aligns well with the PR objectives (no runtime updates or per-world variation).
newton/tests/test_import_mjcf.py (1)
1040-1074: LGTM! Test comprehensively validates MJCF actuatorgravcomp parsing.The test properly covers all three cases:
- Explicit
actuatorgravcomp="true"- Explicit
actuatorgravcomp="false"- Default behavior (no attribute specified)
The attribute mapping from MJCF's
actuatorgravcompto Newton'sjnt_actgravcompis correctly handled via the custom attribute registration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1020-1026: Attribute propagation into MuJoCo joints is aligned with existing DOF mappingReading
model.mujoco.jnt_actgravcompviaget_custom_attributeand indexing it byaiin the linear and angular DOF loops mirrors how other JOINT_DOF attributes (limit_margin, solimplimit, solimpfriction, passive stiffness/damping) are applied, and fits the per-DOF → per-MuJoCo-joint mapping used elsewhere in this solver (one MuJoCo joint per Newton DOF in these branches, per prior mapping learnings). Given the PR’s scope, omitting runtime updates and per-world variation is acceptable.If you later want per-world or runtime-adjustable actuator gravcomp, this would need extending through
expand_model_fieldsandupdate_joint_dof_properties_kernel; for now the implementation is coherent and minimal as described.Based on learnings, ...
Also applies to: 1373-1374, 1451-1452
newton/tests/test_import_usd.py (1)
1773-1773: Remove unnecessarynoqadirective.The
# noqa: PLC0415directive is unnecessary since the PLC0415 rule (import-not-at-top-of-module) is not enabled in your Ruff configuration.Apply this diff:
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_import_mjcf.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_mujoco_solver.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 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/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 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/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (2)
newton/tests/test_mujoco_solver.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(151-249)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(623-668)ModelBuilder(70-5751)CustomAttribute(308-398)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-70)ModelAttributeAssignment(31-46)
🪛 Ruff (0.14.7)
newton/tests/test_import_usd.py
1773-1773: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ 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). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
238-249: Registration ofjnt_actgravcompcustom attribute looks correctThe new
ModelBuilder.CustomAttributeforjnt_actgravcompis consistent with existing MuJoCo JOINT_DOF attributes (namespace, assignment, frequency, dtype, USD/MJCF names, and default). This should integrate cleanly into the existing custom-attribute machinery.newton/tests/test_mujoco_solver.py (1)
1504-1543:test_jnt_actgravcomp_conversionadequately validates end-to-end wiringThe test cleanly exercises the full path: registration → builder
custom_attributes→model.mujoco.jnt_actgravcomp→SolverMuJoCo→mj_model.jnt_actgravcomp. The True/False → 1/0 expectations match the underlying MuJoCo representation, and the setup mirrors other custom-attribute tests in this file.newton/tests/test_import_usd.py (1)
1770-1864: Note: AI summary inconsistency regarding duplicate tests.The AI-generated summary states that this file "Adds two duplicate test methods named test_jnt_actgravcomp_parsing," but only one test method exists in this file. The summary may be conflating tests across multiple files (test_import_usd.py, test_import_mjcf.py, and test_mujoco_solver.py).
0a22111
…s#1174) Signed-off-by: Alain Denzler <adenzler@nvidia.com>
…s#1174) Signed-off-by: Alain Denzler <adenzler@nvidia.com>
This one does not support varying values between worlds and runtime updates, so the changes are very minimal.
Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.