MatrixLieGroup implementation and traits#2187
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how matrix Lie groups are implemented by introducing a generic MatrixLieGroup CRTP helper and migrating existing geometry and dynamics classes to use it, removing duplicate vectorization and adjoint implementations, and updating dependent factors and tests.
- Introduce
MatrixLieGroupconcept ingtsam/base/MatrixLieGroup.hwith default implementations ofvec()andAdjointMap(). - Migrate geometry classes (
SO3,Pose2,Pose3,Gal3,NavState, etc.) to inherit fromMatrixLieGroupinstead ofLieGroup, removing their specializedvec()andAdjointMap()code. - Update
FrobeniusFactorandFrobeniusBetweenFactorto assertIsMatrixLieGroup<T>and usetraits<T>::Vec/traits<T>::AdjointMapfor vectorization and adjoint computations.
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| gtsam/base/MatrixLieGroup.h | Added CRTP helper MatrixLieGroup with generic vec() and AdjointMap() |
| gtsam/slam/FrobeniusFactor.h | Added IsMatrixLieGroup asserts and switched to traits<T>::Vec/AdjointMap |
| gtsam/geometry/SO3.h | Changed SO3 to inherit from MatrixLieGroup<SO3,3,3> and removed old code |
| gtsam/geometry/Pose3.h | Switched Pose3 to MatrixLieGroup<Pose3,6,4> and removed vec() override |
| gtsam/navigation/NavState.h | Switched NavState to MatrixLieGroup<NavState,9,5> and updated traits |
Comments suppressed due to low confidence (3)
gtsam/geometry/tests/testGal3.cpp:1202
- [nitpick] The test name uses lowercase 'vec' whereas other vec tests (e.g., TEST(Pose3, Vec)) use uppercase 'Vec' for consistency. Consider renaming to TEST(Gal3, Vec).
TEST(Gal3, AdjointMap) {
gtsam/geometry/SOn.h:54
- The generic implementation for SOn via MatrixLieGroup is introduced here, but there are no corresponding unit tests for SOn< N > (especially for N>4 or dynamic N). Consider adding tests for
vec()andAdjointMap()to validate the generic code.
class SO : public MatrixLieGroup<SO<N>, internal::DimensionSO(N), N> {
gtsam/geometry/SOn.cpp:102
- [nitpick] There are large blocks of commented-out legacy
vec()implementations across geometry sources (e.g., SOn.cpp). SinceMatrixLieGroupnow provides the generic implementation, these commented sections can be removed to improve code clarity.
return result;
Member
Author
|
@mkielo3 adding you to look at Gal3 changes. After this PR merges you should probably rebase your changes on this. |
f5cdd48 to
daa86fe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UPDATE: took timing and new vecs out of this PR for easier review, avoid segafult in #2191 .
This PR introduces a significant refactoring of how matrix Lie groups are handled in GTSAM, aiming to reduce code duplication and improve consistency:
MatrixLieGroupConcept: A new CRTP helper class MatrixLieGroup has been introduced in gtsam/base/MatrixLieGroup.h. This class provides generic implementations for common matrix Lie group operations like vec() (vectorization of the matrix representation) and AdjointMap().Lie.h: Existing MatrixLieGroup related definitions and functions have been moved from gtsam/base/Lie.h to the new gtsam/base/MatrixLieGroup.h. The AdjointMap assignment in IsLieGroup concept check in Lie.h was also corrected.traitsSpecializations: The traits specializations for the migrated classes have been updated to use internal::MatrixLieGroup.FrobeniusFactorUpdates: FrobeniusFactor and FrobeniusBetweenFactor now utilize the generic traits::Vec and traits::AdjointMap methods.