-
Notifications
You must be signed in to change notification settings - Fork 779
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
CombinedImuFactor with just one measurement leads to degeneracy #368
Comments
@varunagrawal take a look at this? |
@mindThomas I assume by covariance term you mean the noise model of the CombinedImuFactor? I just ran an example using your reproduction steps, and I am seeing non-zero terms in the noise model. Please let me know if I am mistaken, or better yet, can you provide me with a minimal working example? I'll gladly fix this if I can identify the issue. 🙂 |
@varunagrawal Here is a minimal example:
Output is:
Conversely with the ImuFactor:
Where there is no zero rows in the measurement covariance:
|
@mindThomas I notice that in your minimal example you're setting the integration covariance to zero. Does this bug still exist when you use a (small) non-zero positive quantity? Of course, in the ImuFactorExamples.cpp code they use eye(3)*1.0e-8. Apologies for not confirming this myself right now, but I can check in a few days. It was just an initial thought I had when reading this issue. I certainly have a gut reaction towards never specifying actually-zero variance noise models. |
@tmcg0 Theoretically speaking the integration covariance should not be necessary. The way I understand it is that you add the integration covariance to capture unmodelled errors as a result from the discrete integration (would be even worse if we weren't doing proper Lie Group integration). So if the integration is perfectly continuous there shouldn't be a need for this integration covariance. The result of
Now the position covariance is no longer 0, but it is still extremely small:
|
A quick update (because it drove me nuts trying to resolve this), the output from the ImuFactor code should be multiplied by a factor of 1e-6. |
@varunagrawal sorry, can you be specific as to what output you're referring to? The covariance? @mindThomas totally should have gotten back to you a while ago. Regarding your first paragraph ("theoretically speaking...."): There's always unmodeled sources of error. Always. I feel like it should be standard practice to never use zero for the integration covariance quantity (you never know if the people who wrote the code unit tested for a zero value!). But I think your practical example in the second paragraph ("Also the integration covariance is set to 0...") does show evidence of a bug. Trivially, position covariance should be non-zero merely from integration of a (non-zero noise) accelerometer measurement. It would be awesome if we could build a unit test for this that fails with an assert statement. I told myself I wanted to look into this and then got totally sidetracked. |
This example
Where there is no zero rows in the measurement covariance:
This issue is going to take a while since I need to redo the math for noise propagation for |
Any updates on this? |
Okay I think I figured this out! Had to go through all the math and then some but I did it. Opening a PR. |
Draft PR #874 opened. I need to update ImuFactor.pdf to account for what I've learned while solving this issue so we can verify the correctness of the PR. |
@varunagrawal @dellaert : I spoke too early: I do have an extensive write-up on the Combine IMU factor that I wrote back when I was at GT. I'll follow up shortly to share it via email. |
Awesome-sauce |
@lucacarlone Is it possible that you can share this doc with the rest of us as well? |
@mindThomas : I think @varunagrawal is preparing a revised document that he will share. |
That is correct. @dellaert, @lucacarlone and I have spoken about this and I'm rewriting all the math to ensure we fix this from the inside out. |
Description
If only 1 measurement is integrated into the CombinedImuFactor, the position related covariance terms are all zero.
This only seems to happen for the CombinedImuFactor though. If the same single measurement is integrated into an ImuFactor all elements of the covariance matrix is filled out. No zero rows/columns.
Unfortunately, these zero rows in the covariance leads to degeneracy and the infamous 'Indeterminant linear system' error.
Steps to reproduce
In general the problem can be reproduced with the ImuFactorsExample.cpp by ensuring that only 1 measurement is integrated: preintegrated->integrateMeasurement(imu.head<3>(), imu.tail<3>(), dt);
Expected behavior
Even with just one measurement I would expect at the very least all diagonal element to be non-zero of the preintegrated factor covariance, just as is the case if just one measurement is integrated into the ImuFactor.
Additional information
More information can be found here: https://groups.google.com/forum/#!msg/gtsam-users/WskPzqexiNk/UwQrbBe7AAAJ
The text was updated successfully, but these errors were encountered: