Cayley Rot3 fixes #597
Cayley Rot3 fixes #597
Conversation
| @@ -31,6 +31,13 @@ if(NOT MSVC AND NOT XCODE_VERSION) | |||
| option(GTSAM_BUILD_WITH_CCACHE "Use ccache compiler cache" ON) | |||
| endif() | |||
|
|
|||
| # Enable GTSAM_ROT3_EXPMAP if GTSAM_POSE3_EXPMAP is enabled, and vice versa. | |||
There was a problem hiding this comment.
This is a bit problematic as it is done "silently". Add a message?
There was a problem hiding this comment.
Ideally it should be one flag 🙁 but I need to consult you before making that design change.
gtsam/geometry/Rot3M.cpp
Outdated
| const double y = b * f - ce - c; | ||
| const double z = fg - di - d; | ||
| return K * Vector3(x, y, z); | ||
| const Matrix3 P = A + I_3x3; |
There was a problem hiding this comment.
Yes if accompanied by benchmark results on effect on timing.
There was a problem hiding this comment.
Old version is way faster ~650 ms vs 2248 ms for the fully Eigen based one. Reverting.
There was a problem hiding this comment.
I'll instead add more documentation.
|
Note, search through "timeXXX.cpp" files, might already be there. |
|
I added a CI path for the Cayley Transform in the last commit. |
|
@chrisbeall good news! The CI for the Cayley transform passes. |
|
Good, now timing sanity-check and we can land this :-) |
|
Timing results for Rot3: Before: After: There is negligible impact due to the determinant check. |
|
I was not worried about the determinant check. I was worried about replacing hand optimized code with much nicer but more high-level code :-) could you just point out the isolated results for Cayley, which is affected by that piece of code? And make sure the right flags are set so Cayley is in fact called ? (You did not mention flags in your comment) |
|
I undid the change from hand optimized to high-level, so it is back to hand optimized code now (albeit with an extra determinant check). The flags being set are in the CI and I already verified that. 🙂 |
|
Wait! I liked your nicer code, if it is the same speed. What was the timing on that? |
|
Oh, wait, I did not see your comment above. OK, I’ll approve this version, then :-) |
This PR is intended to fix #591.
POSE3_EXPMAPandROT3_EXPMAPdepend on each other.