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

Added a convenience function for plotting 2D points. #1042

Merged
merged 2 commits into from
Jan 19, 2022

Conversation

magicbycalvin
Copy link
Contributor

Included a convenience function for plotting 2D points in Python. The new code closely resembles the existing plotting functions available in gtsam.utils.plot. Plotting a 2D point is useful for visualizing the location of landmarks.

@dellaert dellaert requested a review from varunagrawal January 18, 2022 03:25
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.

LGTM but please address my comment before merging.

if P is not None:
w, v = np.linalg.eig(P)

# k = 2.296
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kill this line? And maybe add a comment on what k is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the commented out line and included a comment as to what k is (the "sigma" value for the uncertainty ellipse). I specifically chose 5 sigma as that is the same value used in the other functions, such as plot_pose2_on_axes.
Thanks!

@dellaert
Copy link
Member

@varunagrawal I'll let you merge

@varunagrawal varunagrawal merged commit ee7d32d into borglab:develop Jan 19, 2022
if P is not None:
w, v = np.linalg.eig(P)

# "Sigma" value for drawing the uncertainty ellipse. 5 sigma corresponds
Copy link
Contributor

Choose a reason for hiding this comment

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

@magicbycalvin This is not correct in 2D, your values seem to be for the 1D case. See Geometric interpretation here and read this. Can you fix that in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will go ahead and look into that and update it. Depending on what I learn from the links and any additional reading I come across, the new PR may also have changes to the k values in some of the other functions as well.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I added a note in #1063, and I think k=5 indeed corresponds to 99.9996% in 2D case. Check the note and let me know... That PR could be amended to accommodate a change if needed, if @gchenfc does not merge it first.

Copy link
Member

Choose a reason for hiding this comment

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

It's in plot.py, in that PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just now seeing this discussion. I have since added an additional PR addressing the comments above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I added a note in #1063, and I think k=5 indeed corresponds to 99.9996% in 2D case. Check the note and let me know... That PR could be amended to accommodate a change if needed, if @gchenfc does not merge it first.

While I agree with 99.9996% (see #1067), that is not what was written in the comment:

to a 99.9999% confidence, i.e. assuming the estimation has been computed properly, there is a 99.999% chance that the true position

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.

4 participants