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

Better SE(3) and SE2(3) Jacobians #1938

Merged
merged 8 commits into from
Dec 17, 2024
Merged

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Dec 16, 2024

Implements ExpmapTranslation with SO3::applyLeftJacobian and simplifies the Expmap jacobians in a way that also provides insight in how they relate to the applyLeftJacobian Jacobians. The key piece of code is this:

  Matrix3 H;
  Vector t = local.applyLeftJacobian(v, H); // = J_l * v !

  // We return Jacobians for use in Expmap, so we multiply with X, that
  // translates from left to right for our right expmap convention:
  Matrix3 X = local.rightJacobian() * local.leftJacobianInverse();
  Q = X * H;

This does three things:

  • replaces very complicated expression $-0.5 * V + C * (W * V + V * W - WVW) + F * (WW * V + V * WW - 3 * WVW) - 0.5 * E * (WVW * W + W * WVW)$ from before
  • explains why the bottom-right block of the Expmap Jacobian was $J_r$, not $J_l$
  • functor in Pose3 is now no longer needed.

Smaller things:

  • added applyLeftJacobianInverse and tested
  • ExpmapTranslation no longer needs R
  • Changed sign of D and E "constants" and renamed them to E and F.
  • New D "constant" for use in inverses

@dellaert dellaert requested a review from ProfFan December 16, 2024 19:16
gtsam/geometry/SO3.h Outdated Show resolved Hide resolved
@dellaert dellaert mentioned this pull request Dec 16, 2024
8 tasks
gtsam/geometry/tests/testSO3.cpp Outdated Show resolved Hide resolved
gtsam/geometry/tests/testPose3.cpp Show resolved Hide resolved
gtsam/navigation/NavState.cpp Outdated Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator

Tentative approval but that unit test being killed is bothering me.

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

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

I think it's better that we stop and think about the nearZero throughly. As I mentioned in #1233, we have to be very careful in these approximations.

for example, the expression for D here has a 1000x error when theta is small.

gtsam/geometry/Pose3.cpp Outdated Show resolved Hide resolved
gtsam/geometry/Pose3.cpp Show resolved Hide resolved
gtsam/geometry/tests/testPose3.cpp Show resolved Hide resolved
@ProfFan
Copy link
Collaborator

ProfFan commented Dec 16, 2024

Lie Derivatives.pdf

output-01
output-02
output-03
output-04
output-05

@ProfFan
Copy link
Collaborator

ProfFan commented Dec 16, 2024

Particularly D and E are problematic.

image

@dellaert
Copy link
Member Author

@ProfFan I agree we have to look into it - and that issue is amazing - but I'd rather have a separate PR to deal with the nonZero thresholds. I will add the more thorough tests in this PR, but would like to merge after that.

I will add a citation to Ethan Eade's document as I am using his A,B,C constants now.

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

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

Cool cool

@dellaert
Copy link
Member Author

PS, why did you say "The cutoff should be 1 × 10−3 to use the Taylor expansion" for A? And, I don't understand what you are plotting, actually...

@dellaert dellaert merged commit 8c903c2 into develop Dec 17, 2024
33 checks passed
@dellaert dellaert deleted the feature/betterSE3Jacobian branch December 17, 2024 13:50
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.

3 participants