Skip to content
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

Merged
merged 3 commits into from
Jul 7, 2022

Conversation

shteren1
Copy link

@shteren1 shteren1 commented Jul 4, 2022

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.

@varunagrawal
Copy link
Collaborator

Thanks for the PR @shteren1. Is there any reason we can't wrap interpolate directly? The python wrapper supports inheritance.

@shteren1
Copy link
Author

shteren1 commented Jul 4, 2022

@varunagrawal Ohh I was not aware of that, I didn't see any wrappings of other Lie group classes.
Can you please do this one example? I'm not sure how to wrap it.

@varunagrawal
Copy link
Collaborator

I also recommend adding unit tests in python to verify the wrapping works as expected.

@shteren1
Copy link
Author

shteren1 commented Jul 4, 2022

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?

@varunagrawal
Copy link
Collaborator

Both! And yes numerical derivatives should suffice as a high level check.

* @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,
Copy link
Collaborator

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?

Copy link
Author

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
@shteren1
Copy link
Author

shteren1 commented Jul 7, 2022

@varunagrawal added tests for jacobians on some pose3 methods for python.
and changes interp name to slerp

@varunagrawal
Copy link
Collaborator

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. :)

@varunagrawal
Copy link
Collaborator

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.

@shteren1
Copy link
Author

shteren1 commented Jul 7, 2022

@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.

@varunagrawal
Copy link
Collaborator

Yes I am expecting numerical derivatives on the tangent space directly in the case of numpy.
I'm curious whether Jax supports Lie Groups. I know Tim Barfoot's group has an excellent python based Lie Groups library with Jacobian support.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome sauce!

@varunagrawal varunagrawal merged commit b91c43e into borglab:develop Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants