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

More Expression wrappers #1376

Merged
merged 4 commits into from
Jan 10, 2023
Merged

More Expression wrappers #1376

merged 4 commits into from
Jan 10, 2023

Conversation

BrettRD
Copy link
Contributor

@BrettRD BrettRD commented Jan 9, 2023

This pull request adds Expressions for:

  • Range<Point2, Point2>
  • Pose3::transformPoseTo
  • Pose3::translation
  • Unit3::point3
  • OrientedPlane3::distance
  • OrientedPlane3::normal
  • PinholeCamera::getPose

This pull request also adds Jacobians to the getter functions OrientedPlane3::distance and OrientedPlane3::normal

Let me know if any of these need tests, I couldn't spot any existing test patterns for these kinds of wrappers
I have some examples for these that I can share as well

@dellaert
Copy link
Member

dellaert commented Jan 9, 2023

Awesome! No need to test the expressions, but the new derivatives in Unit3 should have a test :-) Are you up for adding those?

@BrettRD
Copy link
Contributor Author

BrettRD commented Jan 9, 2023

I can give it a whirl,
Do you mean the new derivatives in OrientedPlane3.h?
Would the test end up testExpressionFactor.cpp?

@dellaert
Copy link
Member

dellaert commented Jan 9, 2023

Yep, those. They should be tested in testOrientedPlane.cpp, look at TEST(OrientedPlane3, jacobian_retract) for an example on using functions from numericalDerivative.h.

@BrettRD
Copy link
Contributor Author

BrettRD commented Jan 9, 2023

I was just thinking the explicit-value tests looked superfluous,
I'll try rebuilding those and actually refer to the tangent space

@BrettRD
Copy link
Contributor Author

BrettRD commented Jan 9, 2023

I think the new tests are what you're after.
I left in the test that compares to retract(zero) because it's fast and readable.

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Looks awesome ! Will merge when CI passes. Thanks !!!

@dellaert dellaert merged commit b882eaf into borglab:develop Jan 10, 2023
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