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

Adds unit test for imu preintegration of a single step #633

Merged
merged 9 commits into from
Jan 20, 2021

Conversation

raabuchanan
Copy link

Related to issue #627

This adds a unit test which integrates simulated data from an IMU in a foot while to foot takes a single step. See issue for related dataset.

Currently the unit test actually passes which is in contradiction to what I am seeing in the issue. I need to investigate the problem further but wanted to put up this unit test to keep everyone in the loop. Also someone may spot something wrong that I'm doing. I will write more unit tests to try and understand the problem.

@varunagrawal
Copy link
Collaborator

I believe we can merge all 3 files into 1 so we have a comprehensive test file that is easy to modify. While I like the structure a lot, it seems like a lot of added complexity.

@varunagrawal
Copy link
Collaborator

@raabuchanan I've made some updates to the code to make it agnostic to users. I'd love to work with you on this PR and figure this issue out.

Currently, I am a bit confused on what are we testing for specifically? Is there a specific assertion you want to test for?

@varunagrawal varunagrawal force-pushed the fix/foot-imu branch 2 times, most recently from 46c376c to e54ef58 Compare December 14, 2020 23:41
@dellaert
Copy link
Member

@varunagrawal , please help land? Also, don’t force-push :-)

@varunagrawal
Copy link
Collaborator

Since @raabuchanan got the Imu impact working, what should we be testing for? The same thing as before?

@raabuchanan please could you refresh my memory on the goal of this test? I believe you had a slide on it.

@raabuchanan
Copy link
Author

Hi @varunagrawal yes I fixed the issue on my side and this passes now. This test checks a sequence of simulated IMU data that the relative Z position is zero. It also checks that there are no rotations. I will push a small cleanup soon but I can add more documentation if you want.

@varunagrawal
Copy link
Collaborator

Awesome. I'll clean it up a little bit and then we can merge.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Very minor comments, but LGTM.

tests/testImuPreintegration.cpp Outdated Show resolved Hide resolved
tests/testImuPreintegration.cpp Outdated Show resolved Hide resolved
@varunagrawal varunagrawal changed the title [WIP] Adds unit test for imu preintegration of a single step Adds unit test for imu preintegration of a single step Jan 19, 2021
@varunagrawal varunagrawal merged commit 06d8ec2 into develop Jan 20, 2021
@varunagrawal varunagrawal deleted the fix/foot-imu branch January 20, 2021 02:03
@varunagrawal varunagrawal restored the fix/foot-imu branch January 20, 2021 02:03
@dellaert dellaert deleted the fix/foot-imu branch December 29, 2021 00:22
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