Skip to content

Fix displayed 3d coordinates showing gibberish #23485

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

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Jul 25, 2022

PR Summary

Closes #22775 and closes #4334. Also gets rid of the annoying helper function at cause in #23430. Some previous discussion in #22624.

As the first issue shows, the displayed coordinates under the mouse cursor are currently meaningless. The calculation in this PR shows the x, y, z point on the displayed plane at the mouse location. For example, ignoring the broken Z coordinate for now, the mouse here is at the circled location and the coordinates displayed are on the x-z plane (the y coordinate is fixed at any point on this plane).
image

TODO:

  • Coordinates for x and y are working, z is not. Need to figure out why, likely not passing in the right z view coordinates. This page might help
  • https://github.com/matplotlib/matplotlib/pull/23449/files replaces self.vvec with self._view_w, need to switch that over depending on which is merged first. The signs are flipped between those two variables but we multiply twice so it cancels out and can be directly replaced.
  • Tests

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@jklymak
Copy link
Member

jklymak commented Jul 25, 2022

I don't understand how such a number can ever have meaning unless you arbitrarily assign a surface to project onto. That seems of super limited value to me and I think we should just not show a cursor coordinates in 3D.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 25, 2022

The surfaces we are projecting onto here are not so arbitrary - they’re the “corner of a box” formed by the three axis panes which are visible in every 3D plot that is created. We are already drawing gridlines on them, which on its own shows the value of knowing the values of points on those panes and means they should be easy to interpret. At the very least, they’re unambiguous.

I see two use cases which these enable and which I personally would find extremely useful:

  1. Looking at 3D data along each of the primary axes - the coordinates as proposed here are accurate when looking at each of the x-y, y-z, x-z planes. I fail to see how that's not as valuable as hovered coordinates in 2D plots.

  2. Using the panes as “slice planes” through the data, panning the view so that the data is intersected by one of the panes at a desired location, and reading off the coordinates in that plane at that slice. That's useful at any angle, not just looking down the primary axes. For example, if you want the approximate coordinate of a scatter point, you can pan the plot so that point intersects one of the planes and hover over it to read its location.

@jklymak
Copy link
Member

jklymak commented Jul 25, 2022

I'm still not sure how you will communicate to the user that the cursor is running along the backing planes? I guess I agree that doing that is better than having a random output, but it's still going to be a bit mysterious.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 25, 2022

I think it should be pretty discoverable by waving the mouse around? One coordinate is always fixed on each plane, and the transitions when crossing the primary axes are pretty noticeable. Lemme fix the Z coordinate issue and y'all can play around with it and see if it makes sense (I'll take a video too). Open to suggestions on documentation that would make this explicit.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Aug 3, 2022

This is going to take some more effort than I thought, getting the right z coordinate is not a simple fix and will require writing up proper inverse projections. I've got to do some learning on that, so leaving open for now and will try to get back to it when I have time.

@timhoffm
Copy link
Member

Before further work goes into this, I would like to state that I'm also not convinced of this. The planes themselves are not very much of interest for the user. Data locations would be. While one can rotate and pan so that coordinates of a desired data point can be inferred from the displayed pane coordinates, this is an ugly hack at best:

  1. Usually I don't want to change my view to infer a data coordinate. Yes I can to that and then undo.
  2. As @jklymak stated, the behavior is not obvious, and likely surprising to users.

In my opinion, providing 3D coordinates for a 2D cursor position without additional context* is fundamentally flawed, and we should not do this (In the face of ambiguity, refuse the temptation to guess).

*The only reasonable way for returning 3D coordinates is some additional visual feedback and guidance. Something like a 3D snapping crosshair (i.e. the 3D version of https://matplotlib.org/stable/gallery/event_handling/cursor_demo.html#snapping-to-data-points) would work. Unfortunately, I'm afraid, Matplotlib is not well designed for this as you would have to redraw the image when moving the crosshair, which is quite costly and thus would not be very responsive.

@scottshambaugh
Copy link
Contributor Author

I'd support removing the coordinates entirely right now since they're worse than useless at the moment, but I still see a lot of utility in having us show the backing plane coordinates. Let me see if I can get it working and then I can demo it which I think will show that it's not so unintuitive.

@timhoffm
Copy link
Member

Let me see if I can get it working and then I can demo it which I think will show that it's not so unintuitive.

Fair. To set expectations right: As I see currently see it, this should not be a default behavior in Matplotlib. It might be an example, or a third-party package.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Feb 19, 2023

This is working now for all projection types and is ready for review / discussion.

I think the below video demonstrates the behavior well, as well as the utility of this. When looking at data with an ortho view along one of the primary view planes, the behavior is exactly the same as 2D plots. This is super common in my use cases at least, it was the motivation behind adding documentation for the primary view planes here, here, and here, and would become more useful with the addition of this feature. Otherwise the main utility I see is if a user projects an image onto one of the backing panes and wants to read off coordinates from that, as is shown in these gallery examples: 1, 2, 3. It's also a nice quick way to grab the axis limits visually.

It would be nice to have lines along the backing planes to show the cursor location, somewhat analogous to what plotly does, but as @timhoffm points out above the redraw would be prohibitively costly. But I don't think you need this for the behavior to be discoverable - IMO the "fixed" coordinate as you move around on one of the backing panes makes what is happening obvious fairly quickly.

As a side note on the issue of documentation / discoverability, I don't think there is any info in the docs showing the figure GUI and how to use it, unless I'm missing it. I think it would make most sense to put an annotated image of the GUI in here. Descriptions of what the coordinates mean could go there. I'll open a docs issue for that separately (edit: #25266).

2023-02-20.21-18-04-1.mp4

The alternative is removing these hover coordinates altogether. If that end up as the consensus then I'll open another MR with that change (I think there's some good refactoring that can carry over from here). Will bring up in the dev call this week.

@scottshambaugh scottshambaugh force-pushed the 3d_plot_coords branch 3 times, most recently from f2cd740 to 4bd8626 Compare February 20, 2023 16:04
@jklymak jklymak added the status: needs comment/discussion needs consensus on next step label Feb 20, 2023
@scottshambaugh
Copy link
Contributor Author

I update the video above to better show some use cases.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Feb 24, 2023

In the dev call today the consensus was that this is useful and we should do it, but we should explicitly tell the user in the UI that these are coordinates from the axis panes to avoid confusion. I changed it to the below as a maximally compact way of conveying this information, but am open to revising this if it seems unclear.

if pane_idx == 0:
    coords = f'x pane={xs}, y={ys}, z={zs}'
elif pane_idx == 1:
    coords = f'x={xs}, y pane={ys}, z={zs}'
elif pane_idx == 2:
    coords = f'x={xs}, y={ys}, z pane={zs}'
2023-02-23.18-56-02-1.mp4

@scottshambaugh
Copy link
Contributor Author

This is good to go btw.

Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Looks good!

I agree lines on the pane would be nice, but then they should also be optional.

@jklymak
Copy link
Member

jklymak commented Jul 6, 2023

I'm squashing your commits...

Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants