-
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
Fix uncertainty ellipses in python helpers #1177
Conversation
which will be represented as an ellipse. | ||
""" | ||
|
||
w, v = np.linalg.eig(covariance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, changing that to eigh is even better I think. This would prevent from getting complex results due to float precision issues. Comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@senselessDev just noticed that your PR is targeting the |
Yes, because I was not really sure if everything @dellaert wanted is adressed. Obviously, once it is on his branch, I hope he can add if something's still left to do and then merge to |
Will do, sorry for delay - strapped for time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is awesome! Sorry for the delay in reviewing. I will merge into my branch, try it out, and then create a PR to develop.
Because #1067 was not merged into develop, the uncertainty ellipses are still plotted wrong on develop.
I'd like to have that fixed and updated the explanation in the script and tried to avoid the duplicate implementation.
I'm not sure if it was 100% in a way @dellaert wished for. So please have a look.
I set the scaling in such a way that the uncertainty ellipse would include 95% of samples as inliers.
It does not change the fixed bugs by @magicbycalvin, just reorganizes it.
In 2D it is doing the right thing:
Can be tested with