-
Notifications
You must be signed in to change notification settings - Fork 864
Adds a Lie-Group aware differentiable spline curve implementation #2212
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
base: develop
Are you sure you want to change the base?
Conversation
@BrettRD this is amazing! Here are my overall (Codex-assisted!) comments without yet trying to understand the code in detail. The first two comments are obvious/easy as they relate to style/comment currently requested for GTSAM PRs. The third one is way less obvious and might need a face-to-face meeting so I can help think through it with you: Code style
DocumentationPlease add Doxygen comments to new public classes, methods, and major typedefs in header files. Briefly describe the intent, parameters, and return values; this improves discoverability and helps future contributors. The Basis frameworkPrimer on the gtsam/basis frameworkBasis.h defines a CRTP base class Basis that supplies utility functors:
Integrating your spline work
That being said, we should have a chat about what that might look like here, and whether it fits your code. |
PS calibrations objects satisfy the manifold traits, but not (yet) Lie group. It does not make obvious sense to “compose” 2 calibration objects, and reading the CVPR paper the cumulative spline representation seems to demand composition and inverse. You could, in an application, “hack” this by deriving from a Calibration type, adding the composition / addition, and make that a Lie group / vector space, with corresponding traits. |
I'll have another read of the basis framework. On a first read, it looked like a lot of machinery to handle butterfly calculations in bounded domains and it wasn't clear if any of it could be adapted to single evaluations of FIR taps. For Cal3, I suspect every choice of manifold embedding is going to be equally arbitrary, so I'm tempted to make a simple CRTP adaptor to hack Vector-space traits onto the side of Manifold, based on a naive Reimannian embedding. |
Indeed, Basis is designed to create functors (in BasisFactors.h) at single "taps" in bounded domains. But, if you have a trajectory with
I like it. Maybe |
Something like that. I really ought to split this PR
I sent an email to your gatech address, is that the best way to reach you? |
…tesianProduct between multiplicative and additive groups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a Lie-Group aware differentiable spline curve implementation to enable clock recovery for continuous-time trajectories. The implementation follows CVPR 2020 research on efficient derivative computation for cumulative B-splines on Lie Groups.
Key changes:
- Adds TrajectoryModel template class for cardinal spline interpolation up to 7th order with Expression-based API
- Implements CartesianProduct and AsVectorSpace template utilities for combining manifolds
- Provides Irwin Hall polynomial kernels for spline basis functions
- Extends Expression templates with expmap function and scalar multiplication operators
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
File | Description |
---|---|
gtsam/slam/expressions.h | Adds expmap Expression template and scalar multiplication operators |
gtsam/geometry/CartesianProduct.h | Template for combining unrelated manifolds into single data structure |
gtsam/geometry/AsVectorSpace.h | Adapter to add VectorSpace traits to Manifold-only classes |
gtsam/basis/polynomial/TrajectoryModel.h | Main spline interpolation Expression factory for continuous trajectories |
gtsam/basis/polynomial/PiecewisePolynomial.h | Base class for piecewise polynomial kernel functions |
gtsam/basis/polynomial/IrwinHall.h | Header declaring Irwin Hall polynomial coefficients |
gtsam/basis/polynomial/IrwinHall.cpp | Irwin Hall PDF coefficient implementations |
gtsam/basis/polynomial/IrwinHallCDF.cpp | Irwin Hall CDF coefficient implementations |
gtsam/basis/polynomial/kernels.h | Abstract base class for convolutional kernel functions |
examples/ClockRecoveryContinuousTrajectory.cpp | Example demonstrating clock recovery with asynchronous sensors |
Test files | Comprehensive unit tests for all new functionality |
Comments suppressed due to low confidence (2)
gtsam/basis/polynomial/TrajectoryModel.h:134
- Accessing 'kernel' as a public member when it's declared as 'const KernelBase& kernel_;' - should use 'trajectory_model.kernel_' instead.
*
gtsam/basis/polynomial/TrajectoryModel.h:143
- Accessing 'kernel' as a public member when it should be 'accel_clock_model.kernel_' based on the class definition.
Double_ timestamp,
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -0,0 +1,148 @@ | |||
/** | |||
* @file testCartesinProduct.cpp |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filename in comment has a typo: 'testCartesinProduct.cpp' should be 'testCartesianProduct.cpp'
* @file testCartesinProduct.cpp | |
* @file testCartesianProduct.cpp |
Copilot uses AI. Check for mistakes.
@@ -0,0 +1,157 @@ | |||
/** | |||
* @file testCartesinProduct.cpp |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong filename in comment: should be 'testAsVectorSpace.cpp' not 'testCartesinProduct.cpp'
* @file testCartesinProduct.cpp | |
* @file testAsVectorSpace.cpp |
Copilot uses AI. Check for mistakes.
{ | ||
Key accel_clock_drift_key = Key('a', i); | ||
Key gyro_clock_drift_key = Key('g', i); | ||
initial_values.insert<double>(accel_drift_key, 0.0); |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable 'accel_drift_key' is undefined. Should be 'accel_clock_drift_key' based on the declaration on line 144.
initial_values.insert<double>(accel_drift_key, 0.0); | |
initial_values.insert<double>(accel_clock_drift_key, 0.0); |
Copilot uses AI. Check for mistakes.
|
||
|
||
// sample each sensor | ||
shutter_event_t shutter_event = sample_camera_add_slam_factors(); |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function 'sample_camera_add_slam_factors()' is undefined and not declared anywhere in the file.
Copilot uses AI. Check for mistakes.
|
||
// sample each sensor | ||
shutter_event_t shutter_event = sample_camera_add_slam_factors(); | ||
acceleration_t acceleration = sample_accelerometer(); |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function 'sample_accelerometer()' is undefined and not declared anywhere in the file.
Copilot uses AI. Check for mistakes.
// sample each sensor | ||
shutter_event_t shutter_event = sample_camera_add_slam_factors(); | ||
acceleration_t acceleration = sample_accelerometer(); | ||
angular_rate_t angular_rate = sample_gyro(); |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function 'sample_gyro()' is undefined and not declared anywhere in the file.
Copilot uses AI. Check for mistakes.
for(int i=0; i<trajectory_model.getControlPoints().size(); i++){ | ||
auto smoothness_factor = ExpressionFactor<Vector6>( | ||
/* any robust noise model */ | ||
Vector6()::Zero(), |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid syntax: 'Vector6()::Zero()' should be 'Vector6::Zero()' - cannot call static member function on temporary object.
Vector6()::Zero(), | |
Vector6::Zero(), |
Copilot uses AI. Check for mistakes.
if (H) *H << I_3x3, Z_3x3; | ||
return pose_derivative.seq(0,2); | ||
} | ||
Point3Deriv_ pose3DerivativeTranslation(Pose3Deriv_ pose_derivative, OptionalJacobian<3, 6> H = {}){ |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function parameter should be passed by const reference for efficiency: 'const Pose3Deriv_& pose_derivative'
Point3Deriv_ pose3DerivativeTranslation(Pose3Deriv_ pose_derivative, OptionalJacobian<3, 6> H = {}){ | |
Rot3Deriv_ pose3DerivativeRotation(const Pose3Deriv_& pose_derivative, OptionalJacobian<3, 6> H = {}){ | |
if (H) *H << I_3x3, Z_3x3; | |
return pose_derivative.seq(0,2); | |
} | |
Point3Deriv_ pose3DerivativeTranslation(const Pose3Deriv_& pose_derivative, OptionalJacobian<3, 6> H = {}){ |
Copilot uses AI. Check for mistakes.
if (H) *H << I_3x3, Z_3x3; | ||
return pose_derivative.seq(0,2); | ||
} | ||
Point3Deriv_ pose3DerivativeTranslation(Pose3Deriv_ pose_derivative, OptionalJacobian<3, 6> H = {}){ |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function parameter should be passed by const reference for efficiency: 'const Pose3Deriv_& pose_derivative'
Point3Deriv_ pose3DerivativeTranslation(Pose3Deriv_ pose_derivative, OptionalJacobian<3, 6> H = {}){ | |
Point3Deriv_ pose3DerivativeTranslation(const Pose3Deriv_& pose_derivative, OptionalJacobian<3, 6> H = {}){ |
Copilot uses AI. Check for mistakes.
) | ||
{ | ||
if(Hk) *Hk << x; | ||
if(Hx) *Hx << k * MakeJacobian<T,T>::type::Identity(); |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid syntax: 'MakeJacobian<T,T>::type::Identity()' should likely be 'typename MakeJacobian<T,T>::type::Identity()' or use a different approach to get the identity matrix.
if(Hx) *Hx << k * MakeJacobian<T,T>::type::Identity(); | |
if(Hx) *Hx << k * typename MakeJacobian<T,T>::type::Identity(); |
Copilot uses AI. Check for mistakes.
I liked many of Copilot's suggestions. Two more comments (and I might have more):
|
The majority of that copilot output seems to be about the example (which is only illustrative at this point and certainly won't compile) My last email might also be in your spam folder |
FYI: saw this paper advertised on Linkedin https://arxiv.org/abs/2504.11580 |
Wonderful! Their cubic basis function in (3) exactly matches the IrwinHall3 struct with a time-shift for each sample. Their approach to differentiate the polynomial that populates the time vector (7) is the same as in PiecewisePolynomial.h, giving it the connection to pseudo-spectral methods (pay quantisation errors up front, in exchange for exact derivatives thereafter) I'm surprised to see them running 100 control points per second, that seems a bit much, though I haven't seen a (correct) analysis of how the bandwidth of the curve scales with control-point frequency and spline order. The piecewise approximation of a Gaussian is a fairly aggressive low-pass filter, and insufficient control point density can make the SO3 component wrap when trying to describe high angular accelerations. I haven't forgotten about the changes we discussed, I haven't had a suitable block of time to get familiar with Eigen sparse matrices and plan out how to concatenate block-wise parameters into a contiguous model |
This PR follows a discussion on the mailing list about clock recovery for continuous-time trajectories.
I got the ok from my team to push the abstract features upstream, and I've re-written the implementation to be more versatile and line up better with existing literature.
I'm not ready to merge just yet (the header file preambles are conspicuously missing and the comments are still a bit of a mess), but I'd like some feedback for where the files should live, name-spacing, etc.
I'd also like some guidance on how best to expose Expression based features to the python bindings.
Basic usage:
Features:
O(spline_order+1)
O(spline_order + timestamp_search_window)
CartesianProduct
s of existing manifolds (like a std::pair but inheriting from LieGroup).AsVectorSpace
for extrapolatingVectorSpace
features fromManifold
traitsMiscellany:
expmap
(probably needs a quick test)unused features that can be be removed from the PR:
Double_
typesImplementation
Spline implementation follows a CVPR paper where the weights are pre-computed, and the weighted sum happens in the Lie Algebra using the first point (of the points in the window) as the datum.
This results in an execution that can act on a minimal number of control points, considers each point once, and has a trivial derivative propagation rule.
The weights for (uniformly sampled) splines are captured by the Irwin Hall Cumulative Probability Density Function.
The (uniformly sampled) weighted sum happens to be structurally equivalent to a Finite Impulse Response (FIR) Filter.
Questions:
traits<Rot3>::TangentVector
?traits<T>::TangentVector
is a sensible type for first derivatives ofT
, but is it an adequate type for second and Nth derivatives?TrajectoryModel
, but it's capable of defining surface-patches. what's a better name for it?Missing Tests:
Analytic derivatives of Rot3 spline curves match numerical derivatives to Nth orderMissing Examples:
Missing Features:
SeeCal3
doesn't have an expmap, so powered zoom lenses can't be built into a coordinated motion model usingTrajectoryModel<CartesianProduct<Pose3, Cal3> >
A Riemannian embedding using thelocalCoordinates
call would be (barely) enough like a logmap for this spline to function.AsVectorSpace