Conversation
|
Ready for review @dellaert :) |
|
I’m traveling so it might be a bit before I get to it. |
There was a problem hiding this comment.
Pull request overview
Implements an EqVIO (equivariant VIO) filter on top of EquivariantFilter, adds the EqF linearization blocks using an inverse-depth landmark chart, updates symmetry/action utilities accordingly, and introduces tests + an example replay driver.
Changes:
- Add
EqVIOFilter(dynamic landmark management, propagate/correct pipeline) and corresponding unit tests. - Extend EqVIO symmetry utilities with inverse-depth EqF matrices (
A/B/C) and innovation lifting for group updates. - Update base
EquivariantFilterto support derived filters that need to reset/sync internal buffers when dimensions change.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| gtsam_unstable/navigation/tests/testEqVIOSymmetry.cpp | Updates symmetry tests and adds helper routines used by the new symmetry behavior. |
| gtsam_unstable/navigation/tests/testEqVIOFilter.cpp | Adds unit tests for initialization, propagation parity, vision update, and dynamic landmarks. |
| gtsam_unstable/navigation/EqVIOSymmetry.h | Adds/updates public EqVIO symmetry APIs + EqF matrices + innovation lift; renames action to Symmetry. |
| gtsam_unstable/navigation/EqVIOSymmetry.cpp | Implements inverse-depth chart conversions, EqF matrices, and innovation lift; refactors action helpers. |
| gtsam_unstable/navigation/EqVIOState.cpp | Minor cleanup + adds gravityDir() comment. |
| gtsam_unstable/navigation/EqVIOFilter.h | Introduces EqVIOFilter API and parameter struct. |
| gtsam_unstable/navigation/EqVIOFilter.cpp | Implements filter logic: propagation, correction, outlier rejection, and dynamic landmark resize/sync. |
| gtsam_unstable/navigation/EqVIOCommon.h | Adds brief doc comments for group typedefs. |
| gtsam_unstable/examples/EqVIOFilterExample.cpp | Adds a replay example that consumes a CSV event stream and prints terminal stats. |
| gtsam/nonlinear/doc/PriorFactor.ipynb | Fixes markdown link formatting and cell string formatting. |
| gtsam/navigation/EquivariantFilter.h | Adds protected reset/sync hooks and a vector update overload with custom innovation lift; fixes identity sizing for dynamic groups. |
| gtsam/base/ProductLieGroup-inl.h | Initializes Jacobians to avoid uninitialized usage in dynamic settings. |
| Jacobian1 D_g_second; | ||
| Jacobian2 D_h_first; | ||
| Jacobian2 D_h_second; | ||
| Jacobian1 D_g_first = Jacobian1::Zero( |
There was a problem hiding this comment.
This is regrettable- does this only fail for certain compilers/versions?
There was a problem hiding this comment.
Yes, I noticed that it failed for the ubuntu-latest build, as a result of the template predictwithJacobian(...) in the EqVIOFilter. After backtracing a little bit the warning appears in compose/Logmap, which gets instantiated by the nested dynamic VioGroup during propagateState (VioGroup::Logmap(...)).
From my current understanding it is a false warning and zero-initializing fixes it. GCC can’t see that the Jacobians get assigned before they’re copied in this template path.
| */ | ||
|
|
||
| /// Reset reference, covariance, and group estimate; sync manifold state. | ||
| void resetReferenceAndGroup(const M& xi_ref, const CovarianceM& P, |
There was a problem hiding this comment.
If we can avoid, would be better. And would we ever create a new xi_ref and a non-identity X? If we re-reference, would X not be identity?
There was a problem hiding this comment.
Some updates (landmark add/remove / dimension changes) change xi_ref for bookkeeping/structure, not to the full current estimate. In that case, X stays non-identity to preserve the same physical estimate. I think if we forced identity the estimate would jump.
|
I removed the Specifics:
Let me know what you think about these changes @dellaert. |
dellaert
left a comment
There was a problem hiding this comment.
It seems to me that there’s still a lot done in the derived class that should be done in the base class. One of the goals here is to demonstrate how the templates can be used with a minimum of additional code.
It’s good that the shadow state is gone!
| static Matrix defaultCovariance(size_t nLandmarks); | ||
|
|
||
| /// Propagate covariance through linearized error dynamics for one effective IMU segment. | ||
| void propagateCovariance(const IMUInput& imu, double dt); |
There was a problem hiding this comment.
Why can EqF not do this for us?
There was a problem hiding this comment.
The paper was vague about how covariance/state was propagated, so I defaulted to van Goor's code implementation, which propagated covariance once and then propagated the state piecewise over each IMU hold.
It is true that we can have the EqF do this for us. I changed the flow so that for each IMU hold we now call predict(lift, psi, etc.). This gets rid of a good chunk of code (which is nice), but also means the linearization is updated for each hold, which minorly changes the filter output; looks fine on the example, but will do broader tests later.
| void addNewLandmarks(const VisionMeasurement& measurement, | ||
| const std::shared_ptr<const CameraModel>& camera); | ||
| /// Remove landmark by contiguous index in `cameraLandmarks`. | ||
| void removeLandmarkByIndex(int idx); |
There was a problem hiding this comment.
Yes, these are used by the update step of the filter, which along with the internal filter update, also does some landmark bookkeeping/management (removes stale landmarks, rejects outliers, inserts new landmarks as they become available)
Changelog:
ProductLiegroup-inl.hfor dynamic landmarks. CI was failing due to initialized Jacobians here, I patch this here.EqVIOFilter.h/cpp, extending fromEquivariantFilter.h. Made some changes to the base class to support syncing/re-binding filter internals once landmarks are added/removed, since this is an event where the model dimension changes.EqVIOFilterExample.cppthat runs the filter on the first 10 seconds of the EuRoC MAV Vicon Room 1 dataset. I ran feature tracking independently and condensed input information into a CSV that is less than 1mb in size. The first 300 lines or so of this file are helpers to read the CSV and build the buffers; the main loop is below it.Working example output below!