Skip to content

Conversation

@thowell
Copy link
Collaborator

@thowell thowell commented Sep 19, 2025

adds tests from mujoco's engine_forward_tests.cc https://github.com/google-deepmind/mujoco/blob/main/test/engine/engine_forward_test.cc

fixes _next_activation launch in _rk_perturb_state

@thowell thowell linked an issue Sep 19, 2025 that may be closed by this pull request
Copy link
Collaborator

@erikfrey erikfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, let's see what we can do about improving CPU test coverage.

d_arr = d_arr[: d.nefc.numpy()[0]]
_assert_eq(d_arr, getattr(mjd, arr), arr)

@absltest.skipIf(not wp.get_device().is_cuda, "Skipping test that requires GPU.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOVE seeing these tests, thank you for doing this.

Let's think of using ScopedCapture inside a test as a last resort... I'm definitely guilty of using it, but only where my imagination comes up short on how to test expected behavior with only a few steps. It would be nice to keep the property that our test coverage on CPU only is still pretty good.

Some of these unit tests only step 10 times or so - you can check but I think for 10 steps, the test time is not too bad if we step via CPU.

For the tests that are stepping hundreds of times, can you take a look and decide if there's anyway to reduce the number of steps and still have a meaningful test?

If after reviewing these two things, we still have some tests with the absltest.skipIf at the top, that's OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve test coverage of functions and models

2 participants