Skip to content

Adding Galilean3 Lie group#2122

Merged
dellaert merged 15 commits intoborglab:developfrom
mkielo3:gal
May 16, 2025
Merged

Adding Galilean3 Lie group#2122
dellaert merged 15 commits intoborglab:developfrom
mkielo3:gal

Conversation

@mkielo3
Copy link
Copy Markdown
Contributor

@mkielo3 mkielo3 commented May 1, 2025

Code follows style of NavState and implements core functionality: identity(), Hat(), Vee(), Expmap(), Logmap(), inverse(), between(), matrix(), compose(), adjointMap().

Tests are based off:

  1. Self consistency
  2. Comparison to numerical expectations
  3. Reconcilation to Gal3 in Manif by Deray & Sola where possible.

Note: derivatives of ExpMap and LogMap currently rely on numerical differentiation, which makes these tests circular.

Copy link
Copy Markdown
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.

Wow, amazing! Had time to review .cpp file, not yet other things.

@dellaert
Copy link
Copy Markdown
Member

dellaert commented May 6, 2025

Still some stragglers, e.g.,

/home/runner/work/gtsam/gtsam/gtsam/geometry/tests/testEvent.cpp:22:10: fatal error: gtsam_unstable/geometry/Event.h: No such file or directory
   22 | #include <gtsam_unstable/geometry/Event.h>

/// The type of the Lie algebra (matrix representation)
using LieAlgebra = Matrix5;

// Helper functions for accessing tangent vector components
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I might put these in an anonymous namespace in the .cpp file…

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unless we need them here, can we move them to anonymous namespace in the .cpp file?

@dellaert
Copy link
Copy Markdown
Member

dellaert commented May 7, 2025

@mkielo3 maybe run "make check" before you push: it will suss out remaining compilation problems.

Copy link
Copy Markdown
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.

Now CI fails because of wrapper :-) I commented on what the issue is I think.

* TOA functor below provides a measurement function for those applications.
*/
class GTSAM_UNSTABLE_EXPORT Event {
class GTSAM_EXPORT Event {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TimeOfArrival might also need an EXPORT

Event(double t, double x, double y, double z);
double time() const;
gtsam::Point3 location() const;
TimeOfArrival();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You need a new class here, move missing pieces from unstable

Copy link
Copy Markdown
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.

Awesome, just a few nits left and this can be merged :-)

/// The type of the Lie algebra (matrix representation)
using LieAlgebra = Matrix5;

// Helper functions for accessing tangent vector components
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unless we need them here, can we move them to anonymous namespace in the .cpp file?

#else
#warning "IncrementalFixedLagSmoother was moved to the gtsam/nonlinear directory"
#endif
// #ifdef _MSC_VER
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why comment these out?



#include <gtsam/nonlinear/IncrementalFixedLagSmoother.h> No newline at end of file
#include <gtsam/nonlinear/IncrementalFixedLagSmoother.h>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add newline in again

NonlinearFactorGraph, Point3, Values, noiseModel)
from gtsam_unstable import Event, TimeOfArrival, TOAFactor
NonlinearFactorGraph, Point3, Values, noiseModel, Event)
from gtsam_unstable import TimeOfArrival, TOAFactor
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TimeOfArrival should be in GTSAM now

Copy link
Copy Markdown
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.

Awesome!

@dellaert dellaert merged commit d6b4383 into borglab:develop May 16, 2025
36 checks passed
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