Add remaining menagerie models #22370
Conversation
RussTedrake
added
the
release notes: none
There was a problem hiding this comment.
looks like the CI error is real (but likely tolerance related). I'm investigating the numerics.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/parsing/test/detail_mujoco_parser_test.cc line 204 at r1 (raw file):
{"boston_dynamics_spot/spot_arm", kItWorks}, {"flybody/fruitfly", kItWorks}, {"flybody/scene", kItWorks},
btw -- i've noticed that a few of these models parse a LOT more slowly than the others. Should I be worried?
There was a problem hiding this comment.
I've fixed the numerical issue, but now debug builds hit a timeout. I'm not surprised... a few of the models run exceptionally slowly in debug mode. Please let me know what you think.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
There was a problem hiding this comment.
I checked the licenses on all of the new stuff and everything is good.
In terms of test speed, one thing we can do for developer usability is add shard_count = 4, (or whatever number) to the BUILD file for the test. That will divide the test cases across 4 processes running in parallel, which means better latency when running locally.
However, we also don't to paper over a real problem. If a test case is running very slowly, that might indicate that an end user parsing a similar kind of model might also bog down. We should probably try to build an understanding about why it's so slow. That might need to block this PR, though, since we're not actually adding any new parser code here, so any slowness bugs are already there on master. In that case, the focus for now should be making sure the new test cases do no annoy developers, either by making CI flaky for the buildcop, or making local test runs annoyingly slow.
I can circle back at a later point and try to help with that, too. For now, I'll just say first feature review pass is complete and LGTM pending the small nits below and the question about speed.
Reviewed 3 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/parsing/detail_mujoco_parser.cc line 154 at r2 (raw file):
Here's what the MuJoCo documentation says:
The first 3 numbers are the X axis of the frame. The next 3 numbers are the Y axis of the frame, which is automatically made orthogonal to the X axis. The Z axis is then defined as the cross-product of the X and Y axes.
Is ProjectToRotationMatrix the correct way to perform "Y axis ... is automatically made orthogonal to the X axis". My mental model is that "Project..." might also alter X beyond just normalizing it.
My intuition is that we should be normalizing X to a unit vector as the first step, then dealing with Y as a second step (also unit), and then Z-is-cross is trivial and we don't need to Project since we already have orthogonal unit vectors.
In any case, if you want to have this change here you probably need someone besides me (i.e., someone who knows math) to sign off on it.
If the code ends up with any nuance, it would be good to make it a helper function and unit test it directly, instead of only via fully-fledged models.
tools/workspace/mujoco_menagerie_internal/repository.bzl line 9 at r2 (raw file):
name = name, repository = "google-deepmind/mujoco_menagerie", commit = "469893211c41d5da9c314f5ab58059fa17c8e360",
Working
If the January upgrade cadence doesn't upgrade this before this PR lands, then we need to document the "upgrade to latest commit" in our commit message, for the release notes.
multibody/parsing/test/detail_mujoco_parser_test.cc line 177 at r2 (raw file):
// unusable. const char* kItWorks = "";
nit Constants named kFoo must be const -- these new ones aren't b/c they are all missing their second const (to be non-reassignable):
const char* const kItWorks = "";
...Fixing that alone would be sufficient.
However, to avoid the trap of "how to put the right const on a char pointer" there are generally two better options to spell this:
(1) With constexpr as an array:
constexpr char kItWorks[] = "";
(2) With constexpr as a string_view:
constexpr std::string_view kItWorks{""};
Using string_view helps simplify the test case code, also -- no need to convert from char* to std::string to check for empty:
--- a/multibody/parsing/test/detail_mujoco_parser_test.cc
+++ b/multibody/parsing/test/detail_mujoco_parser_test.cc
@@ -144,10 +144,9 @@ const char* dm_control_models[] = {
INSTANTIATE_TEST_SUITE_P(DeepMindControl, DeepMindControlTest,
testing::ValuesIn(dm_control_models));
-class MujocoMenagerieTest
- : public MujocoParserTest,
- public testing::WithParamInterface<std::pair<const char*, const char*>> {
-};
+class MujocoMenagerieTest : public MujocoParserTest,
+ public testing::WithParamInterface<
+ std::pair<const char*, std::string_view>> {};
TEST_P(MujocoMenagerieTest, MujocoMenagerie) {
// Confirm successful parsing of the MuJoCo models in the DeepMind control
@@ -163,7 +162,7 @@ TEST_P(MujocoMenagerieTest, MujocoMenagerie) {
// For this test, ignore all warnings.
warning_records_.clear();
- if (!std::string(error_regex).empty()) {
+ if (!error_regex.empty()) {
EXPECT_THAT(TakeError(), MatchesRegex(error_regex));
// For now, we'll just capture the *first* error.
error_records_.clear();
@@ -181,7 +180,7 @@ const char* kMoreThanOneOrientation = ".*more than one orientation.*";
const char* kCapsuleSize = ".*size attribute for capsule geom.*";
} // namespace KnownErrors
-const std::pair<const char*, const char*> mujoco_menagerie_models[] = {
+const std::pair<const char*, std::string_view> mujoco_menagerie_models[] = {
{"agility_cassie/cassie", KnownErrors::kNonUniformScale},
{"agility_cassie/scene", KnownErrors::kNonUniformScale},
{"aloha/aloha", kItWorks},multibody/parsing/test/detail_mujoco_parser_test.cc line 233 at r2 (raw file):
{"hello_robot_stretch/stretch", KnownErrors::kNonUniformScale}, {"hello_robot_stretch_3/scene", KnownErrors::kNonUniformScale}, {"hello_robot_stretch_3/stretch", KnownErrors::kNonUniformScale},
We generally frown on committing commented-out code. Instead of commenting code, better would be to have a flag like kDontEvenTry or whatever phrasing you prefer, and then have the test code recognize that and skip it.
Or alternatively, change it to be purely a comment without code, like:
// We hope to parse these, but currently they throw in Debug ... etc.
// - hello_robot_stretch/scene
// - hello_robot_stretch/stretch
|
@rpoyner-tri my Ctrl-C profiling of one of the slow test cases here points to |
There was a problem hiding this comment.
I suppose since that hot spot points to a pre-existing problem unrelated to this PR, a plausible plan here would be to skip the really slow test cases (e.g., franka_emika_panda_panda_nohand) when using in a Debug build. Would that be a good approach, or would you rather get the proximity engine fixed first, or withdraw those files from the suite with a TODO for now?
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/parsing/test/detail_mujoco_parser_test.cc line 145 at r2 (raw file):
"swimmer", "walker"}; INSTANTIATE_TEST_SUITE_P(DeepMindControl, DeepMindControlTest, testing::ValuesIn(dm_control_models));
nit See RussTedrake#57 for a cleanup to be merged into here. With that change, the suite-based test cases have nice names in the output.
There was a problem hiding this comment.
FWIW, some are really slow. From my (new fast) Puget with debug build flags:
[ RUN ] MujocoMenagerie/MujocoMenagerieTest.MujocoMenagerie/27
[ OK ] MujocoMenagerie/MujocoMenagerieTest.MujocoMenagerie/27 (68937 ms)
[ RUN ] MujocoMenagerie/MujocoMenagerieTest.MujocoMenagerie/28
[ OK ] MujocoMenagerie/MujocoMenagerieTest.MujocoMenagerie/28 (68495 ms)
Given that each of those two take over a minute, I'd say we just withdraw them with a TODO. We should fix the other problems, but that might take some time.
Beyond those, there are probably another minute's worth of medium slow models (4-15 sec).
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
There was a problem hiding this comment.
Worth considering is splitting the mountain-of-files tests from the basic parser test. They arguably different beasts that answer different questions. The basic test should be about "did I implement the feature correctly, or somehow break it?" with detailed feedback and fine-grained cases, and the mountain-of-files tests are acceptance tests, without concern for case details.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
There was a problem hiding this comment.
Done.
- I've added the shard_count=4,
- I've commented out the most painful tests.
- I've split them out into a different test file. (I was thinking of doing that myself).
+@rpoyner-tri for platform review, since you're already here?
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
multibody/parsing/detail_mujoco_parser.cc line 154 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Here's what the MuJoCo documentation says:
The first 3 numbers are the X axis of the frame. The next 3 numbers are the Y axis of the frame, which is automatically made orthogonal to the X axis. The Z axis is then defined as the cross-product of the X and Y axes.
Is
ProjectToRotationMatrixthe correct way to perform "Y axis ... is automatically made orthogonal to the X axis". My mental model is that "Project..." might also alter X beyond just normalizing it.My intuition is that we should be normalizing X to a unit vector as the first step, then dealing with Y as a second step (also unit), and then Z-is-cross is trivial and we don't need to Project since we already have orthogonal unit vectors.
In any case, if you want to have this change here you probably need someone besides me (i.e., someone who knows math) to sign off on it.
If the code ends up with any nuance, it would be good to make it a helper function and unit test it directly, instead of only via fully-fledged models.
I liked your logic and implemented it... but it was not sufficient. Specifically
R.col(0) = xyaxes.head<3>().normalized();
R.col(1) = xyaxes.tail<3>().normalized();
R.col(2) = R.col(0).cross(R.col(1));
return RigidTransformd(RotationMatrixd(R), pos);
was not enough to avoid the error (for the cassie model).
My personal view is that the projection here is ambiguous.. it's recovering from the fact that the orientation is being specified with floating point strings to finite precision.
xyaxes="0.7922 -0.60599 -0.072096 0.60349 0.79547 -0.054922"
I don't think one rounding scheme is obviously better than another. So I'm inclined to leave my ProjectToRotationMatrix fix and call it good.
multibody/parsing/test/detail_mujoco_parser_test.cc line 204 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
btw -- i've noticed that a few of these models parse a LOT more slowly than the others. Should I be worried?
Done. (as discussed above)
multibody/parsing/test/detail_mujoco_parser_test.cc line 145 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit See RussTedrake#57 for a cleanup to be merged into here. With that change, the suite-based test cases have nice names in the output.
Done. Nice. Thanks.
multibody/parsing/test/detail_mujoco_parser_test.cc line 177 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Constants named
kFoomust be const -- these new ones aren't b/c they are all missing their second const (to be non-reassignable):const char* const kItWorks = ""; ...Fixing that alone would be sufficient.
However, to avoid the trap of "how to put the right const on a char pointer" there are generally two better options to spell this:
(1) With constexpr as an array:
constexpr char kItWorks[] = "";(2) With constexpr as a string_view:
constexpr std::string_view kItWorks{""};Using string_view helps simplify the test case code, also -- no need to convert from
char*tostd::stringto check for empty:--- a/multibody/parsing/test/detail_mujoco_parser_test.cc +++ b/multibody/parsing/test/detail_mujoco_parser_test.cc @@ -144,10 +144,9 @@ const char* dm_control_models[] = { INSTANTIATE_TEST_SUITE_P(DeepMindControl, DeepMindControlTest, testing::ValuesIn(dm_control_models)); -class MujocoMenagerieTest - : public MujocoParserTest, - public testing::WithParamInterface<std::pair<const char*, const char*>> { -}; +class MujocoMenagerieTest : public MujocoParserTest, + public testing::WithParamInterface< + std::pair<const char*, std::string_view>> {}; TEST_P(MujocoMenagerieTest, MujocoMenagerie) { // Confirm successful parsing of the MuJoCo models in the DeepMind control @@ -163,7 +162,7 @@ TEST_P(MujocoMenagerieTest, MujocoMenagerie) { // For this test, ignore all warnings. warning_records_.clear(); - if (!std::string(error_regex).empty()) { + if (!error_regex.empty()) { EXPECT_THAT(TakeError(), MatchesRegex(error_regex)); // For now, we'll just capture the *first* error. error_records_.clear(); @@ -181,7 +180,7 @@ const char* kMoreThanOneOrientation = ".*more than one orientation.*"; const char* kCapsuleSize = ".*size attribute for capsule geom.*"; } // namespace KnownErrors -const std::pair<const char*, const char*> mujoco_menagerie_models[] = { +const std::pair<const char*, std::string_view> mujoco_menagerie_models[] = { {"agility_cassie/cassie", KnownErrors::kNonUniformScale}, {"agility_cassie/scene", KnownErrors::kNonUniformScale}, {"aloha/aloha", kItWorks},
Done.
multibody/parsing/test/detail_mujoco_parser_test.cc line 233 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
We generally frown on committing commented-out code. Instead of commenting code, better would be to have a flag like
kDontEvenTryor whatever phrasing you prefer, and then have the test code recognize that and skip it.Or alternatively, change it to be purely a comment without code, like:
// We hope to parse these, but currently they throw in Debug ... etc. // - hello_robot_stretch/scene // - hello_robot_stretch/stretch
Done. Added kSkipMe.
tools/workspace/mujoco_menagerie_internal/repository.bzl line 9 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
If the January upgrade cadence doesn't upgrade this before this PR lands, then we need to document the "upgrade to latest commit" in our commit message, for the release notes.
Done.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform)
multibody/parsing/detail_mujoco_parser.cc line 154 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
I liked your logic and implemented it... but it was not sufficient. Specifically
R.col(0) = xyaxes.head<3>().normalized(); R.col(1) = xyaxes.tail<3>().normalized(); R.col(2) = R.col(0).cross(R.col(1)); return RigidTransformd(RotationMatrixd(R), pos);was not enough to avoid the error (for the cassie model).
My personal view is that the projection here is ambiguous.. it's recovering from the fact that the orientation is being specified with floating point strings to finite precision.
xyaxes="0.7922 -0.60599 -0.072096 0.60349 0.79547 -0.054922"I don't think one rounding scheme is obviously better than another. So I'm inclined to leave my ProjectToRotationMatrix fix and call it good.
to be clear; the reason it's insufficient is because x and y are not orthogonal to each other.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)
multibody/parsing/detail_mujoco_parser.cc line 154 at r2 (raw file):
the reason it's insufficient is because x and y are not orthogonal to each other.
Right! That's along of the lines of what the docs mention.
I wonder if this works...
(1) Normalize x to be a unit vector x_hat.
(2) Compute z axis as x cross y, normalize to be z_hat.
(3) Compute y_hat from x_hat and z_hat.
const Vector3d x = xyaxes.head<3>();
const Vector3d y = xyaxes.head<3>();
const Vector3d x_hat = x.normalized();
const Vector3d z_hat = (x.cross(y)).normalized();
const Vector3d y_hat = z_axis.cross(x_axis);
R.col(0) = x_hat;
R.col(1) = y_hat;
R.col(2) = z_hat;
(Double-check my right hand rule on that last line. Also I'm not sure if we should normalize x.cross(y) term itself, or normalize each operand individually first, for least floating point precision loss.)
For the curious, it also be fun to look at the upstream code to see exactly what it does, aside from the docs.
Big picture, I am not really worried about projection error due to the least significant ASCII digits in the file. I am worried that if someone writes two vectors that are purposefully far from orthogonal, we need to keep the x-axis exactly as they wrote it and it's not clear to me that ProjectTo... meets that requirement. Maybe people don't actually write files like that, though.
If this is best left to future work to tidy up, at least we should have a comment here citing their documentation and our uncertainty about whether we do it correctly or not.
multibody/parsing/BUILD.bazel line 673 at r4 (raw file):
"//geometry:test_stl_files", "@mujoco_menagerie_internal//:hello_robot_stretch", ] + _DM_CONTROL_MUJOCO_FILES,
nit Probably not super important, but it seems to me this DM_CONTROL_MUJOCO_FILES list is a bit more than we need here? I think we only need the quaduped, cartpole, acrobot now.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)
multibody/parsing/BUILD.bazel line 673 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Probably not super important, but it seems to me this DM_CONTROL_MUJOCO_FILES list is a bit more than we need here? I think we only need the quaduped, cartpole, acrobot now.
I actually did try that, but it's not just the three, it's also
<include file="./common/visual.xml"/>
<include file="./common/skybox.xml"/>
<include file="./common/materials.xml"/>
which is almost half the list at that point. Plus I have to change the code which imports it to use FindRunFiles instead of FindResourceOrThrow. So I figured this was best.
multibody/parsing/detail_mujoco_parser.cc line 154 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
the reason it's insufficient is because x and y are not orthogonal to each other.
Right! That's along of the lines of what the docs mention.
I wonder if this works...
(1) Normalize x to be a unit vector x_hat.
(2) Compute z axis as x cross y, normalize to be z_hat.
(3) Compute y_hat from x_hat and z_hat.const Vector3d x = xyaxes.head<3>(); const Vector3d y = xyaxes.head<3>(); const Vector3d x_hat = x.normalized(); const Vector3d z_hat = (x.cross(y)).normalized(); const Vector3d y_hat = z_axis.cross(x_axis); R.col(0) = x_hat; R.col(1) = y_hat; R.col(2) = z_hat;(Double-check my right hand rule on that last line. Also I'm not sure if we should normalize x.cross(y) term itself, or normalize each operand individually first, for least floating point precision loss.)
For the curious, it also be fun to look at the upstream code to see exactly what it does, aside from the docs.
Big picture, I am not really worried about projection error due to the least significant ASCII digits in the file. I am worried that if someone writes two vectors that are purposefully far from orthogonal, we need to keep the x-axis exactly as they wrote it and it's not clear to me that ProjectTo... meets that requirement. Maybe people don't actually write files like that, though.
If this is best left to future work to tidy up, at least we should have a comment here citing their documentation and our uncertainty about whether we do it correctly or not.
I've gone ahead and implemented that. That's better, thanks.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 3 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion
multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 16 at r5 (raw file):
using ::testing::MatchesRegex; class MujocoParserTest : public test::DiagnosticPolicyTestBase {
minor This name is copy-pasted from the other file and is in fact a different class. Maybe we need a better name?
Suggestion:
MujocoParserExamplesTest
Also upgrades mujoco_menagerie to the latest commit. Towards RobotLocomotion#20444 Brings the remaining models from the menagerie under test for the mujoco parser. All of them are permissively licensed (but this requires manual confirmation). The non-uniform mesh error is being tracked in RobotLocomotion#22046. My plan is to follow-up immediately with fixes for the other two known errors that are being documented in the expanded test.
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform)
multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 16 at r5 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
minor This name is copy-pasted from the other file and is in fact a different class. Maybe we need a better name?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status:
Towards #20444
Brings the remaining models from the menagerie under test for the mujoco parser. All of them are permissively licensed (but this requires manual confirmation).
The non-uniform mesh error is being tracked in #22046. My plan is to follow-up immediately with fixes for the other two known errors that are being documented in the expanded test.
+@jwnimmer-tri for feature review, please.
This change is