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

Change from biasAccOmegaInt to biasAccOmegaInit #1709

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

varunagrawal
Copy link
Collaborator

Made these changes a few years ago, and wanted to create the PR for whenever it was deemed appropriate to merge.

This PR changes all instances of biasAccOmegaInt to biasAccOmegaInit which is the correct name for the initial noise on the bias for both the accelerometer and the gyroscope.

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

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

Wanted to do this for years, but I'll let Frank decide :P

@ProfFan
Copy link
Collaborator

ProfFan commented Aug 18, 2024

Ping @dellaert

@dellaert
Copy link
Member

Convince me that this is the correct change. There is also an integration covariance mentioned in the paper. Are we sure this is not that? How is an initialization part of a parameter?

@varunagrawal
Copy link
Collaborator Author

PreintegrationParams has an integrationCovariance which is what is used for integration.

const Matrix3& iCov = p().integrationCovariance;

In that same file, we have bInitCov which is assigned from p().biasAccOmegaInt.

const Matrix6& bInitCov = p().biasAccOmegaInt;

biasAccOmegaInit should be the initial covariance for the 6 dimensional bias term encapsulating both the acceleration and angular velocity (aka ConstantBias) and is used by the CombinedImuFactor for preintegrating and propagating the bias covariance.

@dellaert
Copy link
Member

Ok, you convinced me. But how could we make this change in a way that does not break existing user code?

@varunagrawal
Copy link
Collaborator Author

Ok, you convinced me. But how could we make this change in a way that does not break existing user code?

This has been the big question for me. I wonder if we can put in a deprecation warning like message that lets users know the variable name has changed? If we're cutting 4.3a as a new release, can we make an exception?

@dellaert
Copy link
Member

Typically, we bracket in ifdefs, so we can do that here as well?

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