Tighten example test assertions with empirical bounds#2396
Open
adenzler-nvidia wants to merge 10 commits intonewton-physics:mainfrom
Open
Tighten example test assertions with empirical bounds#2396adenzler-nvidia wants to merge 10 commits intonewton-physics:mainfrom
adenzler-nvidia wants to merge 10 commits intonewton-physics:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMany example tests under Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Replace loose "didn't explode" tests with tight empirical bounds across all 49 example files. Run each example, capture the final body/particle state, and write assertions against the observed values. - Fill 5 previously-empty test_final() stubs (3 IK, UR10, cable_bundle_hysteresis) with real physics checks - Add centroid, bounding-box, and velocity checks to ~20 examples that only checked "above ground" - Tighten ~10 examples with extremely loose bounds (bbox < 10-20, z > -0.5) to observed values + small margin - Add CPU vs CUDA guards where frame counts differ significantly - Tolerances: ±2 % body positions, ±5 % particle centroids, 1.5x observed peak velocity Skipped examples that were already tight: cartpole, basic_joints, brick_stacking, diffsim (6), sensor_contact, selection_cartpole.
adenzler-nvidia
force-pushed
the
adenzler/tighten-example-tests
branch
from
8be4ee7 to
3544782
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
adenzler-nvidia
force-pushed
the
adenzler/tighten-example-tests
branch
2 times, most recently
from
66311ee to
2cb1359
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
newton/examples/robot/example_robot_panda_hydro.py (1)
438-438: Document the widened XY tolerance to preserve test intent.Line 438 increases the bound to
0.08; please add a short inline note with the observed max XY error (CPU/CUDA) used to choose this value so future updates keep this check intentionally tight.💡 Suggested non-functional update
- tolerance_xy = 0.08 + # Empirical max XY cup-center offset observed across CPU/CUDA runs: ~X.XX m. + # Keep a small guard band to reduce flakiness without masking regressions. + tolerance_xy = 0.08🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/robot/example_robot_panda_hydro.py` at line 438, Add an inline comment next to the tolerance_xy = 0.08 assignment documenting why the XY bound was widened: include the observed maximum XY error values on CPU and CUDA (e.g., "observed max XY error: CPU ~X.XX m, CUDA ~Y.YY m") and a short note that these measurements informed choosing 0.08 to preserve test intent; place this comment immediately beside the tolerance_xy variable so future maintainers can see the empirical justification (reference: tolerance_xy in example_robot_panda_hydro.py).newton/examples/basic/example_basic_heightfield.py (1)
127-128: Prefer linear speed for this sphere velocity assertion.
np.linalg.norm(body_qd[body_idx])includes angular components too. If this check is intended as translational speed, use only XYZ linear velocity for clearer semantics.♻️ Proposed refinement
- vel = float(np.linalg.norm(body_qd[body_idx])) + vel = float(np.linalg.norm(body_qd[body_idx, :3])) assert vel < 15.0, f"Sphere body {body_idx} moving too fast: vel={vel:.4f}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/basic/example_basic_heightfield.py` around lines 127 - 128, The assertion currently computes vel using np.linalg.norm(body_qd[body_idx]) which includes angular components; change it to compute linear speed from only the XYZ translational components (e.g., the first three elements of body_qd[body_idx]) and use that value in the assert message for Sphere body {body_idx}; update the variable name if needed (e.g., linear_vel) and maintain the same threshold and formatted assertion message.newton/examples/basic/example_basic_conveyor.py (1)
419-420: Extract and check linear velocity explicitly.Line 419 computes
np.linalg.norm(body_qd[body_idx]), which mixes linear velocity [m/s] (indices 0–2) with angular velocity [rad/s] (indices 3–5). This is dimensionally inconsistent and makes the thresholdvel < 8.0ambiguous. Usenp.linalg.norm(body_qd[body_idx, :3])to check only translational speed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/basic/example_basic_conveyor.py` around lines 419 - 420, The check currently computes vel = float(np.linalg.norm(body_qd[body_idx])) which mixes linear and angular velocity; update the computation to use only translational components (use np.linalg.norm(body_qd[body_idx, :3]) or equivalent) and adjust the assert message to indicate it's checking linear speed (e.g., "linear velocity" and appropriate units) referencing the variables body_qd, body_idx and the vel/assert expression so the threshold applies only to translational speed.newton/examples/sensors/example_sensor_imu.py (1)
179-190: Scope these checks to cube bodies explicitly.These predicates are cube-specific, but with
indices=Nonethey run on every body. That makes the test fragile if this example later adds helper/static bodies.♻️ Suggested patch
class Example: def __init__(self, viewer, args): @@ self.visual_cubes = [] self.visual_fillers = [] self.imu_sites = [] + self.cube_body_indices = [] @@ body = builder.add_body( xform=wp.transform( @@ ) ) + self.cube_body_indices.append(body) @@ newton.examples.test_body_state( self.model, self.state_0, "cubes resting at correct height", lambda q, qd: wp.abs(q[2] - 0.2) < 0.05, + indices=self.cube_body_indices, ) newton.examples.test_body_state( self.model, self.state_0, "cubes have zero velocity", lambda q, qd: wp.length(qd) < 1e-3, + indices=self.cube_body_indices, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/sensors/example_sensor_imu.py` around lines 179 - 190, The predicates passed to newton.examples.test_body_state are intended for the cube bodies but currently run on all bodies; update both calls to include an indices parameter that restricts checks to the cube bodies (e.g., indices=<list_of_cube_body_indices>) so only cube bodies are evaluated. Locate the test_body_state invocations (newton.examples.test_body_state(self.model, self.state_0, ...)) and supply a list of cube body indices—either from an existing attribute like self.cube_indices / self.cube_body_indices or by deriving indices from self.model (e.g., by body name matching) —for both the "cubes resting at correct height" and "cubes have zero velocity" checks.newton/examples/cloth/example_cloth_hanging.py (1)
194-213: Solver branch comment is misleading.The
elsebranch on Line 201 handlesvbd,semi_implicit, andxpbdsolvers, but the comment only references VBD. If the empirical bounds were calibrated against VBD runs, this could cause false failures for semi_implicit or xpbd.Consider either:
- Adding solver-specific branches for semi_implicit and xpbd with their own calibrated bounds, or
- Updating the comment to clarify these bounds apply to all non-style3d solvers.
💡 Suggested fix if solver-specific handling is needed
if self.solver_type == "style3d": # Style3D: observed centroid [-1.60, -0.43, 1.34] assert np.allclose(centroid, [-1.60, -0.43, 1.34], atol=[0.30, 0.30, 0.30]), ( f"Centroid drift (style3d): {centroid}" ) else: - # VBD: observed centroid [-1.80, 0.35, 1.20], min_z=0.049 + # Non-style3d solvers (vbd, semi_implicit, xpbd): observed centroid [-1.80, 0.35, 1.20], min_z=0.049 min_z = np.min(particle_q[:, 2]) assert min_z > 0.02, f"Ground penetration: min_z={min_z}" assert np.allclose(centroid, [-1.80, 0.35, 1.20], atol=[0.30, 0.30, 0.30]), ( - f"Centroid drift (vbd): {centroid}" + f"Centroid drift ({self.solver_type}): {centroid}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/cloth/example_cloth_hanging.py` around lines 194 - 213, The comment and branching around solver_type is misleading because the else branch currently covers vbd, semi_implicit, and xpbd but only references VBD; update the logic in the block handling solver_type (the conditional that checks self.solver_type) to either add separate branches for "vbd", "semi_implicit", and "xpbd" with their own calibrated assertions (using centroid, particle_q/min_z, bbox_size, max_vel) or, if the same bounds are valid for all non-style3d solvers, change the comment to clearly state these assertions apply to all non-style3d solvers; locate the conditional on self.solver_type and adjust comments or split into explicit elifs for each solver name and tailor the tolerances/thresholds accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/examples/cable/example_cable_bundle_hysteresis.py`:
- Around line 421-445: The test currently skips all checks when
self.state_0.body_q or self.state_0.body_qd is None; add an explicit fail-fast
assertion at the start of the validation (e.g., in test_final or the block
containing the shown code) that both self.state_0.body_q and
self.state_0.body_qd are not None with a clear message like "Missing final body
state: body_q or body_qd is None", then proceed with the existing numpy
conversions and assertions; reference self.state_0.body_q and
self.state_0.body_qd to locate where to add this check.
In `@newton/examples/cloth/example_cloth_franka.py`:
- Around line 674-676: The assertion on max_vel computed from particle_qd
(max_vel = np.max(np.linalg.norm(particle_qd, axis=1))) is too tight and flakes
on AWS GPU; update the velocity cap in the assertion (the assert using max_vel
and the message "Excessive velocity") to a slightly higher value (e.g., >=12.5)
or implement device-specific thresholds (check device type and choose a looser
limit for GPU runs) so CI no longer fails intermittently.
In `@newton/examples/ik/example_ik_custom.py`:
- Line 317: The code builds obstacle_center from the stored initial
self.obstacle_center, which can be stale; instead read the live obstacle center
from the runtime gizmo/objective state before the clearance assertion (e.g.,
replace uses of self.obstacle_center when forming obstacle_center with the
current value exposed by the gizmo or objective state such as
self.gizmo.state['center'] or the appropriate self.objective_state key), so that
the clearance check uses the updated obstacle position (update the line creating
obstacle_center and any downstream assertions to use that live value).
In `@newton/examples/mpm/example_mpm_granular.py`:
- Around line 156-169: These empirical centroid/bbox/velocity assertions
(variables positions, centroid, bbox, max_vel) are specific to the cube collider
but currently run unconditionally; wrap them so they only execute when the scene
is using the cube collider by checking the instance collider attribute (e.g. if
getattr(self, "collider", None) == "cube" or getattr(self, "collider_type",
None) == "cube":) and then perform the existing assertions inside that block;
this ensures wedge/concave/none runs skip these cube-specific checks.
In `@newton/examples/sensors/example_sensor_tiled_camera.py`:
- Around line 439-444: The "all bodies have zero velocity" assertion using
newton.examples.test_body_state against self.model and self.state is invalid
because step() only updates joint_q (kinematics) and does not update joint_qd,
so the test can falsely pass; fix by removing that assertion call or replace it
with a check that computes expected joint velocities from the kinematic/joint_q
updates (e.g., derive joint_qd from joint_q differences over time or from the
controller) and assert those expected non-zero velocities instead, referencing
the same test function and the step() updates to ensure the validation matches
how the scene is driven.
---
Nitpick comments:
In `@newton/examples/basic/example_basic_conveyor.py`:
- Around line 419-420: The check currently computes vel =
float(np.linalg.norm(body_qd[body_idx])) which mixes linear and angular
velocity; update the computation to use only translational components (use
np.linalg.norm(body_qd[body_idx, :3]) or equivalent) and adjust the assert
message to indicate it's checking linear speed (e.g., "linear velocity" and
appropriate units) referencing the variables body_qd, body_idx and the
vel/assert expression so the threshold applies only to translational speed.
In `@newton/examples/basic/example_basic_heightfield.py`:
- Around line 127-128: The assertion currently computes vel using
np.linalg.norm(body_qd[body_idx]) which includes angular components; change it
to compute linear speed from only the XYZ translational components (e.g., the
first three elements of body_qd[body_idx]) and use that value in the assert
message for Sphere body {body_idx}; update the variable name if needed (e.g.,
linear_vel) and maintain the same threshold and formatted assertion message.
In `@newton/examples/cloth/example_cloth_hanging.py`:
- Around line 194-213: The comment and branching around solver_type is
misleading because the else branch currently covers vbd, semi_implicit, and xpbd
but only references VBD; update the logic in the block handling solver_type (the
conditional that checks self.solver_type) to either add separate branches for
"vbd", "semi_implicit", and "xpbd" with their own calibrated assertions (using
centroid, particle_q/min_z, bbox_size, max_vel) or, if the same bounds are valid
for all non-style3d solvers, change the comment to clearly state these
assertions apply to all non-style3d solvers; locate the conditional on
self.solver_type and adjust comments or split into explicit elifs for each
solver name and tailor the tolerances/thresholds accordingly.
In `@newton/examples/robot/example_robot_panda_hydro.py`:
- Line 438: Add an inline comment next to the tolerance_xy = 0.08 assignment
documenting why the XY bound was widened: include the observed maximum XY error
values on CPU and CUDA (e.g., "observed max XY error: CPU ~X.XX m, CUDA ~Y.YY
m") and a short note that these measurements informed choosing 0.08 to preserve
test intent; place this comment immediately beside the tolerance_xy variable so
future maintainers can see the empirical justification (reference: tolerance_xy
in example_robot_panda_hydro.py).
In `@newton/examples/sensors/example_sensor_imu.py`:
- Around line 179-190: The predicates passed to newton.examples.test_body_state
are intended for the cube bodies but currently run on all bodies; update both
calls to include an indices parameter that restricts checks to the cube bodies
(e.g., indices=<list_of_cube_body_indices>) so only cube bodies are evaluated.
Locate the test_body_state invocations
(newton.examples.test_body_state(self.model, self.state_0, ...)) and supply a
list of cube body indices—either from an existing attribute like
self.cube_indices / self.cube_body_indices or by deriving indices from
self.model (e.g., by body name matching) —for both the "cubes resting at correct
height" and "cubes have zero velocity" checks.
🪄 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: 7f69c3b3-351c-4bba-b11a-4a63f3c6a94c
📒 Files selected for processing (49)
newton/examples/basic/example_basic_conveyor.pynewton/examples/basic/example_basic_heightfield.pynewton/examples/basic/example_basic_pendulum.pynewton/examples/basic/example_basic_plotting.pynewton/examples/basic/example_basic_shapes.pynewton/examples/basic/example_basic_urdf.pynewton/examples/cable/example_cable_bundle_hysteresis.pynewton/examples/cable/example_cable_pile.pynewton/examples/cable/example_cable_twist.pynewton/examples/cable/example_cable_y_junction.pynewton/examples/cloth/example_cloth_bending.pynewton/examples/cloth/example_cloth_franka.pynewton/examples/cloth/example_cloth_h1.pynewton/examples/cloth/example_cloth_hanging.pynewton/examples/cloth/example_cloth_poker_cards.pynewton/examples/cloth/example_cloth_rollers.pynewton/examples/cloth/example_cloth_style3d.pynewton/examples/cloth/example_cloth_twist.pynewton/examples/contacts/example_nut_bolt_hydro.pynewton/examples/contacts/example_nut_bolt_sdf.pynewton/examples/contacts/example_pyramid.pynewton/examples/ik/example_ik_custom.pynewton/examples/ik/example_ik_franka.pynewton/examples/ik/example_ik_h1.pynewton/examples/mpm/example_mpm_anymal.pynewton/examples/mpm/example_mpm_beam_twist.pynewton/examples/mpm/example_mpm_grain_rendering.pynewton/examples/mpm/example_mpm_granular.pynewton/examples/mpm/example_mpm_multi_material.pynewton/examples/mpm/example_mpm_snow_ball.pynewton/examples/mpm/example_mpm_twoway_coupling.pynewton/examples/mpm/example_mpm_viscous.pynewton/examples/multiphysics/example_softbody_dropping_to_cloth.pynewton/examples/multiphysics/example_softbody_gift.pynewton/examples/robot/example_robot_allegro_hand.pynewton/examples/robot/example_robot_anymal_c_walk.pynewton/examples/robot/example_robot_anymal_d.pynewton/examples/robot/example_robot_g1.pynewton/examples/robot/example_robot_h1.pynewton/examples/robot/example_robot_panda_hydro.pynewton/examples/robot/example_robot_policy.pynewton/examples/robot/example_robot_ur10.pynewton/examples/selection/example_selection_articulations.pynewton/examples/selection/example_selection_materials.pynewton/examples/selection/example_selection_multiple.pynewton/examples/sensors/example_sensor_imu.pynewton/examples/sensors/example_sensor_tiled_camera.pynewton/examples/softbody/example_softbody_franka.pynewton/examples/softbody/example_softbody_hanging.py
Comment on lines
+421
to
+445
| if self.state_0.body_q is not None and self.state_0.body_qd is not None: | ||
| body_positions = self.state_0.body_q.numpy() | ||
| body_velocities = self.state_0.body_qd.numpy() | ||
|
|
||
| # Numerical stability | ||
| assert np.isfinite(body_positions).all(), "Non-finite values in body positions" | ||
| assert np.isfinite(body_velocities).all(), "Non-finite values in body velocities" | ||
|
|
||
| # Exclude kinematic obstacle bodies (they teleport during release phase) | ||
| obstacle_set = set(self.obstacle_bodies) | ||
| cable_indices = [i for i in range(body_positions.shape[0]) if i not in obstacle_set] | ||
| cable_positions = body_positions[cable_indices] | ||
| cable_velocities = body_velocities[cable_indices] | ||
|
|
||
| # Position bounds for cable bodies (observed z_range=[0.019, 0.300]) | ||
| z_positions = cable_positions[:, 2] | ||
| min_z = np.min(z_positions) | ||
| max_z = np.max(z_positions) | ||
| assert min_z > -0.05, f"Cable penetrated ground: min_z = {min_z:.3f}" | ||
| assert max_z < 0.6, f"Cables too high: max_z = {max_z:.3f}" | ||
|
|
||
| # Velocity bounds for cable bodies (observed max_body_vel ~1.41) | ||
| assert (np.abs(cable_velocities) < 3.0).all(), ( | ||
| f"Cable velocities too large: max = {np.max(np.abs(cable_velocities)):.3f}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Fail fast when body state is missing instead of silently passing.
Right now, this test can pass without executing any assertions if either array is None. That creates a false-green path for regressions in state setup.
✅ Proposed fix
def test_final(self):
"""Test cable bundle hysteresis simulation for stability and correctness (called after simulation)."""
- if self.state_0.body_q is not None and self.state_0.body_qd is not None:
- body_positions = self.state_0.body_q.numpy()
- body_velocities = self.state_0.body_qd.numpy()
+ if self.state_0.body_q is None or self.state_0.body_qd is None:
+ raise RuntimeError("Body state is not available.")
+
+ body_positions = self.state_0.body_q.numpy()
+ body_velocities = self.state_0.body_qd.numpy()
- # Numerical stability
- assert np.isfinite(body_positions).all(), "Non-finite values in body positions"
- assert np.isfinite(body_velocities).all(), "Non-finite values in body velocities"
+ # Numerical stability
+ assert np.isfinite(body_positions).all(), "Non-finite values in body positions"
+ assert np.isfinite(body_velocities).all(), "Non-finite values in body velocities"
- # Exclude kinematic obstacle bodies (they teleport during release phase)
- obstacle_set = set(self.obstacle_bodies)
- cable_indices = [i for i in range(body_positions.shape[0]) if i not in obstacle_set]
- cable_positions = body_positions[cable_indices]
- cable_velocities = body_velocities[cable_indices]
+ # Exclude kinematic obstacle bodies (they teleport during release phase)
+ obstacle_set = set(self.obstacle_bodies)
+ cable_indices = [i for i in range(body_positions.shape[0]) if i not in obstacle_set]
+ cable_positions = body_positions[cable_indices]
+ cable_velocities = body_velocities[cable_indices]As per coding guidelines: newton/examples/**/*.py: “Implement test_final() — runs after the example completes to verify simulation state is valid.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if self.state_0.body_q is not None and self.state_0.body_qd is not None: | |
| body_positions = self.state_0.body_q.numpy() | |
| body_velocities = self.state_0.body_qd.numpy() | |
| # Numerical stability | |
| assert np.isfinite(body_positions).all(), "Non-finite values in body positions" | |
| assert np.isfinite(body_velocities).all(), "Non-finite values in body velocities" | |
| # Exclude kinematic obstacle bodies (they teleport during release phase) | |
| obstacle_set = set(self.obstacle_bodies) | |
| cable_indices = [i for i in range(body_positions.shape[0]) if i not in obstacle_set] | |
| cable_positions = body_positions[cable_indices] | |
| cable_velocities = body_velocities[cable_indices] | |
| # Position bounds for cable bodies (observed z_range=[0.019, 0.300]) | |
| z_positions = cable_positions[:, 2] | |
| min_z = np.min(z_positions) | |
| max_z = np.max(z_positions) | |
| assert min_z > -0.05, f"Cable penetrated ground: min_z = {min_z:.3f}" | |
| assert max_z < 0.6, f"Cables too high: max_z = {max_z:.3f}" | |
| # Velocity bounds for cable bodies (observed max_body_vel ~1.41) | |
| assert (np.abs(cable_velocities) < 3.0).all(), ( | |
| f"Cable velocities too large: max = {np.max(np.abs(cable_velocities)):.3f}" | |
| ) | |
| if self.state_0.body_q is None or self.state_0.body_qd is None: | |
| raise RuntimeError("Body state is not available.") | |
| body_positions = self.state_0.body_q.numpy() | |
| body_velocities = self.state_0.body_qd.numpy() | |
| # Numerical stability | |
| assert np.isfinite(body_positions).all(), "Non-finite values in body positions" | |
| assert np.isfinite(body_velocities).all(), "Non-finite values in body velocities" | |
| # Exclude kinematic obstacle bodies (they teleport during release phase) | |
| obstacle_set = set(self.obstacle_bodies) | |
| cable_indices = [i for i in range(body_positions.shape[0]) if i not in obstacle_set] | |
| cable_positions = body_positions[cable_indices] | |
| cable_velocities = body_velocities[cable_indices] | |
| # Position bounds for cable bodies (observed z_range=[0.019, 0.300]) | |
| z_positions = cable_positions[:, 2] | |
| min_z = np.min(z_positions) | |
| max_z = np.max(z_positions) | |
| assert min_z > -0.05, f"Cable penetrated ground: min_z = {min_z:.3f}" | |
| assert max_z < 0.6, f"Cables too high: max_z = {max_z:.3f}" | |
| # Velocity bounds for cable bodies (observed max_body_vel ~1.41) | |
| assert (np.abs(cable_velocities) < 3.0).all(), ( | |
| f"Cable velocities too large: max = {np.max(np.abs(cable_velocities)):.3f}" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/examples/cable/example_cable_bundle_hysteresis.py` around lines 421 -
445, The test currently skips all checks when self.state_0.body_q or
self.state_0.body_qd is None; add an explicit fail-fast assertion at the start
of the validation (e.g., in test_final or the block containing the shown code)
that both self.state_0.body_q and self.state_0.body_qd are not None with a clear
message like "Missing final body state: body_q or body_qd is None", then proceed
with the existing numpy conversions and assertions; reference
self.state_0.body_q and self.state_0.body_qd to locate where to add this check.
|
|
||
| # Verify collision avoidance: check that all monitored links maintain | ||
| # clearance from the obstacle sphere | ||
| obstacle_center = np.array([self.obstacle_center[0], self.obstacle_center[1], self.obstacle_center[2]]) |
Contributor
There was a problem hiding this comment.
Use the live obstacle center in the clearance assertion.
At Line 317, the assertion uses self.obstacle_center (initial value), but obstacle position can be updated through gizmo/objective state during the run. This can make the clearance check compare against a stale center.
🔧 Proposed fix
- obstacle_center = np.array([self.obstacle_center[0], self.obstacle_center[1], self.obstacle_center[2]])
+ obstacle_center = self.obstacle_centers.numpy()[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| obstacle_center = np.array([self.obstacle_center[0], self.obstacle_center[1], self.obstacle_center[2]]) | |
| obstacle_center = self.obstacle_centers.numpy()[0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/examples/ik/example_ik_custom.py` at line 317, The code builds
obstacle_center from the stored initial self.obstacle_center, which can be
stale; instead read the live obstacle center from the runtime gizmo/objective
state before the clearance assertion (e.g., replace uses of self.obstacle_center
when forming obstacle_center with the current value exposed by the gizmo or
objective state such as self.gizmo.state['center'] or the appropriate
self.objective_state key), so that the clearance check uses the updated obstacle
position (update the line creating obstacle_center and any downstream assertions
to use that live value).
Comment on lines
+156
to
+169
| # Empirical centroid, bbox, and velocity checks (100 frames, cube collider) | ||
| positions = self.state_0.particle_q.numpy() | ||
| velocities = self.state_0.particle_qd.numpy() | ||
| centroid = np.mean(positions, axis=0) | ||
| bbox = np.max(np.max(positions, axis=0) - np.min(positions, axis=0)) | ||
| max_vel = np.max(np.linalg.norm(velocities, axis=1)) | ||
|
|
||
| assert abs(centroid[0] - (-0.31)) < max(0.02, 0.31 * 0.05), f"Centroid X out of range: {centroid[0]}" | ||
| assert abs(centroid[1] - 0.0) < 0.02, f"Centroid Y out of range: {centroid[1]}" | ||
| assert abs(centroid[2] - 0.28) < max(0.02, 0.28 * 0.05), f"Centroid Z out of range: {centroid[2]}" | ||
| assert bbox < 8.97 + max(0.05, 8.97 * 0.05), f"Bounding box too large: {bbox}" | ||
| assert max_vel < max(0.002, 5.26 * 1.5), f"Max velocity too high: {max_vel}" | ||
| assert np.min(positions[:, 2]) > -0.001 - 0.02, f"Min Z too low: {np.min(positions[:, 2])}" | ||
|
|
Contributor
There was a problem hiding this comment.
Gate cube-specific empirical bounds to cube mode only.
At Line 156, the comment says these limits are for the cube collider, but they currently run for all collider modes. That can fail valid wedge/concave/none runs.
💡 Suggested fix
- # Empirical centroid, bbox, and velocity checks (100 frames, cube collider)
- positions = self.state_0.particle_q.numpy()
- velocities = self.state_0.particle_qd.numpy()
- centroid = np.mean(positions, axis=0)
- bbox = np.max(np.max(positions, axis=0) - np.min(positions, axis=0))
- max_vel = np.max(np.linalg.norm(velocities, axis=1))
-
- assert abs(centroid[0] - (-0.31)) < max(0.02, 0.31 * 0.05), f"Centroid X out of range: {centroid[0]}"
- assert abs(centroid[1] - 0.0) < 0.02, f"Centroid Y out of range: {centroid[1]}"
- assert abs(centroid[2] - 0.28) < max(0.02, 0.28 * 0.05), f"Centroid Z out of range: {centroid[2]}"
- assert bbox < 8.97 + max(0.05, 8.97 * 0.05), f"Bounding box too large: {bbox}"
- assert max_vel < max(0.002, 5.26 * 1.5), f"Max velocity too high: {max_vel}"
- assert np.min(positions[:, 2]) > -0.001 - 0.02, f"Min Z too low: {np.min(positions[:, 2])}"
+ if self.collider == "cube":
+ # Empirical centroid, bbox, and velocity checks (100 frames, cube collider)
+ positions = self.state_0.particle_q.numpy()
+ velocities = self.state_0.particle_qd.numpy()
+ centroid = np.mean(positions, axis=0)
+ bbox = np.max(np.max(positions, axis=0) - np.min(positions, axis=0))
+ max_vel = np.max(np.linalg.norm(velocities, axis=1))
+
+ assert abs(centroid[0] - (-0.31)) < max(0.02, 0.31 * 0.05), f"Centroid X out of range: {centroid[0]}"
+ assert abs(centroid[1] - 0.0) < 0.02, f"Centroid Y out of range: {centroid[1]}"
+ assert abs(centroid[2] - 0.28) < max(0.02, 0.28 * 0.05), f"Centroid Z out of range: {centroid[2]}"
+ assert bbox < 8.97 + max(0.05, 8.97 * 0.05), f"Bounding box too large: {bbox}"
+ assert max_vel < max(0.002, 5.26 * 1.5), f"Max velocity too high: {max_vel}"
+ assert np.min(positions[:, 2]) > -0.001 - 0.02, f"Min Z too low: {np.min(positions[:, 2])}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Empirical centroid, bbox, and velocity checks (100 frames, cube collider) | |
| positions = self.state_0.particle_q.numpy() | |
| velocities = self.state_0.particle_qd.numpy() | |
| centroid = np.mean(positions, axis=0) | |
| bbox = np.max(np.max(positions, axis=0) - np.min(positions, axis=0)) | |
| max_vel = np.max(np.linalg.norm(velocities, axis=1)) | |
| assert abs(centroid[0] - (-0.31)) < max(0.02, 0.31 * 0.05), f"Centroid X out of range: {centroid[0]}" | |
| assert abs(centroid[1] - 0.0) < 0.02, f"Centroid Y out of range: {centroid[1]}" | |
| assert abs(centroid[2] - 0.28) < max(0.02, 0.28 * 0.05), f"Centroid Z out of range: {centroid[2]}" | |
| assert bbox < 8.97 + max(0.05, 8.97 * 0.05), f"Bounding box too large: {bbox}" | |
| assert max_vel < max(0.002, 5.26 * 1.5), f"Max velocity too high: {max_vel}" | |
| assert np.min(positions[:, 2]) > -0.001 - 0.02, f"Min Z too low: {np.min(positions[:, 2])}" | |
| if self.collider == "cube": | |
| # Empirical centroid, bbox, and velocity checks (100 frames, cube collider) | |
| positions = self.state_0.particle_q.numpy() | |
| velocities = self.state_0.particle_qd.numpy() | |
| centroid = np.mean(positions, axis=0) | |
| bbox = np.max(np.max(positions, axis=0) - np.min(positions, axis=0)) | |
| max_vel = np.max(np.linalg.norm(velocities, axis=1)) | |
| assert abs(centroid[0] - (-0.31)) < max(0.02, 0.31 * 0.05), f"Centroid X out of range: {centroid[0]}" | |
| assert abs(centroid[1] - 0.0) < 0.02, f"Centroid Y out of range: {centroid[1]}" | |
| assert abs(centroid[2] - 0.28) < max(0.02, 0.28 * 0.05), f"Centroid Z out of range: {centroid[2]}" | |
| assert bbox < 8.97 + max(0.05, 8.97 * 0.05), f"Bounding box too large: {bbox}" | |
| assert max_vel < max(0.002, 5.26 * 1.5), f"Max velocity too high: {max_vel}" | |
| assert np.min(positions[:, 2]) > -0.001 - 0.02, f"Min Z too low: {np.min(positions[:, 2])}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/examples/mpm/example_mpm_granular.py` around lines 156 - 169, These
empirical centroid/bbox/velocity assertions (variables positions, centroid,
bbox, max_vel) are specific to the cube collider but currently run
unconditionally; wrap them so they only execute when the scene is using the cube
collider by checking the instance collider attribute (e.g. if getattr(self,
"collider", None) == "cube" or getattr(self, "collider_type", None) == "cube":)
and then perform the existing assertions inside that block; this ensures
wedge/concave/none runs skip these cube-specific checks.
Comment on lines
+439
to
+444
| newton.examples.test_body_state( | ||
| self.model, | ||
| self.state, | ||
| "all bodies have zero velocity", | ||
| lambda q, qd: wp.length(qd) < 1e-3, | ||
| ) |
Contributor
There was a problem hiding this comment.
"zero velocity" check is not validating real settling here.
Line 443 reads body twist, but step() drives kinematics via joint_q updates without updating joint_qd, so this can pass even while the scene is actively animated. Consider removing this assertion (or computing expected non-zero joint_qd and asserting against that instead).
✅ Minimal fix
- newton.examples.test_body_state(
- self.model,
- self.state,
- "all bodies have zero velocity",
- lambda q, qd: wp.length(qd) < 1e-3,
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| newton.examples.test_body_state( | |
| self.model, | |
| self.state, | |
| "all bodies have zero velocity", | |
| lambda q, qd: wp.length(qd) < 1e-3, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/examples/sensors/example_sensor_tiled_camera.py` around lines 439 -
444, The "all bodies have zero velocity" assertion using
newton.examples.test_body_state against self.model and self.state is invalid
because step() only updates joint_q (kinematics) and does not update joint_qd,
so the test can falsely pass; fix by removing that assertion call or replace it
with a check that computes expected joint velocities from the kinematic/joint_q
updates (e.g., derive joint_qd from joint_q differences over time or from the
controller) and assert those expected non-zero velocities instead, referencing
the same test function and the step() updates to ensure the validation matches
how the scene is driven.
2cb1359 to
d14af1b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
newton/examples/ik/example_ik_franka.py (1)
146-152: Consider asserting end-effector orientation too.Line 146 to Line 152 validates position only; a regression in
IKObjectiveRotationcould still pass. Adding a quaternion assertion would close that gap.♻️ Proposed enhancement
ee_pos = body_q[self.ee_index, :3] ee_target = np.array([0.0967, 0.0, 0.9034]) assert np.allclose(ee_pos, ee_target, atol=0.01), ( f"End-effector position {ee_pos} too far from target {ee_target}" ) + ee_quat = body_q[self.ee_index, 3:7] + q_target = wp.transform_get_rotation(self.ee_tf) + ee_target_quat = np.array([q_target[0], q_target[1], q_target[2], q_target[3]]) + assert np.allclose(np.abs(ee_quat), np.abs(ee_target_quat), atol=0.02), ( + f"End-effector orientation {ee_quat} too far from target {ee_target_quat}" + )Based on learnings: "Quaternions in builder/test assertions use Warp’s XYZW convention (identity [0, 0, 0, 1]). In newton/tests/test_import_mjcf.py ... prefer identity/non-identity checks that account for q and -q equivalence (e.g., via np.abs on components)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/ik/example_ik_franka.py` around lines 146 - 152, Add an assertion that the end-effector orientation matches the expected quaternion in addition to the position check: after obtaining body_q[self.ee_index] compare its orientation quaternion (using Warp's XYZW convention, identity [0,0,0,1]) against the expected quaternion and handle q vs -q equivalence (e.g., normalize and compare either absolute component-wise np.allclose or compare min(np.allclose(q, q_ref), np.allclose(q, -q))). Reference body_q, ee_index and the IKObjectiveRotation result when adding this check.newton/examples/ik/example_ik_h1.py (1)
156-167: Add orientation assertions for the four IK rotation objectives.Line 156 to Line 167 validates EE positions, but does not validate EE rotations even though rotation objectives are part of the solve. A per-link quaternion check would make this test cover the full objective set.
Based on learnings: "Quaternions in builder/test assertions use Warp’s XYZW convention (identity [0, 0, 0, 1]). In newton/tests/test_import_mjcf.py ... prefer identity/non-identity checks that account for q and -q equivalence (e.g., via np.abs on components)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/ik/example_ik_h1.py` around lines 156 - 167, The test currently asserts only end-effector positions (ee_expected / body_q) but omits rotation checks; add per-link quaternion assertions for the four IK rotation objectives by extracting the quaternion components from body_q (the rotation slice following the position) for indices 5, 10, 16, 33 and comparing to the expected quaternions using Warp's XYZW convention (identity [0,0,0,1] or the specific target quaternions used in the IK objectives), accounting for q and -q equivalence by comparing absolute values (e.g., np.allclose(np.abs(actual_q), np.abs(expected_q), atol=...)). Ensure you reference the same indices as ee_expected and keep tolerance consistent with position checks.newton/examples/mpm/example_mpm_grain_rendering.py (1)
82-89: Assert per-axis spread instead of only the largest extent.
bbox = max(span_x, span_y, span_z)can still hide lateral spreading regressions whenever Z stays dominant. For a granular pile, it would be safer to record and assert the X/Y/Z spans separately so this test verifies spreading in all axes, not just the largest envelope. Based on learnings: "In granular heap simulations, when particles fall onto a plane, they spread outward in all horizontal directions due to impact and granular flow, not just settle vertically. Test assertions should verify spreading in all axes, not just along the gravity/up-axis."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/mpm/example_mpm_grain_rendering.py` around lines 82 - 89, The test currently computes bbox = np.max(np.max(positions, axis=0) - np.min(positions, axis=0)) which hides per-axis spreading; change to compute per-axis spans (e.g., spans = np.max(positions, axis=0) - np.min(positions, axis=0)) and assert spans[0], spans[1], and spans[2] against axis-appropriate thresholds instead of the single bbox check; update the failure messages to reference span_x/span_y/span_z and keep the existing centroid and max_vel asserts (use centroid, positions, velocities identifiers to locate the code).newton/examples/softbody/example_softbody_hanging.py (1)
154-155: Minor redundancy: min Z is already validated bytest_particle_state.The
p_lower[2] = 0.21bound intest_particle_state(Line 146) already ensures all particles are at or above Z=0.21. The separate assertion on Line 155 duplicates this check. Consider removing it to reduce redundancy, or keep it if you prefer the explicit assertion message for debugging.🔧 Optional simplification
- # Min Z check: observed 0.23, allow down to 0.21 - assert min_pos[2] > 0.21, f"Excessive ground penetration: z_min={min_pos[2]:.4f}" -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/softbody/example_softbody_hanging.py` around lines 154 - 155, Remove the redundant ground-penetration assertion by deleting the explicit check "assert min_pos[2] > 0.21" in the example_softbody_hanging test, since test_particle_state already enforces p_lower[2] = 0.21; alternatively, if you want an explicit debug message keep the assertion but tighten it (e.g., > 0.22) or change the message to reference test_particle_state to avoid duplication—locate min_pos and test_particle_state in the same file to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/examples/mpm/example_mpm_snow_ball.py`:
- Around line 356-360: The per-particle domain predicate passed to
newton.examples.test_particle_state currently only checks z (lambda q, qd: q[2]
> -7.0 and q[2] < 17.0) and must also enforce X/Y terrain bounds using self.L_x
and self.L_y; modify that lambda (or replace with a named function) to require
q[0] within [-self.L_x/2, self.L_x/2] and q[1] within [-self.L_y/2, self.L_y/2]
in addition to the existing z checks so each particle is tested against full
(x,y,z) terrain domain.
---
Nitpick comments:
In `@newton/examples/ik/example_ik_franka.py`:
- Around line 146-152: Add an assertion that the end-effector orientation
matches the expected quaternion in addition to the position check: after
obtaining body_q[self.ee_index] compare its orientation quaternion (using Warp's
XYZW convention, identity [0,0,0,1]) against the expected quaternion and handle
q vs -q equivalence (e.g., normalize and compare either absolute component-wise
np.allclose or compare min(np.allclose(q, q_ref), np.allclose(q, -q))).
Reference body_q, ee_index and the IKObjectiveRotation result when adding this
check.
In `@newton/examples/ik/example_ik_h1.py`:
- Around line 156-167: The test currently asserts only end-effector positions
(ee_expected / body_q) but omits rotation checks; add per-link quaternion
assertions for the four IK rotation objectives by extracting the quaternion
components from body_q (the rotation slice following the position) for indices
5, 10, 16, 33 and comparing to the expected quaternions using Warp's XYZW
convention (identity [0,0,0,1] or the specific target quaternions used in the IK
objectives), accounting for q and -q equivalence by comparing absolute values
(e.g., np.allclose(np.abs(actual_q), np.abs(expected_q), atol=...)). Ensure you
reference the same indices as ee_expected and keep tolerance consistent with
position checks.
In `@newton/examples/mpm/example_mpm_grain_rendering.py`:
- Around line 82-89: The test currently computes bbox = np.max(np.max(positions,
axis=0) - np.min(positions, axis=0)) which hides per-axis spreading; change to
compute per-axis spans (e.g., spans = np.max(positions, axis=0) -
np.min(positions, axis=0)) and assert spans[0], spans[1], and spans[2] against
axis-appropriate thresholds instead of the single bbox check; update the failure
messages to reference span_x/span_y/span_z and keep the existing centroid and
max_vel asserts (use centroid, positions, velocities identifiers to locate the
code).
In `@newton/examples/softbody/example_softbody_hanging.py`:
- Around line 154-155: Remove the redundant ground-penetration assertion by
deleting the explicit check "assert min_pos[2] > 0.21" in the
example_softbody_hanging test, since test_particle_state already enforces
p_lower[2] = 0.21; alternatively, if you want an explicit debug message keep the
assertion but tighten it (e.g., > 0.22) or change the message to reference
test_particle_state to avoid duplication—locate min_pos and test_particle_state
in the same file to apply the change.
🪄 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: 93b14d0c-934b-43b4-b19f-b7e91109e05e
📒 Files selected for processing (49)
newton/examples/basic/example_basic_conveyor.pynewton/examples/basic/example_basic_heightfield.pynewton/examples/basic/example_basic_pendulum.pynewton/examples/basic/example_basic_plotting.pynewton/examples/basic/example_basic_shapes.pynewton/examples/basic/example_basic_urdf.pynewton/examples/cable/example_cable_bundle_hysteresis.pynewton/examples/cable/example_cable_pile.pynewton/examples/cable/example_cable_twist.pynewton/examples/cable/example_cable_y_junction.pynewton/examples/cloth/example_cloth_bending.pynewton/examples/cloth/example_cloth_franka.pynewton/examples/cloth/example_cloth_h1.pynewton/examples/cloth/example_cloth_hanging.pynewton/examples/cloth/example_cloth_poker_cards.pynewton/examples/cloth/example_cloth_rollers.pynewton/examples/cloth/example_cloth_style3d.pynewton/examples/cloth/example_cloth_twist.pynewton/examples/contacts/example_nut_bolt_hydro.pynewton/examples/contacts/example_nut_bolt_sdf.pynewton/examples/contacts/example_pyramid.pynewton/examples/ik/example_ik_custom.pynewton/examples/ik/example_ik_franka.pynewton/examples/ik/example_ik_h1.pynewton/examples/mpm/example_mpm_anymal.pynewton/examples/mpm/example_mpm_beam_twist.pynewton/examples/mpm/example_mpm_grain_rendering.pynewton/examples/mpm/example_mpm_granular.pynewton/examples/mpm/example_mpm_multi_material.pynewton/examples/mpm/example_mpm_snow_ball.pynewton/examples/mpm/example_mpm_twoway_coupling.pynewton/examples/mpm/example_mpm_viscous.pynewton/examples/multiphysics/example_softbody_dropping_to_cloth.pynewton/examples/multiphysics/example_softbody_gift.pynewton/examples/robot/example_robot_allegro_hand.pynewton/examples/robot/example_robot_anymal_c_walk.pynewton/examples/robot/example_robot_anymal_d.pynewton/examples/robot/example_robot_g1.pynewton/examples/robot/example_robot_h1.pynewton/examples/robot/example_robot_panda_hydro.pynewton/examples/robot/example_robot_policy.pynewton/examples/robot/example_robot_ur10.pynewton/examples/selection/example_selection_articulations.pynewton/examples/selection/example_selection_materials.pynewton/examples/selection/example_selection_multiple.pynewton/examples/sensors/example_sensor_imu.pynewton/examples/sensors/example_sensor_tiled_camera.pynewton/examples/softbody/example_softbody_franka.pynewton/examples/softbody/example_softbody_hanging.py
✅ Files skipped from review due to trivial changes (4)
- newton/examples/basic/example_basic_shapes.py
- newton/examples/basic/example_basic_heightfield.py
- newton/examples/mpm/example_mpm_twoway_coupling.py
- newton/examples/robot/example_robot_panda_hydro.py
🚧 Files skipped from review as they are similar to previous changes (29)
- newton/examples/robot/example_robot_allegro_hand.py
- newton/examples/contacts/example_nut_bolt_hydro.py
- newton/examples/robot/example_robot_anymal_c_walk.py
- newton/examples/robot/example_robot_anymal_d.py
- newton/examples/basic/example_basic_urdf.py
- newton/examples/basic/example_basic_plotting.py
- newton/examples/selection/example_selection_multiple.py
- newton/examples/basic/example_basic_pendulum.py
- newton/examples/robot/example_robot_policy.py
- newton/examples/robot/example_robot_g1.py
- newton/examples/cable/example_cable_pile.py
- newton/examples/cable/example_cable_twist.py
- newton/examples/robot/example_robot_h1.py
- newton/examples/sensors/example_sensor_tiled_camera.py
- newton/examples/basic/example_basic_conveyor.py
- newton/examples/cloth/example_cloth_bending.py
- newton/examples/mpm/example_mpm_multi_material.py
- newton/examples/mpm/example_mpm_viscous.py
- newton/examples/mpm/example_mpm_anymal.py
- newton/examples/cable/example_cable_bundle_hysteresis.py
- newton/examples/mpm/example_mpm_granular.py
- newton/examples/robot/example_robot_ur10.py
- newton/examples/multiphysics/example_softbody_dropping_to_cloth.py
- newton/examples/cable/example_cable_y_junction.py
- newton/examples/cloth/example_cloth_rollers.py
- newton/examples/ik/example_ik_custom.py
- newton/examples/cloth/example_cloth_poker_cards.py
- newton/examples/softbody/example_softbody_franka.py
- newton/examples/mpm/example_mpm_beam_twist.py
Comment on lines
356
to
360
| newton.examples.test_particle_state( | ||
| self.state_0, | ||
| "all particles are within the terrain domain", | ||
| lambda q, qd: q[2] > -20.0 and q[2] < 30.0, | ||
| lambda q, qd: q[2] > -7.0 and q[2] < 17.0, | ||
| ) |
Contributor
There was a problem hiding this comment.
Include X/Y bounds in the terrain-domain predicate.
This now tightens only q[2], so particles can still leave the heightfield laterally and satisfy "all particles are within the terrain domain". The centroid/bbox checks below are aggregate-only, so they will not reliably catch a few X/Y outliers. Please fold the self.L_x / self.L_y limits into this per-particle check too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/examples/mpm/example_mpm_snow_ball.py` around lines 356 - 360, The
per-particle domain predicate passed to newton.examples.test_particle_state
currently only checks z (lambda q, qd: q[2] > -7.0 and q[2] < 17.0) and must
also enforce X/Y terrain bounds using self.L_x and self.L_y; modify that lambda
(or replace with a named function) to require q[0] within [-self.L_x/2,
self.L_x/2] and q[1] within [-self.L_y/2, self.L_y/2] in addition to the
existing z checks so each particle is tested against full (x,y,z) terrain
domain.
GPU observed max_vel=0.47, exceeding the 0.45 bound calibrated on CPU (0.18-0.29). Raise to 0.55 to accommodate GPU variance.
CI observed 18.9 cm/s on some GPU architectures, exceeding the 18.0 bound. Widen to 22.0 to accommodate variance while still catching genuine blowups.
…ample-tests # Conflicts: # newton/examples/robot/example_robot_panda_hydro.py
CI g7e.2xlarge observed centroid z=1.23, exceeding the 1.5 lower bound calibrated locally. Lower to 0.8 and widen bbox from 8 to 10 to accommodate architecture-dependent tumbling trajectories.
GPU Tests on AWS observed centroid x = -0.056 with a 0.05 bound. Widen to 0.10 to cover architecture-level variance, consistent with the softbody_gift bound widening.
# Conflicts: # newton/examples/cloth/example_cloth_twist.py
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
1 participant
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.
Description
Replace loose "didn't explode" example tests with tight empirical bounds across all 49 example files. Each example was run, its final body/particle state captured, and assertions written against observed values with small tolerances.
What changed:
test_final()stubs (3 IK, UR10, cable_bundle_hysteresis) with real physics checksbbox < 20,z > -0.5) to observed values + small marginTolerance strategy: ±2% body positions (min 1 cm), ±5% particle centroids (min 2 cm), 1.5× observed peak velocity. Start tight, loosen per-example only if flaky.
Skipped (already tight):
robot_cartpole,basic_joints,brick_stacking, all 6 diffsim examples,sensor_contact,selection_cartpole.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Full example test suite (98 tests) passes on both CPU and CUDA, including torch-dependent variants:
Pre-commit clean:
Summary by CodeRabbit