Skip to content
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

Normalize rotations after composition #555

Merged
merged 3 commits into from
Oct 6, 2020
Merged

Normalize rotations after composition #555

merged 3 commits into from
Oct 6, 2020

Conversation

varunagrawal
Copy link
Collaborator

This PR fixes #372

I added normalize function to orthogonalize the rotation after composition for the matrix representation and I used the normalized method for the quaternion representation.

I added a test case based on #372 and while it initially failed, it now succeeds, validating this approach.

The idea is from here.

@varunagrawal varunagrawal added the bugfix Fixes an issue or bug label Oct 4, 2020
@varunagrawal varunagrawal self-assigned this Oct 4, 2020
@varunagrawal
Copy link
Collaborator Author

@Alexma3312 FYI 🙂

@ProfFan
Copy link
Collaborator

ProfFan commented Oct 5, 2020

Amazing find :) Two questions:

  • this works when the error is small? If so it should be documented
  • should this be enabled by default?
    • This operation is a bit expensive (1 det = 9 mul, then 3 block ops)
    • if ROT3_QUAT is enabled then one quat->mat and one mat->quat
    • not necessary? since quat is normalized by itself?

@varunagrawal
Copy link
Collaborator Author

  1. If the error is less than 1e-12 it just returns the current rotation.
  2. Since we check if the determinant is very close to 1, we don't always perform the normalization. The determinant isn't too expensive for 3x3 matrices and I am sure Eigen optimizes that at compile time. Since we only perform the normalization occasionally (when the drift is large), the block operations cost should be amortized.
  3. I did propose using the quat formulation but @dellaert found it inelegant. Also, I believe the matrix to quaternion conversion and vice-versa requires more FLOPS than the determinant and block operations combined.

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.

Very cool! But please search for normalize through the GTSAM code base as I think we might already have some of this in place. I can be mistaken.

And: we really cannot add this in the low-level operations. We can discuss in person.

/* ************************************************************************* */
Rot3 Rot3::operator*(const Rot3& R2) const {
return Rot3(quaternion_ * R2.quaternion_);
return normalize(Rot3(quaternion_ * R2.quaternion_));
Copy link
Member

Choose a reason for hiding this comment

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

That's unacceptable I'm afraid: people will have to do this explicitly themselves, it can't be part of the low-level geometry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@varunagrawal
Copy link
Collaborator Author

I did a search and the only normalize functions/methods I could find were in Point3 and Rot2. Nothing for Rot3.

There is a ClosestTo method in SO3.cpp which also does a normalization, but uses a full SVD which is far more expensive than what this PR is proposing.

@dellaert
Copy link
Member

dellaert commented Oct 5, 2020

OK, please add comments in both places to compare and contrast them. Maybe move declaration and definitions of the new method to be adjacent To ClosestTo.

@dellaert dellaert merged commit a0c298a into develop Oct 6, 2020
@dellaert dellaert deleted the fix/372 branch October 6, 2020 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes an issue or bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rot3 determinant!=1 after recursive operation
3 participants