Repository navigation
Parametrize sim tests over integrators - #142
Conversation
Extend the test suite to cover the different integrators, as requested in learnsyslab#106: - test_sim_step now runs the full pipeline for every integrator - test_floor_penetration now runs for every integrator - add test_free_fall_convergence, which verifies that the trajectory under every integrator converges to a 10x higher frequency reference sim in free fall. This catches integration breakage like the broken symplectic integrators observed in learnsyslab#102
amacati
left a comment
amacati
left a comment
There was a problem hiding this comment.
I left some comments and linked a reference PR. Thanks for working on this!
|
|
||
|
|
||
| @pytest.mark.unit | ||
| @pytest.mark.parametrize("integrator", Integrator) |
There was a problem hiding this comment.
How many variants does this add? While I'm in favor of sanity-checking all combinations at least once, this is likely going to massively increase the number of tests we run and thereby the test duration.
There was a problem hiding this comment.
It added 240 variants (40 -> 280 in test_sim.py), roughly doubling the unit test duration on CPU (~+100 s locally). Resolved in 4b48be4 by reverting the parametrization and moving the integrator coverage into a dedicated tests/unit/test_integrators.py following the approach of #122: 12 variants (4 dynamics x 3 integrators), ~13 s on CPU.
Per the review, rework the approach to follow the one sketched in learnsyslab#122: - Revert the integrator parametrization of test_sim_step and test_floor_penetration, and drop the free-fall convergence test. The parametrized test_sim_step alone added 240 test variants (40 -> 280) and roughly doubled the unit test duration on CPU. - Add tests/unit/test_integrators.py instead, which compares the complete sim.data pytree of every dynamics x integrator combination against a separately stepped default-integrator Sim after two steps. This adds 12 fast variants (~13 s on CPU) and covers the so_rpy families where the broken symplectic integrator from learnsyslab#102 lived.
amacati
left a comment
amacati
left a comment
There was a problem hiding this comment.
One minor thing I realized, otherwise this looks good to go
| sim = Sim(dynamics=dynamics, integrator=integrator, device=device) | ||
| default_sim = Sim(dynamics=dynamics, device=device) | ||
| sim.step(2) | ||
| default_sim.step(2) |
There was a problem hiding this comment.
One more thing: Could you spawn the drones in the air and then do all the checks below? With the floor clipping, it's likely that we would not see a difference in the dynamics
There was a problem hiding this comment.
Done in bc7776c — both sims now spawn at z=2.0 before stepping, so the floor clipping can no longer mask differences between the integration schemes.
At the default spawn on the floor, floor clipping masks any difference in the dynamics between integrators. Spawn both sims at z=2.0 instead.
--------- Co-authored-by: Martin Schuck <57562633+amacati@users.noreply.github.com>
Fixes #106
Summary
Adds integrator coverage to the test suite following the approach sketched in #122. #102 showed that a broken symplectic integrator went unnoticed because the tests only exercised the default integrator.
tests/unit/test_integrators.pycompares the completesim.datapytree of every dynamics x integrator combination against a separately stepped default-integratorSimafter two simulation steps: tree structures must match, PRNG key data is compared exactly, floating-point arrays withrtol=2e-2, atol=1e-4(rotor speeds differ by several percent between Euler and RK4), everything else exactly. Parametrizing over dynamics keeps theso_rpyfamilies where the Add clipping to rotor_vel #102 bug lived inside the comparison.Testing
pytest tests/unit: 713 passed, 0 failed, 43 skipped, on CPU.ruff formatandruff checkpass.