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

SE2(3) representation and Lie group pre-integration of Brossard et al. #1916

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

Conversation

stefangachter
Copy link
Contributor

@stefangachter stefangachter commented Dec 6, 2024

This pull request is to provide a "cleaned-up" version of Brossard's fork: https://github.com/mbrossar/gtsam
The PR adds two cmake options: GTSAM_LIEGROUP_PREINTEGRATION and GTSAM_EXTENDED_POSE_RETRACT
GTSAM_LIEGROUP_PREINTEGRATION enables the Lie group pre-integration as described in https://ieeexplore.ieee.org/document/9519152 (Note that this implementation differs only in the bias update from on-manifold pre-integration, i.e., integration and uncertainty propagation are the same.)
GTSAM_EXTENDED_POSE_RETRACT enables the extended-pose (SE2(3)) retraction in the Lie group pre-integration. This is disabled by default in the source of Brossard. Note that enabling pose retraction might "break" your optimization. It might need a consistent implementation of the extended pose for all the factors.
The handling of Earth rotation is enabled by default. See the conversation here #1613 as well.
(Note, the original branch is based on release 4.2. This branch is cherry-picked and not yet "tested".)

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.

Great! As I said in the NavState PR, I think:

  • I’d opt for landing that PR, maybe with the extendedPose3 tests added, so we just have the one class that does both manifold and Lie: NavState
  • Coriolis and earth rotation correction might have to be added in a separate PR, with tests, to make this PR cleaner/smaller
  • Focus (IMHO) should be on the “invariant” advantages, I.e., LieGroup integration.

@@ -18,6 +18,7 @@

#pragma once

#include <gtsam/geometry/ExtendedPose3.h>
Copy link
Member

Choose a reason for hiding this comment

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

Don’t see why

@@ -103,6 +103,16 @@ bool NavState::equals(const NavState& other, double tol) const {
&& equal_with_abs_tol(v_, other.v_, tol);
}

//------------------------------------------------------------------------------
#ifdef GTSAM_EXTENDED_POSE_RETRACT
Copy link
Member

Choose a reason for hiding this comment

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

I think this is done for the ImuFactor only, hence I’d prefer a solution that chooses NavState::exmap in the IMUFactor itself, once the other PR lands, if LieGroup integration is selected.

D_dt_R, -I_3x3, Z_3x3, //
D_dv_R, Z_3x3, -I_3x3;
}
const ExtendedPose3 T0(R_, t_, v_);
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why there are so many changes here?

H2 ? &D_biasCorrected_bias : nullptr);
H2 ? &D_biasCorrected_bias : nullptr);

// Correct for Earth rate
Copy link
Member

Choose a reason for hiding this comment

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

Is this tested?

NavState state_j = state_i.retract(xi,
H1 ? &D_predict_state : nullptr,
H1 || H2 ? &D_predict_delta : nullptr);
NavState state_j = state_ii.retract(xi, D_predict_state, D_predict_delta);
Copy link
Member

Choose a reason for hiding this comment

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

We. An select here to use expmap based on integration scheme, once NavState has Lie

/**
* @file testLieGroupPreintegration.cpp
* @brief Unit test for the LieGroupPreintegration
* @author Luca Carlone
Copy link
Member

Choose a reason for hiding this comment

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

Brossard, Gachter?

@@ -53,7 +53,7 @@ TEST(NavState, Constructor) {
numericalDerivative32(create, kAttitude, kPosition, kVelocity), aH2));
EXPECT(
assert_equal(
numericalDerivative32(create, kAttitude, kPosition, kVelocity), aH2));
numericalDerivative33(create, kAttitude, kPosition, kVelocity), aH3));
Copy link
Member

Choose a reason for hiding this comment

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

Good catch

@@ -200,10 +200,10 @@ TEST(NavState, Coriolis2) {
}

TEST(NavState, Coriolis3) {
/** Consider a massless planet with an attached nav frame at
* n_omega = [0 0 1]', and a body at position n_t = [1 0 0]', travelling with
/** Consider a massless planet with an attached nav frame at
Copy link
Member

Choose a reason for hiding this comment

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

This test did not change?


namespace gtsam {

#ifdef GTSAM_TANGENT_PREINTEGRATION
typedef TangentPreintegration PreintegrationType;
#else
#ifdef GTSAM_LIEGROUP_PREINTEGRATION
Copy link
Member

Choose a reason for hiding this comment

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

We might want to consider deprecating using compile-time flags in favor of a run-time mechanism.

@varunagrawal
Copy link
Collaborator

@stefangachter would it help is #1613 was finished? Your pointers and comments on that PR are immensely helpful.

@dellaert
Copy link
Member

Ok, since I have not yet heard yet from @stefangachter , I’ll take a crack at just the most basic group operations, making sure @varunagrawal changes and Brossard’s changes agree. I’ll do the PR straight to develop and then we can merge develop into both the PRs.

@stefangachter
Copy link
Contributor Author

I'm sorry for not getting back to you sooner. I was out of the office and busy with other stuff. I wanted to gain an overview of the different implementations before making a "commitment." (And I am setting up gtsam for development purposes under Windows, which takes its time as well.)

@dellaert
Copy link
Member

I'm sorry for not getting back to you sooner. I was out of the office and busy with other stuff. I wanted to gain an overview of the different implementations before making a "commitment." (And I am setting up gtsam for development purposes under Windows, which takes its time as well.)

No worries! I have been reading a lot of papers, and I’m not yet fully sure about which way to go, as I noticed the NavState manifold uses R,p,v and SE2(3) seems to always use R,v,p order. It’s a conundrum how to maintain backwards compatibility with NavState yet also agree with papers. Possibilities:

  1. a separate class with Rvp. But it’s so much duplication!
  2. One class but clearly document the constructor being Rpv. A bit confusing, but inline with Pose3 being both manifold and group.
  3. Keep NavState as the SE2(3) “torsor” and have ExtendedPose3 inherit, with an Rvp constructor. Confusing as hell, and not inline with “combined manifold/group classes”.

TBH, if backwards compatibility was not an issue I’d probably opt to split all classes, e.g., Pose3/Transform3, etc., but that’s a can of worms.

@stefangachter
Copy link
Contributor Author

From a state estimation point of view, I would like to be able to choose the composition of the state "freely" and not depend on a particular "structure". Currently, two variables are needed to estimate R, p, and v: 'x' representing pose (R,p) and 'v' representing velocity (v). It would be desirable to have a navigation state as a single variable 'x' but that can be composed of <SE(3), R3> or SE2(3), for example. This is my high-level view. -- I recently looked at https://github.com/aau-cns/Lie-plusplus, which allows compositions of groups.( There are other similar templated libraries for "groups". But I do not have an "overview" of them.) Thus, the navigation state NavState could be defined as a composition of groups. -- The biggest impact of the "navigation state representation" is on IMU pre-integration. But as Brossard et al. https://ieeexplore.ieee.org/document/9519152 and Delama et al. https://ieeexplore.ieee.org/document/10777045 show, the pre-integration can be nicely formulated in a "group composition agnostic" way. It would take an effort to adapt the code accordingly but backward "compatibility" might be possible. -- From your suggestion above, Frank, I would go for 2). -- P.S. Probably I am a little bit sloppy in using the term "group" and do not make the distinction to "manifold".

@dellaert
Copy link
Member

From your suggestion above, Frank, I would go for 2). -- P.S. Probably I am a little bit sloppy in using the term "group" and do not make the distinction to "manifold".

All good points, I appreciate the input. I am actually more and more nudging to 3, though, as the most advanced methods now are based on the Equivariant filter, https://ieeexplore.ieee.org/document/9840886. There the distinction between manifold and group is paramount. It is crucial especially, when the manifold, e.g., Unit3, does not admit group operations - but SO(3) "acts" on the sphere.

@varunagrawal
Copy link
Collaborator

I'm sorry for not getting back to you sooner. I was out of the office and busy with other stuff. I wanted to gain an overview of the different implementations before making a "commitment." (And I am setting up gtsam for development purposes under Windows, which takes its time as well.)

No worries! I have been reading a lot of papers, and I’m not yet fully sure about which way to go, as I noticed the NavState manifold uses R,p,v and SE2(3) seems to always use R,v,p order. It’s a conundrum how to maintain backwards compatibility with NavState yet also agree with papers. Possibilities:

  1. a separate class with Rvp. But it’s so much duplication!
  2. One class but clearly document the constructor being Rpv. A bit confusing, but inline with Pose3 being both manifold and group.
  3. Keep NavState as the SE2(3) “torsor” and have ExtendedPose3 inherit, with an Rvp constructor. Confusing as hell, and not inline with “combined manifold/group classes”.

TBH, if backwards compatibility was not an issue I’d probably opt to split all classes, e.g., Pose3/Transform3, etc., but that’s a can of worms.

Maybe my $0.02 since my state estimation metric paper uses the NavState as a $SE_2(3)$ group:

  1. I personally think underneath the hood the representation we use doesn't quite matter. In my own paper, I used (R, p, v) and was able to show equivalent group operations. It's not ideal, but similar to how ConstantBias is (a, $\omega$) (which a lot of people have asked me about), it's something we have to live with. I would focus on clean documentation instead aka option 2.
  2. Thanks for all the nice resources @stefangachter! Both you and @dellaert seem to have found a vein of research with the equivariant estimators that I was not familiar with. I have been following work on Invariant filtering (and smoothing) from various groups, and my primary motivation was to have something like this for my own work. PS the first author, Ziwon, is now a PhD student at Georgia Tech.

@ProfFan
Copy link
Collaborator

ProfFan commented Dec 13, 2024

2. I have been following work on Invariant filtering (and smoothing) from various groups, and my primary motivation was to have something like this for my own work. PS the first author, Ziwon, is now a PhD student at Georgia Tech.

Thank you for the great reference! I got one question though - the invariant filtering math in this paper is the same as in Grizzle's paper (R. Hartley, M. Ghaffari, R. M. Eustice, and J. W. Grizzle, “Contact-aided invariant extended Kalman filtering for robot state estimation,” Int. J. Robot. Res., vol. 39, no. 4, pp. 402–430, 2020.), correct? The novelty of this paper is more on the slip detection method? Please correct me if I am wrong.

@stefangachter
Copy link
Contributor Author

@dellaert, the code I provide here is a copy of Brossard's. I did not review the code. I adapted it slightly so that I could integrate and test it with the latest develop branch. I have limited time available to do some further improvements and reviews. Should I resolve your comments above? Or, how would you like to go forward?

@dellaert
Copy link
Member

I’ll take a look at both PRs in detail - don’t worry for now.

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.

5 participants