drake icon indicating copy to clipboard operation
drake copied to clipboard

CompareMatrices takes RigidTransform and RotationMatrix directly

Open SeanCurtis-TRI opened this issue 3 years ago • 3 comments

This changes calls like:

EXPECT_TRUE(CompareMatrices(X_AB.GetAsMatrix34(),
                            X_AB_expected.GetAsMatrix34()));

to

EXPECT_TRUE(CompareMatrices(X_AB, X_AB_expected));

And the same for rotations.


This change is Reviewable

SeanCurtis-TRI avatar Jan 13 '23 17:01 SeanCurtis-TRI

@jwnimmer-tri Is there any reason not to do this?

As implemented currently, it adds spurious dependencies. Now anytime rotation_matrix.h changes, more unit tests will get recompiled than are strictly necessary. That's easily enough solved by moving this new content to a different filename (presuming under math/... somewhere). The question would be whether a new include path is worth the changes at call sites. Assuming we might add more transform-comparison sugar down the road (e.g., rotation tolerance vs translation tolerance), it might very well be worth it.

Bigger picture, though, it's also doubling down on the "AssertionResult" idiom, which we should disprefer in favor of the more flexible architecture of gtest "matchers". It's not necessarily a defect to incrementally improve the old way, but if I were doing this I wouldn't invest my time in the old way, rather I'd work on doing better in the new way.

jwnimmer-tri avatar Jan 13 '23 20:01 jwnimmer-tri

The matcher hypothesis goes like this:

By induction, we already have good a matcher infrastructure for multidimensional matrices, with configuration options and diagnostic output no worse than CompareMatrices. Since we need to compare matrix data beyond just RigidTransform and/or RotationMatrix, we will certainly need this anyway. Essentially the task to make this happen is to port CompareMatrices to the matcher idiom. It's akin to the upstream collection matchers already, except for a multi-dimentional collection, and with better support for tolerances. Possibly we should look into whether anticipating https://en.cppreference.com/w/cpp/container/mdspan would make sense.

Given that multidimensional matcher, for RigidTransform and RotationMatrix we just need to guide it to unwrap them into their matrix form when comparing. That's akin to the higher-order matchers already like http://google.github.io/googletest/reference/matchers.html#composite-matchers and http://google.github.io/googletest/reference/matchers.html#adapters-for-matchers so we have something like EXPECT_THAT(rotmat, AsMatrix(MdElementsAreArray({{1, 2, 3}, {4, 5, 6}, {7, 8, 9}, tol)));.

jwnimmer-tri avatar Jan 17 '23 19:01 jwnimmer-tri

The failure message would be degraded (arguably significantly so).

But why? The failure message for matrix comparison failures should be good. Both values should be printed in full, and the index that differs should receive a mention.

I think you are misunderstanding what I have in mind overall, but that's fine. There's no real need for me to explain it: adding ADL overloads for CompareMatrices in distinct header files is not defective.

jwnimmer-tri avatar Jan 17 '23 20:01 jwnimmer-tri