Skip to content

Conversation

darshan-17
Copy link
Contributor

@darshan-17 darshan-17 commented Apr 18, 2025

An implementation of the Equivariant Filter (EqF) for attitude estimation with both gyroscope bias and sensor extrinsic calibration, based on the paper: "Overcoming Bias: Equivariant Filter Design for Biased Attitude Estimation with Online Calibration" by Fornasier et al.

Uses the same dataset from the paper but implemented through built-in GTSAM functions to reproduce the results.

Authored by Darshan Rajasekaran & Jennifer Oum

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

OK, so, many comments :-)
One main organizational comment that I think will make the transition to a templated version easier:

  1. Put Group, State in ABC.h, no cpp file, everything inlined. Use template size_T and std:array as I suggested earlier
  2. In ABC_EQF.*, only put filter stuff. Can still be specific and include ABC.h, but try to extract as much ABC-specific things you can into ABC.h
  3. Everything else (data loading is biggest part) put in Demo file.

* @param m Number of sensors
* Uses SelfAdjointSolver, completeOrthoganalDecomposition().pseudoInverse()
*/
EqF::EqF(const Matrix& Sigma, int n, int m)
Copy link
Member

Choose a reason for hiding this comment

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

To me, the argument n causes a lot of issues! In the ABC.h code I shared with you I sketched how to use a size_t template argument, ijn which case the dimensions of G and State are known at compile time. I think that's much preferred for an example=, rather than introducing variable dimenions (and hence dynamically allocated matrices) everywhere.

@jenniferoum jenniferoum force-pushed the feature/equivariant-filter branch from 32bcc3a to e5f4978 Compare April 21, 2025 03:14
Copy link
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, great progresss!

I would still simplify much more. Get rid of “create” where possible, don’t check covariance matrices, and move all things that are ABC specific (except filter) to a separate file ABC.h.

And I would template on size_t, which will simplify the code even more. Remember this is an example, so we do not need to support reading data files with varying number of calibrations.

Matrix3 W() const { /// Return w as a skew symmetric matrix
return Rot3::Hat(w);
}
static Input create(const Vector3& w, const Matrix& Sigma) { /// Initialize w and Sigma
Copy link
Member

Choose a reason for hiding this comment

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

Still seems overkill, just use brace init, don’t check covariance

Unit3 d; /// Known direction in global frame
Matrix3 Sigma; /// Covariance matrix of the measurement
int cal_idx = -1; /// Calibration index (-1 for calibrated sensor)
static Measurement create(const Unit3& y_vec, const Unit3& d_vec, /// Initialize measurement
Copy link
Member

Choose a reason for hiding this comment

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

Same: kill and use brace init. Covariance checking is overkill in an example (and is not very relevant when we switch to noise models)


/// State class representing the state of the Biased Attitude System

class State {
Copy link
Member

Choose a reason for hiding this comment

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

Move to ABC.h and template on size_t

* Symmetry group (SO(3) |x so(3)) x SO(3) x ... x SO(3)
* Each element of the B list is associated with a calibration state
*/
class G {
Copy link
Member

Choose a reason for hiding this comment

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

Move to ABC.h and template on size_t

class EqF {
private:
int dof; // Degrees of freedom
int n_cal; // Number of calibration states
Copy link
Member

Choose a reason for hiding this comment

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

Template EqF on size_t as well.

…ordinate to exponential, separate filter and demo specific functions, rename stateAction to operator *, fix brace initialization

/// Check if a vector is a unit vector

bool checkNorm(const Vector3& x, double tol = 1e-3);
Copy link
Member

Choose a reason for hiding this comment

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

kill this


/// Check if vector contains NaN values

bool hasNaN(const Vector3& vec);
Copy link
Member

Choose a reason for hiding this comment

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

maybe kill this too if you don;t use it


/// Create a block diagonal matrix from two matrices

Matrix blockDiag(const Matrix& A, const Matrix& B);
Copy link
Member

Choose a reason for hiding this comment

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

kill declaration

* @return A single consolidated matrix with A in the top left and B in the
* bottom right
*/
Matrix blockDiag(const Matrix& A, const Matrix& B) {
Copy link
Member

Choose a reason for hiding this comment

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

decide whether this is ABC-specific, or Eqf specific. If latter, move it.

* @param n Number of times to be repeated
* @return Block diag matrix with A repeated 'n' times
*/
Matrix repBlock(const Matrix& A, int n) {
Copy link
Member

Choose a reason for hiding this comment

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

same

@dellaert
Copy link
Member

Next PR should have:

  • Abc.h
  • EquivariantFilter.h (does not include Abc.h or Sphere.h)
  • AbcEquivariantFilterExample.cpp (instead of demo)
  • Sphere.h
  • SphereEquivariantFilterExample.h (does not include Abc.h)

@dellaert dellaert merged commit deb4045 into borglab:develop Apr 26, 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.

3 participants