Quaternion Accuracy

[Frank Dellaert]

LogMap in Quaternion mode depended on Eigen::AngleAxis, which yielded inaccurate results and bad numerical derivatives. @lucacarlone and @cbeall3 this might have been an issue in any factors using numerical derivatives in Q mode. I added a fix in pull request #22 but Chris should still check one last issue then merge (and then close this ticket).

[Chris Beall]

So I don't forget to fix before closing issue: New failure in Q mode (no matter what platform) after merging pull request:

 78/180 Test  #78: testPoseRotationPrior ..................***Failed    0.01 sec
not equal:
expected[0.1; 0.2; 0.3];
actual[-0.1; -0.2; -0.3];
/home/cbeall3/git/gtsam/gtsam/slam/tests/testPoseRotationPrior.cpp:67: Failure: "assert_equal((Vector(3) << 0.1,0.2,0.3), factor.evaluateError(pose1, actH1))" 
There were 1 failures

[Michael Bosse]

Its scary that the sign of Rot3::Logmap() or Rot3::rodriguez() should be platform dependent. What could cause this?

and imho, rodriguez should be spelled with an 's' (after Olinde Rodrigues)

[Chris Beall]

The issue I posted above actually is not platform dependent. The expected value should be negative.

We've only observed sign flips very close to singularities, where we end up just over/under pi. This depends on machine precision or rounding error, which is platform dependent. I think we're all set now!

[Frank Dellaert]

But why did it only now surface? Maybe the test was never reached? Anyway, if you're sure, do fix and commit.

[Chris Beall]

Verbose answer to try to clear up the confusion:

This particular line passed in the past in all modes (Q, M, and expmap), because the factor's error was computed as return Rotation::Logmap(newR) - Rotation::Logmap(measured_)

@ltoohey recently made a fix to address wrap-around in commit 7c8716f, changing the error to return measured_.localCoordinates(newR);, This is the correct way to do it, but exposed the computation to switching modes. (We expect exact answer in Q mode, and in M mode with full_expmap, and a slightly inaccurate answer in regular M mode.)

There's no sign switch in going from Rotation::Logmap(newR) - Rotation::Logmap(measured_) to measured_.localCoordinates(newR), but the sign of the expected value on this test changed in commit 3c42fbac on Oct. 8th. The test was also removed from M mode. So I didn't run into this issue in the develop branch until testing Q mode just yesterday.

We didn't run into this in feature/FixQuaternion, because that branch's parent was from Oct. 2nd, before @ltoohey's contribution.

[Frank Dellaert]

Ok, thanks for clarification and fix!