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

Add inaxes method to FigureCanvas to check whether point is in an axes. #9845

Merged
merged 6 commits into from
Feb 10, 2019
Merged

Conversation

lkjell
Copy link
Contributor

@lkjell lkjell commented Nov 24, 2017

Add inaxes method to FigureCanvas to check whether point is in an axes.
Return the top-most axes if found, else None.

PR Summary

An alternative way to get an axes given (x,y) coordinate is to create a LocationEvent and use the inaxes attribute. However, instantiate LocationEvent may trigger axes_enter_event and axes_leave_event, which is not one often would like to do. With this method a new way to find the axes without triggering axes_enter_event and axes_leave_event.

From issue #9821.

PR Checklist

  • Has Pytest style unit tests
  • [x ] Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • [x ] Documentation is sphinx and numpydoc compliant
  • [x ] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer
Copy link
Contributor

anntzer commented Nov 24, 2017

As noted in #9821 I'd rather 1. make Artist.contains be able to take xy input + 2. expose cbook._topmost_artist as public API, which should together provide a much more general solution.

In order to disambiguate whether xy is in figure or in axes or in data coordinates, we could e.g. fiddle with kwargs to support

ax.contains(event)
ax.contains(figure_coords=(x, y))
ax.contains(axes_coords=(x, y))
ax.contains(data_coords=(x, y))

@lkjell
Copy link
Contributor Author

lkjell commented Nov 24, 2017

Problem is how should a user know if there exist a topmost artist method? Heck everything can be done with artist.contains_points. And do some transformasjon between coordinates. Thus the only thing that is really needed is just to expose cbook._topmost_artist.

@lkjell
Copy link
Contributor Author

lkjell commented Nov 24, 2017

In any cases a specific method is not mutually exclusive with general case. I believe for new user which is not matplotlib guru to find this method useful.

@anntzer
Copy link
Contributor

anntzer commented Nov 24, 2017

"How does the user know about topmost_artist" and "how does the user know about inaxes" are essentially the same problem -- discoverability of rarely used functions. I'd rather document a more widely useable function (topmost_artist), possibly with some examples in the docstring that show how the function can be used to solve the inaxes problem, rather than add yet another very specialized tool.

@lkjell
Copy link
Contributor Author

lkjell commented Nov 24, 2017

No it is two different problems. If you have an issue related to figure you go into the Figure class and look for its method. You do not go and look in artist (assume you are putting it there). The inaxes method is specialized method because it does what it should do. If you really want to find it usage then just use it in LocationEvent.

@@ -1531,15 +1531,9 @@ def __init__(self, name, canvas, x, y, guiEvent=None):
self._update_enter_leave()
return

# Find all axes containing the mouse
if self.canvas.mouse_grabber is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably don't want to lose the mouse-grabbing effect.

@anntzer
Copy link
Contributor

anntzer commented Nov 24, 2017

My point is that this has in fact nothing to do with axes and figures: what you want to know is rather what artist (if any), among a list of artists, is the topmost at a point in the canvas... (and I am all for documentation that makes {contains + topmost_artist} more discoverable).
In any case I'll let other devs chime in, as I believe I have made my point.

@lkjell
Copy link
Contributor Author

lkjell commented Nov 24, 2017

Just because you do not find its usage I just used it in LocationEvent.

axes_list = [a for a in self.get_axes() if a.patch.contains_point(xy)]

if axes_list:
axes = max(reversed(axes_list), key=lambda x: x.zorder)
Copy link
Member

Choose a reason for hiding this comment

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

For consistency we probably should use _topmost_artist here (so if we ever need to change that logic we can do it exactly once).

@tacaswell tacaswell added this to the v2.2 milestone Nov 25, 2017
@tacaswell
Copy link
Member

@anntzer Instead of having a zoo of kwargs, we probably should follow the pattern of taking a transform.

@lkjell contains_points is a Path method not an artist method, but you point stands. From a library point of view we have to sort out what makes sense to add as methods and what makes sense to leave as tools for users to implement what they need and where to put it.

This method may make more sense on CanvasBase (as that is the layer that definitely knows about screen coordinates) rather than on Figure (in your example in #9821 that is where you put it).

@lkjell lkjell changed the title Add inaxes method to figure to check whether point is in an axes. Add inaxes method to FigureCanvas to check whether point is in an axes. Nov 26, 2017
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

@tacaswell your point of adding a transform kwarg to contains (for example) is well taken.
I really do not believe this method should be added, so I am going to reject the changes. Other reviewers should feel free to dismiss this review if they do not consider my arguments convincing.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I think this is a useful thing to have factored out and exposed and is a reasonable implementation.

@jklymak jklymak modified the milestones: needs sorting, v3.0 Mar 6, 2018
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This looks good to me. Needs a rebase

@lkjell
Copy link
Contributor Author

lkjell commented Mar 9, 2018

@tacaswell and @jklymak since there are 2 approvals it can be merged now?

@tacaswell tacaswell modified the milestones: v3.0, v3.1 Jul 7, 2018
@jklymak
Copy link
Member

jklymak commented Feb 10, 2019

Merging over @anntzer disapproval. This tool doesn't have a lot of complicated machinery associated with it, and seems like it is a nice shortcut. I agree that we don't want to have a bunch of duplicated methods but in the absence of the work noted above exposing the API, this seems like a harmless stopgap.

@jklymak jklymak merged commit 2fd8f26 into matplotlib:master Feb 10, 2019
@jklymak
Copy link
Member

jklymak commented Feb 11, 2019

This broke the build, so suggesting we revert for now. @lkjell if you care to rebase and figure out what broke, that would be fine...

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.

5 participants