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

Add Lie group capabilities to NavState #1613

Draft
wants to merge 16 commits into
base: develop
Choose a base branch
from
Draft

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Aug 29, 2023

This PR makes NavState the $SE_2(3)$ Lie Group as defined in Barrau20icra.

Frank: this PR needs to just finish the transpose group operations.

@varunagrawal varunagrawal requested a review from ProfFan August 29, 2023 17:35
@varunagrawal varunagrawal self-assigned this Aug 29, 2023
@ProfFan
Copy link
Collaborator

ProfFan commented Aug 29, 2023

As a quick note, unit tests from #1582 should be added and pass

@dellaert
Copy link
Member

dellaert commented Sep 1, 2023

CI also fails for Cayley variants....

@varunagrawal varunagrawal linked an issue Sep 4, 2023 that may be closed by this pull request
@dellaert
Copy link
Member

CI fails. Can you comment on state of this PR?

@r4hul77
Copy link

r4hul77 commented May 6, 2024

Hi, is there any plan for this ? I want to add NavState Between Factor for dynamics constraint between 2 states. My state is NavState as I want to conveniently use ImuFactor2

@varunagrawal
Copy link
Collaborator Author

@dellaert @r4hul77 this paper and this paper have detailed descriptions of the various $SE_2(3)$ Lie algebra operations which I plan to implement. However, this will take a while since I am currently occupied with ICRA and PhD stuff.

@r4hul77
Copy link

r4hul77 commented May 9, 2024

Gotcha, thanks for this amazing library btw. I kinda found a work around by setting up a hard constraint between a pose3 and NavStates Pose3 to be equal and I'm doing the between factors between the pose3. Is it an acceptable work around ?

@stefangachter
Copy link
Contributor

Hi @varunagrawal, there is an older SE2(3) implementation for GTSAM available from Bonnabel and Barrau's team: https://github.com/mbrossar/gtsam see, https://github.com/mbrossar/gtsam/blob/develop/gtsam/geometry/ExtendedPose3.h and https://github.com/mbrossar/gtsam/blob/develop/gtsam/geometry/ExtendedPose3.cpp
This implementation was used to improve IMU pre-integration as described in the paper authored by Brossard: https://ieeexplore.ieee.org/document/9519152 Maybe you can crosscheck or leverage your implementation based on Brossard's implementation.

@stefangachter
Copy link
Contributor

Note that there is an error with coriolis term according to the paper cited above (https://ieeexplore.ieee.org/document/9519152):
image
As far as I can see, this is still not corrected:
https://github.com/mbrossar/gtsam/blob/a5e8d5f7f9ed77032f521b75c728176a42f2766c/gtsam/navigation/NavState.cpp#L222
Because it is part of NavState, it could be corrected within this PR.

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.

Given your PR to bring in Brossard’s changes, @stefangachter , it would be good to finish this PR, but @varunagrawal is very focused on other things. As Varun remarked, there are some things left:

  • the older retract/local I think should be left unchanged unless there is good reason to change them.
  • Unit tests in this PR are not complete. Especially exp/log derivatives need to be validated I think.

Re the other PR:

  • ExtendedPose3 should be removed In favor of the Lie NavState I think, no need to have two redundant classes.
  • Coriolis and earth rotation correction might have to be added in a separate PR
  • Focus (IMHO) should be on the “invariant” advantages
    I’ll also paste these comments there.

@@ -133,6 +388,30 @@ NavState NavState::retract(const Vector9& xi, //
//------------------------------------------------------------------------------
Vector9 NavState::localCoordinates(const NavState& g, //
OptionalJacobian<9, 9> H1, OptionalJacobian<9, 9> H2) const {
// return LieGroup<NavState, 9>::localCoordinates(g, H1, H2);
Copy link
Member

Choose a reason for hiding this comment

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

Why was retract unchanged but local coordinates changed? And, what’s the difference with example and logmap?

Copy link
Member

Choose a reason for hiding this comment

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

@dellaert
Copy link
Member

I merged two PRs, #1930 and #1932. #1930 was based on this PR but also introduced ExpmapTranslation. #1932 simplified the Expmap jacobians of Pose3 and hence also of NavState::Expmap.

I propose yet another PR takes care of retract and localCoordinates, and this one does the more advanced transpose operations...

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.

NavState between constraint
5 participants