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.

Sign up for GitHub

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

Jump to bottom

Fix Preintegration Covariance of CombinedImuFactor #879

Merged
merged 29 commits into from
Jul 20, 2022
Merged

Fix Preintegration Covariance of CombinedImuFactor #879

merged 29 commits into from
Jul 20, 2022

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Sep 20, 2021 •
edited
Loading

This PR serves as a discussion thread for debugging the Preintegration Covariance of the CombinedImuFactor.

This PR provides a set of unit tests for the CombinedImuFactor and a new CombinedScenarioRunner class which is the ScenarioRunner equivalent for the CombinedImuFactor.

@varunagrawal varunagrawal added the bugfix Fixes an issue or bug label Sep 20, 2021
@varunagrawal varunagrawal self-assigned this Sep 20, 2021
@varunagrawal varunagrawal changed the base branch from fix/368 to develop September 20, 2021 01:10
@varunagrawal varunagrawal changed the base branch from develop to fix/368 September 20, 2021 01:10
@varunagrawal
Copy link
Collaborator Author

Pushed the current draft of the updated ImuFactor.pdf here.

@varunagrawal varunagrawal linked an issue Sep 30, 2021 that may be closed by this pull request
Details about IMU Integration Covariance #878
Open
@varunagrawal varunagrawal mentioned this pull request Feb 22, 2022
Closed
@varunagrawal varunagrawal linked an issue Feb 22, 2022 that may be closed by this pull request
The role of the integration covariance #1114
Closed
@gigcastro
Copy link

Hii, I'm very interested in testing out the Preintegration+CombinedImuFactor implementation. Are the fixes and updated docs ready to be merged? I can provide some feedback afterwards, thanks!

@sia79
Copy link

sia79 commented May 2, 2022 via email

@varunagrawal
Copy link
Collaborator Author

We're waiting on #882 after which this should be ready to go.

@varunagrawal varunagrawal requested a review from lucacarlone May 5, 2022 15:43
@varunagrawal varunagrawal mentioned this pull request May 5, 2022
Merged
@varunagrawal varunagrawal changed the title [WIP] Fix Preintegration Covariance of CombinedImuFactor Fix Preintegration Covariance of CombinedImuFactor Jul 5, 2022
@varunagrawal
Copy link
Collaborator Author

@lucacarlone I had forgotten I had added a bunch of unit tests here for exactly what you were asking for in #882.

@lucacarlone
Copy link
Contributor

@lucacarlone I had forgotten I had added a bunch of unit tests here for exactly what you were asking for in #882.

ok thanks!

@varunagrawal
Copy link
Collaborator Author

@lucacarlone whenever you get a chance, I'd love to get a review from you on this (especially since you are most familiar with the changes). I'd like to land this and #874 as soon as possible. 🙂

@varunagrawal
Copy link
Collaborator Author

#874 is super easy to review. Only the CombindImuFactor.cpp file needs to be reviewed.

@lucacarlone
Copy link
Contributor

#874 is super easy to review. Only the CombindImuFactor.cpp file needs to be reviewed.

@varunagrawal I should be able to get to this by Wednesday (and hopefully before then)

lucacarlone
lucacarlone approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@lucacarlone lucacarlone left a comment

Choose a reason for hiding this comment

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

@varunagrawal this looks good to me but I did not re-review the math.

@varunagrawal varunagrawal merged commit c767dfa into fix/368 Jul 20, 2022
@varunagrawal varunagrawal deleted the fix/combined-imu-cov branch July 20, 2022 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucacarlone lucacarlone lucacarlone approved these changes

@dellaert dellaert Awaiting requested review from dellaert

Assignees

@varunagrawal varunagrawal

Labels
bugfix Fixes an issue or bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Details about IMU Integration Covariance The role of the integration covariance
4 participants
@varunagrawal @gigcastro @sia79 @lucacarlone