Use default_body_armature for nonstatic link inertia in urdf parser#870
Conversation
|
|
📝 WalkthroughWalkthroughChanged inertia initialization for forced non-static URDF links to use Changes
Sequence Diagram(s)sequenceDiagram
participant Sim as Simulation Loop
participant Check as Example Validations
Note over Sim,Check #DDEBF7: Per-step validation (examples)
Sim->>Check: step(sim_time, q, qd, ...)
alt sim_time <= 1.0
Check->>Check: verify initial q[2] > 0.1
Check->>Check: verify directional check q[1] > 0.9
else sim_time > 1.0
Check->>Check: build forward_vel_min / forward_vel_max
Check->>Check: vec_inside_limits(qd, forward_vel_min, forward_vel_max)
end
Note over Sim,Check #F7F0F0: Pass/fail reported
sequenceDiagram
participant Import as URDF Import
participant Builder as ModelBuilder
Note over Import,Builder #EFF8F0: Inertia initialization when forcing mass
Import->>Builder: request default inertia scale
Builder-->>Import: builder.default_body_armature
Import->>Import: I_m += builder.default_body_armature * Identity
Note right of Import #F7F7F7: replaces prior scalar `default_shape_density` usage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Hi @lenroe-nv, could you follow these instructions to make your newton-physics membership public? That way we won't have to manually approve your workflows to run in the future:
|
eric-heiden
left a comment
There was a problem hiding this comment.
Thanks @lenroe-nv, this looks good! Can you fix the example test for robot_anymal_c_walk? Make sure to merge main first to get the latest example updates from #836.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_anymal_walk.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/tests/test_anymal_walk.py
[error] 52-52: Forward velocity too low: Step 700: Forward velocity 0.33740318 m/s (expected > 0.4) in test_anymal_walk_policy.
⏰ 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). (1)
- GitHub Check: Run GPU Benchmarks (Pull Request)
|
Had to adjust the example tests to account for the fact that angular velocity of the base is higher now (which is expected due to much smaller (i.e. more realistic) inertia). |
|
I don't understand why |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/examples/mpm/example_mpm_anymal.py(1 hunks)newton/examples/robot/example_robot_anymal_c_walk.py(1 hunks)newton/tests/test_anymal_walk.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
newton/examples/robot/example_robot_anymal_c_walk.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
newton/examples/mpm/example_mpm_anymal.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/tests/test_anymal_walk.py
[error] 52-52: AssertionError: Forward velocity too low: Step 900: Forward velocity -0.0435 m/s (expected > 0.3).
🪛 Ruff (0.13.2)
newton/examples/robot/example_robot_anymal_c_walk.py
245-245: Unused lambda argument: q
(ARG005)
newton/examples/mpm/example_mpm_anymal.py
297-297: Unused lambda argument: qd
(ARG005)
306-306: Unused lambda argument: q
(ARG005)
🔇 Additional comments (3)
newton/examples/robot/example_robot_anymal_c_walk.py (1)
238-247: LGTM: Time-gated range check improves test robustness.The shift from exact velocity assertions to a time-gated, range-based validation is a sound improvement. The
sim_time > 1.0guard allows the robot to stabilize before checking velocity ranges, and the per-axis limits (forward_vel_min,forward_vel_max) are more appropriate for validating physics simulation than exact equality checks.The static analysis warning about the unused
qparameter in the lambda (line 245) is a false positive—the(q, qd)signature is required by thetest_body_stateAPI, even if onlyqdis used in this particular validation.newton/examples/mpm/example_mpm_anymal.py (2)
297-297: LGTM: Relaxed position threshold aligns with inertia changes.Lowering the height threshold from 0.2 to 0.1 meters is appropriate given that the corrected inertia values (using
default_body_armatureinstead ofdefault_shape_density) may result in a slightly lower resting position for the robot. This adjustment ensures the test remains valid without being overly restrictive.The static analysis warning about the unused
qdparameter is a false positive—the(q, qd)lambda signature is required by thetest_body_stateAPI contract.
299-308: LGTM: Time-gated velocity validation is consistent and robust.The addition of time-gated velocity validation (checking only after
sim_time > 1.0) aligns with the pattern established inexample_robot_anymal_c_walk.py. This approach allows the robot to stabilize before asserting velocity ranges, making the test more resilient to transient startup dynamics. The range-based validation usingvec_inside_limitsis more appropriate for physics simulation than exact equality checks.The static analysis warning about the unused
qparameter (line 306) is a false positive—the(q, qd)lambda signature is required by thetest_body_stateAPI.
Can you try maybe to rerun the test job on the CI to see if the test is just flaky? |
|
Yes, seems flaky indeed. It passed at retry. I am going to remove this test then. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@lenroe-nv The |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/examples/mpm/example_mpm_anymal.py (1)
297-306: LGTM! Test bounds adjusted per feedback.The changes directly address the failing test by:
- Lowering the ground clearance threshold (0.2 → 0.1m) to accommodate the robot's behavior with corrected inertias
- Replacing exact velocity matching with range-based validation using per-axis bounds
These adjustments align with the physics corrections from the URDF parser fix and the maintainer's suggestion to increase bounds.
Optional: Consider refactoring duplicated test logic.
The velocity bounds and test structure in lines 299-306 are nearly identical to
newton/examples/robot/example_robot_anymal_c_walk.pylines 238-246. Consider extracting the shared validation logic into a helper function or test fixture innewton/examples/__init__.pyto improve maintainability.Note: The static analysis warnings about unused lambda arguments are false positives—the lambda signatures must match the expected
(q, qd)interface.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/examples/mpm/example_mpm_anymal.py(1 hunks)newton/examples/robot/example_robot_anymal_c_walk.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
newton/examples/robot/example_robot_anymal_c_walk.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
newton/examples/mpm/example_mpm_anymal.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
🪛 Ruff (0.13.3)
newton/examples/robot/example_robot_anymal_c_walk.py
244-244: Unused lambda argument: q
(ARG005)
newton/examples/mpm/example_mpm_anymal.py
297-297: Unused lambda argument: qd
(ARG005)
305-305: Unused lambda argument: q
(ARG005)
⏰ 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 (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (1)
newton/examples/robot/example_robot_anymal_c_walk.py (1)
238-246: LGTM! Range-based validation aligns with physics corrections.The change from exact velocity matching to per-axis bounds validation is appropriate given the URDF parser fix that produces smaller, more realistic inertias. The bounds accommodate the higher base angular velocity that results from the corrected physics.
Note: The static analysis warning about unused lambda argument
qis a false positive—the lambda signature must match the expected(q, qd)interface even though onlyqdis used for velocity validation.
|
Yes, there is a diff between my local tests passing and the AWS job failing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/examples/robot/example_robot_anymal_c_walk.py (1)
246-254: Consider tightening angular velocity bounds if feasible.The angular velocity tolerances (±0.5 to ±0.8 rad/s) are relatively wide. While the PR discussion acknowledges this is necessary after the inertia fix, consider whether these bounds can be tightened in the future as the physics simulation stabilizes.
The per-axis bounds approach using
vec_inside_limitsis good practice and more robust than exact checks.Note: The static analysis warnings about unused lambda arguments (
qandqd) are false positives—both parameters are required by thetest_body_statesignature defined innewton/examples/__init__.py.newton/examples/mpm/example_mpm_anymal.py (1)
306-314: Consider tightening angular velocity bounds and addressing code duplication.The angular velocity bounds (±0.5 to ±0.8 rad/s) are relatively wide, mirroring the same concern raised in the PR discussion. While necessary after the inertia fix, consider whether these can be tightened as the simulation stabilizes.
There's significant code duplication between this test method and
example_robot_anymal_c_walk.py:test(). Both files use similar test patterns withforward_vel_min/maxand directional checks. Consider extracting common test logic into a shared helper function innewton/examples/__init__.pyto improve maintainability.Based on the PR discussion, this test has been failing on CI while passing locally. The velocity bounds may need further adjustment to account for environment-specific behavior differences between local and CI environments.
Note: The static analysis warning about the unused
qparameter at line 312 is a false positive—both parameters are required by thetest_body_statesignature.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/examples/mpm/example_mpm_anymal.py(1 hunks)newton/examples/robot/example_robot_anymal_c_walk.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
newton/examples/robot/example_robot_anymal_c_walk.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
newton/examples/mpm/example_mpm_anymal.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
🪛 Ruff (0.13.3)
newton/examples/robot/example_robot_anymal_c_walk.py
243-243: Unused lambda argument: qd
(ARG005)
252-252: Unused lambda argument: q
(ARG005)
newton/examples/mpm/example_mpm_anymal.py
297-297: Unused lambda argument: qd
(ARG005)
303-303: Unused lambda argument: qd
(ARG005)
312-312: Unused lambda argument: q
(ARG005)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (3)
newton/examples/robot/example_robot_anymal_c_walk.py (1)
239-244: LGTM: Directional check validates forward progress.The directional threshold of 9.0 for 500 frames is appropriate for verifying the robot moved forward as expected.
newton/examples/mpm/example_mpm_anymal.py (2)
297-297: LGTM: Relaxed height threshold appropriate for MPM simulation.The height check threshold of 0.1 is reasonable given the sand interaction and aligns with the same threshold used in
example_robot_anymal_c_walk.py.Note: The static analysis warning about the unused
qdparameter is a false positive—bothqandqdare required by thetest_body_statesignature.
299-304: LGTM: Directional check validates forward progress.The threshold of 0.9 for 100 frames is proportionally consistent with the 9.0 threshold for 500 frames in
example_robot_anymal_c_walk.py.Note: The static analysis warning about the unused
qdparameter is a false positive—both parameters are required by thetest_body_statesignature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/examples/robot/example_robot_anymal_c_walk.py (1)
246-254: Consider prefixing unused lambda argument with underscore.The lambda at line 252 does not use the
qargument. Per Python convention and the static analysis hint, consider prefixing it with_qto indicate it's intentionally unused.Apply this diff:
- lambda q, qd: newton.utils.vec_inside_limits(qd, forward_vel_min, forward_vel_max), + lambda _q, qd: newton.utils.vec_inside_limits(qd, forward_vel_min, forward_vel_max),The velocity bounds approach is appropriate given the inertia fix.
newton/examples/mpm/example_mpm_anymal.py (1)
306-314: Consider prefixing unused lambda arguments with underscore.Per Python convention and static analysis hints, unused lambda arguments should be prefixed with underscore. This applies to:
- Line 297:
qdis unused- Line 303:
qdis unused- Line 312:
qis unusedApply this diff:
- lambda q, qd: q[2] > 0.1, + lambda q, _qd: q[2] > 0.1, ) newton.examples.test_body_state( self.model, self.state_0, "the robot went in the right direction", - lambda q, qd: q[1] > 0.9, # This threshold assumes 100 frames + lambda q, _qd: q[1] > 0.9, # This threshold assumes 100 frames ) forward_vel_min = wp.spatial_vector(-0.2, 0.9, -0.2, -0.8, -0.5, -0.5) forward_vel_max = wp.spatial_vector(0.2, 1.1, 0.2, 0.8, 0.5, 0.5) newton.examples.test_body_state( self.model, self.state_0, "the robot is moving forward and not falling", - lambda q, qd: newton.utils.vec_inside_limits(qd, forward_vel_min, forward_vel_max), + lambda _q, qd: newton.utils.vec_inside_limits(qd, forward_vel_min, forward_vel_max),The velocity bounds approach is appropriate. Based on past review comments, the angular velocity tolerances were already discussed and narrowed as needed.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/examples/mpm/example_mpm_anymal.py(1 hunks)newton/examples/robot/example_robot_anymal_c_walk.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
newton/examples/mpm/example_mpm_anymal.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
newton/examples/robot/example_robot_anymal_c_walk.py (2)
newton/examples/__init__.py (1)
test_body_state(38-94)newton/_src/utils/__init__.py (1)
vec_inside_limits(211-218)
🪛 Ruff (0.13.3)
newton/examples/mpm/example_mpm_anymal.py
297-297: Unused lambda argument: qd
(ARG005)
303-303: Unused lambda argument: qd
(ARG005)
312-312: Unused lambda argument: q
(ARG005)
newton/examples/robot/example_robot_anymal_c_walk.py
243-243: Unused lambda argument: qd
(ARG005)
252-252: Unused lambda argument: q
(ARG005)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (4)
newton/examples/robot/example_robot_anymal_c_walk.py (2)
232-237: LGTM: Ground clearance test updated appropriately.The lowered threshold (0.1m) is reasonable after the inertia fix and aligns with the similar change in
example_mpm_anymal.py.
239-244: Directional threshold calibration verified
The test runs for 500 frames (num-framesinadd_example_test), so theq[1] > 9.0threshold is correctly aligned.newton/examples/mpm/example_mpm_anymal.py (2)
293-298: LGTM: Ground clearance threshold consistent across examples.The lowered threshold (0.1m) matches the change in
example_robot_anymal_c_walk.py, maintaining consistency across the test suite.
299-304: Directional movement check appropriately scaled.The threshold of 0.9 for 100 frames appears proportional to the 9.0 threshold for 500 frames in
example_robot_anymal_c_walk.py.
|
The reason for needing to increase velocity bounds partly is that the linear velocity in world space drifts slightly to the left when running for longer, which is I think more of a neural network issue. anymal_c_walk.webm |
I've also noticed this drifting issue, so I think now with these new test bounds it looks good. Thanks @lenroe-nv! |
…ewton-physics#870) Co-authored-by: Eric Heiden <eheiden@nvidia.com>
…ewton-physics#870) Co-authored-by: Eric Heiden <eheiden@nvidia.com>
Description
Fixes a bug in the urdf parser that resulted in large inertias for links with zero mass (issue #838).
The behavior is now consistent with the mjcf parser.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Bug Fixes
Tests