-
Notifications
You must be signed in to change notification settings - Fork 764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Numerous unit test failures with GTSAM_ROT3_EXPMAP=OFF #591
Comments
I believe this is primarily a |
I found some related info! Issue #248 goes into a lot of detail about why the Cayley parameterization is bad (which is what is used when we set @ProfFan's post also explains why the tests fail. We're basically seeing poorer convergence due to the approximation. I believe this is the reason we don't have a CI path for @dellaert you mentioned removing the Cayley parameterization completely. Is this still on the cards? |
ROT3_EXPMAP=ON turns off Cayley and turns on full exponential map. We changed the defaults to have ROT3_EXPMAP=ON, AFAIK, based on Fan's work. Other than that, let's just fix the failing unit tests by branching on the flag? |
I did some investigating and the fix for this might be easier than expected. It seems like the majority of tests are failing due to setting I made a simple change to the cmake files and it has done the trick, only one I'll also add in a CI path for |
I think this was by design, to let users control the behavior of I failed to mention in my report that all tests (except for |
Yeah, seems strange that you would have |
@dellaert only the I looked into the issue (which took forever because of lack of documentation on the Local function for Cayley maps), and the problem is that it doesn't handle the case where I have already fixed that, though that should be a separate PR. I also recommend using the following snippet instead of the hand written current function implementation. This would allow Eigen to optimize the operations. Matrix3 A = R.matrix();
return SO3::Vee((A + I_3x3).inverse() * (A - I_3x3)) * 2; The current implementation is const double a = A(0, 0), b = A(0, 1), c = A(0, 2);
const double d = A(1, 0), e = A(1, 1), f = A(1, 2);
const double g = A(2, 0), h = A(2, 1), i = A(2, 2);
const double di = d * i, ce = c * e, cd = c * d, fg = f * g;
const double M = 1 + e - f * h + i + e * i;
const double K = -4.0 / (cd * h + M + a * M - g * (c + ce) - b * (d + di - fg));
const double x = a * f - cd + f;
const double y = b * f - ce - c;
const double z = fg - di - d;
return K * Vector3(x, y, z); which requires re-tracing all the steps in the matrix computations. |
Note that in the snippet above, |
Hmm this test case is weird because the prior is a 180 degree rotation around X and Z individually, which the Cayley map cannot accommodate. We should #ifdef that test for Expmap only. |
Description
Numerous unit test failures with
GTSAM_ROT3_EXPMAP=OFF
. This configuration apparently is not be exercised by CI.Steps to reproduce
GTSAM_ROT3_EXPMAP=OFF
, and alsoGTSAM_BUILD_WITH_MARCH_NATIVE=OFF
(separate issue caused bymarch=native
reported in testSmartProjectionFactor unit test fails with march=native #590)make check
Expected behavior
Tests pass, and CI should probably exercise this configuration.
Environment
Ubuntu 16.04, gcc 5.4.0
Mac OS, Xcode 12.1, Apple clang 12.0.0.12000032
Additional information
Detailed failures on Ubuntu:
The text was updated successfully, but these errors were encountered: