[Tests] Consolidate overlapping isaaclab_newton tests - #7603
Open
StafaH wants to merge 2 commits into
Open
Conversation
Merge the dynamics accessor shape and FK-refresh checks into one sim per asset, fold paired sensor and schema checks that rebuilt the same scene, remove permanently skipped tests, and skip the non-strict xfail contact sensor cases that ran to completion without producing a verdict.
StafaH
requested review from
AntoineRichard,
hujc7 and
kellyguo11
as code owners
September 5, 2026 15:05
Contributor
Greptile SummaryThis PR reduces the Newton test-suite runtime by removing permanently skipped cases and consolidating tests that previously created duplicate simulation scenes.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking test-quality issue in the consolidated articulation refresh test. The runtime optimizations generally preserve the tested behavior, but the first Jacobian read refreshes shared FK state and prevents later assertions from detecting accessor-specific refresh regressions. Files Needing Attention: source/isaaclab_newton/test/assets/test_articulation.py Important Files Changed
Reviews (1): Last reviewed commit: "Consolidate overlapping isaaclab_newton ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4003
to
+4013
| assert not torch.allclose(J_link_0, articulation.data.body_link_jacobian_w.torch, atol=1e-3), ( | ||
| "body_link_jacobian_w did not change after manual joint write; FK trigger likely missing" | ||
| ) | ||
| assert not torch.allclose(J_com_0, J_com_1, atol=1e-3), ( | ||
| "body_com_jacobian_w did not change after manual joint write — FK trigger likely missing before eval_jacobian." | ||
| assert not torch.allclose(J_com_0, articulation.data.body_com_jacobian_w.torch, atol=1e-3), ( | ||
| "body_com_jacobian_w did not change after manual joint write; FK trigger likely missing" | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("num_articulations", [1]) | ||
| @pytest.mark.parametrize("device", test_devices(DeviceScope.CUDA)) | ||
| @pytest.mark.parametrize("articulation_type", ["panda", "anymal"]) | ||
| @pytest.mark.parametrize("gravity_enabled", [False]) | ||
| @pytest.mark.isaacsim_ci | ||
| def test_mass_matrix_refreshes_after_manual_joint_write( | ||
| sim, num_articulations, device, articulation_type, gravity_enabled | ||
| ): | ||
| """After ``write_joint_position_to_sim_index`` (no sim step), the mass matrix read | ||
| must reflect the new joint state. | ||
|
|
||
| The mass matrix depends on ``q`` (joint positions) through the body-spatial-inertia | ||
| transformation in eval_mass_matrix's ``compute_body_spatial_inertia`` step, which | ||
| reads ``state.body_q``. Same FK-staleness pattern as the Jacobian. | ||
| """ | ||
| articulation_cfg = generate_articulation_cfg(articulation_type=articulation_type) | ||
| articulation, _ = generate_articulation(articulation_cfg, num_articulations, device=device) | ||
| sim.reset() | ||
| sim.step() | ||
| articulation.update(sim.cfg.dt) | ||
|
|
||
| M_0 = articulation.data.mass_matrix.torch.clone() | ||
| q_target = articulation.data.joint_pos.torch.clone() + 0.5 | ||
| env_ids = wp.array([0], dtype=wp.int32, device=device) | ||
| articulation.write_joint_position_to_sim_index(position=q_target, env_ids=env_ids) | ||
| M_1 = articulation.data.mass_matrix.torch.clone() | ||
|
|
||
| assert not torch.allclose(M_0, M_1, atol=1e-3), ( | ||
| "mass_matrix did not change after manual joint write — " | ||
| "FK trigger likely missing before eval_mass_matrix (compute_body_spatial_inertia " | ||
| "reads stale state.body_q)." | ||
| assert not torch.allclose(M_0, articulation.data.mass_matrix.torch, atol=1e-3), ( | ||
| "mass_matrix did not change after manual joint write; FK trigger likely missing" | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("num_articulations", [1]) | ||
| @pytest.mark.parametrize("device", test_devices(DeviceScope.CUDA)) | ||
| @pytest.mark.parametrize("articulation_type", ["panda"]) | ||
| @pytest.mark.isaacsim_ci | ||
| def test_gravity_compensation_refreshes_after_manual_joint_write(sim, num_articulations, device, articulation_type): | ||
| """After ``write_joint_position_to_sim_index`` (no sim step), the gravity | ||
| compensation read must reflect the new joint state. | ||
|
|
||
| ``g(q)`` depends on ``q`` through the RNEA pass in ``eval_inverse_dynamics_passive``, | ||
| which reads ``state.body_q``. Same FK-staleness pattern as the Jacobian and | ||
| the mass matrix. Gravity stays enabled (the default) — with gravity off, | ||
| ``g(q)`` is identically zero and the assert would be vacuous. | ||
| """ | ||
| articulation_cfg = generate_articulation_cfg(articulation_type=articulation_type) | ||
| articulation, _ = generate_articulation(articulation_cfg, num_articulations, device=device) | ||
| sim.reset() | ||
| sim.step() | ||
| articulation.update(sim.cfg.dt) | ||
|
|
||
| g_0 = articulation.data.gravity_compensation_forces.torch.clone() | ||
| q_target = articulation.data.joint_pos.torch.clone() + 0.5 | ||
| env_ids = wp.array([0], dtype=wp.int32, device=device) | ||
| articulation.write_joint_position_to_sim_index(position=q_target, env_ids=env_ids) | ||
| g_1 = articulation.data.gravity_compensation_forces.torch.clone() | ||
|
|
||
| assert not torch.allclose(g_0, g_1, atol=1e-3), ( | ||
| "gravity_compensation_forces did not change after manual joint write — " | ||
| "FK trigger likely missing before eval_inverse_dynamics_passive." | ||
| assert not torch.allclose(g_0, articulation.data.gravity_compensation_forces.torch, atol=1e-3), ( | ||
| "gravity_compensation_forces did not change after manual joint write; FK trigger likely missing" |
Contributor
There was a problem hiding this comment.
The test reads body_link_jacobian_w first, which refreshes the FK state shared by all four accessors. The later body_com_jacobian_w, mass_matrix, and gravity_compensation_forces checks therefore run with FK already refreshed and would still pass if their own refresh calls were removed. Re-invalidate FK before each check or parameterize the test so each accessor is the first read after a joint write.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Isaac Lab Review Bot
The test-only consolidation largely preserves existing coverage and reduces repeated simulation setup, but the merged dynamics-accessor refresh test no longer independently verifies FK invalidation for three accessors.
- Design and architecture: Grouping related checks into shared scenes is appropriate for reducing Newton test runtime. However, FK-refresh behavior is stateful, so sharing one stale-to-fresh transition across multiple accessor assertions weakens the intended regression coverage.
- API: No production or public API is changed. The test helper cleanup is internally consistent, and the
.skipchangelog fragment appropriately records a non-user-visible test-only change. - Implementation: In
test_dynamics_accessors_refresh_after_manual_joint_write, one joint write marks FK stale, but the firstbody_link_jacobian_wread refreshes the shared FK state. Subsequent reads ofbody_com_jacobian_w,mass_matrix, andgravity_compensation_forcestherefore cannot detect removal of their own FK-refresh triggers. Re-mark FK stale, such as by repeating the joint write, before each accessor assertion.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| "mass_matrix did not change after manual joint write — " | ||
| "FK trigger likely missing before eval_mass_matrix (compute_body_spatial_inertia " | ||
| "reads stale state.body_q)." | ||
| assert not torch.allclose(M_0, articulation.data.mass_matrix.torch, atol=1e-3), ( |
Contributor
There was a problem hiding this comment.
🟡 Warning · Implementation — Chained reads void three FK-refresh assertions
All four accessors are read after one joint write. As the test's own comment states, the write marks FK stale via a single _fk_timestamp, so the first read (body_link_jacobian_w) runs forward() and refreshes body_q. The later COM-Jacobian, mass-matrix and gravity reads then see fresh poses even if their own FK trigger were removed — the exact regressions the docstring claims to catch. Re-write the joint state before each accessor read.
…aaclab-newton-tests-2 # Conflicts: # source/isaaclab_newton/test/assets/test_articulation.py
Contributor
Author
|
run-ci |
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
Second pass over the
isaaclab_newtontest suite after #7306. The goal is the same: cut CI runtime by removing tests that produce no verdict and by collapsing groups of tests that rebuild the same simulation to check one accessor each, while keeping every distinct behavioral check.Removed (no verdict produced)
test_articulation.py:test_initialization_hand_with_tendonsandtest_spatial_tendonsalways skipped (shadow hand / spatial tendons are unsupported on Newton); theshadow_handfixture branch and its sim config go with them.test_setting_gains_from_cfg_dictwas byte-for-byte the same check astest_setting_gains_from_cfg.test_rigid_object.py: four permanently skipped tests (kinematic bodies, no-friction, static friction, restitution) and the material helper only they used.test_rigid_object_collection.py: the permanently skipped kinematic test.test_contact_sensor.py: thenewton_contactscases of the force-matrix, contact-point and finger-isolation tests were non-strictxfails. They ran the full 240-step settle (and the Allegro drop withflakyreruns) and could neither fail nor pass the build. They are nowskipwith the same reason, so the known gap stays visible in the report.Consolidated (same assertions, fewer simulations)
J/M/g× fixed/floating base) each spun up their own sim per batch size. Onetest_dynamics_accessor_shapesreads all three accessors per (asset, batch) and keeps the positive-diagonal mass-matrix check. 12 sims → 4.test_dynamics_accessors_refresh_after_manual_joint_writethat checksJ,Mandgon both assets. Gravity stays on so theg(q)assertion is not vacuous.test_views_xform_prim_newton.py): body/shape-path rejection, world-attached read/write, and clone-plan vs post-reset resolution each collapsed into one scene.SimulationContexts.test_site_injection.py: two tests made the identical call and split the assertions.Before and after measurement
Per-file wall time from the CI-style per-file runner on the same workstation (RTX PRO 6000), warm Warp cache, changed files only:
assets/test_articulation.pysensors/test_contact_sensor.pyassets/test_rigid_object_collection.pyassets/test_rigid_object.pysim/test_views_xform_prim_newton.pysensors/test_frame_transformer.pysensors/test_newton_raycast_sensor.pysensors/test_joint_wrench_sensor.pysensors/test_pva.pysensors/test_imu.pysim/test_newton_schemas.pysensors/test_site_injection.pyThe whole
isaaclab_newtonsuite measured 16 min 20 s before the change on this machine; the changed files account for the full 108 s saved (about 11%). The largest single win istest_articulation.py, which is 30% of the suite.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there