Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR modernizes the numerical-derivative utilities to accept generic callables (improving overload/lambda handling) and updates several geometry components/wrappers alongside a broad mechanical cleanup of test lambdas/binds.
Changes:
- Refactored
gtsam/base/numericalDerivative.hto use callable-based helpers with clearer template requirements and improved overload resolution. - Added/updated geometry functionality and documentation (e.g.,
Gal3::LogmapDerivativeanalytical implementation, SO3 functor cleanup, quaternion trait dimension helper). - Updated many geometry/navigation tests to use
autoinstead ofstd::function, and adjusted wrappers to expose additional APIs / constness.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| gtsam/base/numericalDerivative.h | Reworked numerical derivative API to accept generic callables and added stronger type/trait requirements. |
| gtsam/geometry/Gal3.h | Added tangent-space LogmapDerivative declaration. |
| gtsam/geometry/Gal3.cpp | Implemented analytical LogmapDerivative(xi) and routed LogmapDerivative(g) through it. |
| gtsam/geometry/Quaternion.h | Added traits<QUATERNION_TYPE>::GetDimension. |
| gtsam/geometry/SO3.h | Corrected B comment and inlined ExpmapFunctor::expmap(). |
| gtsam/geometry/SO3.cpp | Removed out-of-line ExpmapFunctor::expmap() definition after inlining. |
| gtsam/geometry/Pose3.cpp | Expanded Expmap implementation comments/documentation pointers. |
| gtsam/geometry/geometry.i | Made DexpFunctor::omega const and exposed Similarity2::matrix() / Similarity3::matrix() to wrappers. |
| gtsam/navigation/tests/testTangentPreintegration.cpp | Replaced std::function wrappers with auto lambdas/binds. |
| gtsam/navigation/tests/testNavStateImuEKF.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/navigation/tests/testNavState.cpp | Replaced std::function wrappers with auto, added [[maybe_unused]] in select cases. |
| gtsam/navigation/tests/testManifoldPreintegration.cpp | Replaced std::function wrappers with auto lambdas/binds. |
| gtsam/navigation/tests/testImuFactor.cpp | Replaced std::function wrapper with auto bind. |
| gtsam/navigation/tests/testImuBias.cpp | Replaced std::function wrappers with auto binds. |
| gtsam/navigation/tests/testGal3ImuEKF.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/navigation/tests/testAHRSFactor.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/geometry/tests/testUnit3.cpp | Replaced std::function wrappers with auto binds/lambdas. |
| gtsam/geometry/tests/testSimilarity3.cpp | Replaced std::function with auto and simplified derivative wrappers. |
| gtsam/geometry/tests/testSimilarity2.cpp | Replaced std::function with auto and simplified derivative wrappers. |
| gtsam/geometry/tests/testSOn.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/geometry/tests/testSO4.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/geometry/tests/testSO3.cpp | Replaced std::function wrappers with auto lambdas and minor formatting cleanup. |
| gtsam/geometry/tests/testRot2.cpp | Replaced std::function wrapper with auto lambda. |
| gtsam/geometry/tests/testPose3.cpp | Replaced std::function wrappers with auto lambdas and minor formatting change. |
| gtsam/geometry/tests/testPose2.cpp | Replaced std::function wrappers with auto lambdas and simplified proxy lambdas. |
| gtsam/geometry/tests/testPoint3.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/geometry/tests/testPinholePose.cpp | Replaced std::function wrapper with auto bind in commented-out test block. |
| gtsam/geometry/tests/testPinholeCamera.cpp | Replaced std::function wrappers with auto binds. |
| gtsam/geometry/tests/testOrientedPlane3.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/geometry/tests/testGal3.cpp | Replaced std::function wrappers with auto lambdas and simplified wrappers. |
| gtsam/geometry/tests/testExtendedPose3.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/geometry/tests/testEssentialMatrix.cpp | Replaced std::function wrappers with auto lambdas. |
| gtsam/geometry/tests/testCalibratedCamera.cpp | Replaced std::function wrapper with auto bind. |
timeSFMBAL benchmark
Worker runs
|
ProfFan
reviewed
Apr 14, 2026
|
|
||
| /// Rodrigues formula | ||
| Matrix3 expmap() const; | ||
| Matrix3 expmap() const { return I_3x3 + A * W + B * WW; } |
Collaborator
There was a problem hiding this comment.
non inline implementation should not be in the header
Member
Author
There was a problem hiding this comment.
Will it not be automatically inlined? That’s what I’ve been told. All CI passes.
Collaborator
There was a problem hiding this comment.
It will be a ODR violation
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.
High-level changes
Modernized
gtsam/base/numericalDerivative.h:Geometry/runtime updates:
Gal3: added analyticalLogmapDerivative(const TangentVector&)and routedLogmapDerivative(const Gal3&)through itQuaterniontraits: addedGetDimensionSO3: small cleanup (inlineExpmapFunctor::expmap, correctedBcomment)Pose3: expanded implementation comments aroundExpmapgeometry.i): exposedSimilarity2::matrix()/Similarity3::matrix()and alignedDexpFunctorfield constnessTest/style updates in geometry/navigation:
std::function<...> var = ...withauto var = ...in test code[[maybe_unused]]only where required to satisfy-WerrorValidation
make -j6 check.geometry✅make -j6 check.navigation✅