-
Notifications
You must be signed in to change notification settings - Fork 779
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
added jacobians for all pose3 methods in python wrappers #1234
Conversation
Thanks for the PR @shteren1. Is there any reason we can't wrap interpolate directly? The python wrapper supports inheritance. |
@varunagrawal Ohh I was not aware of that, I didn't see any wrappings of other Lie group classes. |
I also recommend adding unit tests in python to verify the wrapping works as expected. |
How do you propose to add tests for jacobians? compare them to numerical derivatives? or you meant just sanity tests that the methods can accept a matrix and fills it up without failing? |
Both! And yes numerical derivatives should suffice as a high level check. |
gtsam/geometry/Pose3.h
Outdated
* @param s a value between 0 and 1.5 | ||
* @param other final point of interpolation geodesic on manifold | ||
*/ | ||
Pose3 interp(double t, const Pose3& other, OptionalJacobian<6, 6> Hx = boost::none, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, I actually prefer this since Lie::interpolate
is a free function. Can you please rename this to slerp
similar to Rot3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, isn't "slerp" referring to interpolation of rotations on the quaternion unit sphere specifically?
2. added python tests for jacobians for some pose3 apis
@varunagrawal added tests for jacobians on some pose3 methods for python. |
Woah I was expecting the use of numpy's gradient methods but you really went above and beyond! Thanks @shteren1 I'll do a full review once I'm at a desktop. :) |
We should also consider adding scipy or autograd packages as dev dependencies for unit testing. I imagine this would be useful in the long term. |
@varunagrawal i'm not sure how they will work, the way gtsam calculates the jacobians of manifold actions on Pose3 class is not trivial, autograds packages would probably have operated on the euclidean space. I had to use the retract and localCoordinates actions to get the same results. and I think JAX is the go to package these days for auto differentiating on numpy and scipy objects, outside of pytorch. |
Yes I am expecting numerical derivatives on the tangent space directly in the case of numpy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome sauce!
This PR adds jacobian access for all of Pose3 methods in the python wrappers.
I also added Pose3::interp to access Lie group interpolate from the python wrappers.