FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix detecting which artist(s) the mouse is over by megies · Pull Request #10356 · matplotlib/matplotlib · GitHub

Repository navigation

fix detecting which artist(s) the mouse is over - #10356

Merged
jklymak merged 1 commit into
matplotlib:masterfrom
megies:fix_cursor_coordinates_multiple_images
Oct 6, 2018
Merged

jklymak merged 1 commit into
matplotlib:masterfrom
megies:fix_cursor_coordinates_multiple_images

Conversation

megies commented Jan 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

There is a bug in how moving the mouse over the plot detects what artist should be used to collect additional info about the cursor position from (in my case several Images in the axes). For a plot with multiple images this means that only one image is properly reporting z coordinates and all other images won't be able to show z coordinate info.

Example:

import matplotlib.pyplot as plt
import numpy as np
fig, ax = plt.subplots()
ax.imshow(np.random.randn(25).reshape(5, 5), extent=[0, 3, 0, 1])
ax.imshow(np.random.randn(25).reshape(5, 5), extent=[0, 3, 2, 3])
ax.set_ylim(0, 3)
plt.show()

Only moving through one of the images shows correct z coordinates in the interactive window info bar at the bottom.

The reason for this bug is an oversight in #3989:

Since Artist.contains() returns a tuple it is not usable like this:

    if my_artist.contains():
        ...

So, what is happening is that all artists are included in the list of artists that contain the event and then the wrong artist is queried for data on the location of the event.

Since the name of the method "contains" might easily in the future be prone to such bugs again, I think a better solution would be to change the return from a 2-tuple (which will always evaluate to True) to only False (not contained) or a dictionary (contained) that in minimal fashion is set up like {'artist': self} so that it evaluates to True. But obviously that would drastically change a public API, so I'm assuming this won't be an option, likely..

Sorry, I'm not sure how to best test this, I think to add a test would mean to first refactor NavigationToolbar2.mouse_move() so that getting the cursor info from all artists is in a separate helper method..

PR Checklist

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

jklymak commented Jan 31, 2018

Copy link
Copy Markdown
Member

This fix seems right.

Can you rebase against a recent master - your test failure should be fixed in recent changes to master (I think).

I think you are correct that the return from contains() could have been better, but I think you are alos correct that changing it would be a bit of a project.

megies commented Jan 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I think you are correct that the return from contains() could have been better, but I think you are alos correct that changing it would be a bit of a project.

Yeah. Changing it would be simple, but it will definitely hurt people that use .contains() in third party code. So, yeah, not an option, I figured.

Can you rebase against a recent master

I thought bug fixes used to be against v2.1.x (hmm.. I think they used to be called maintenance_...?).. did I miss a change in the workflow? Are bug fixes now also made against master and then backported? If yes, then I can rebase, no problem.

QuLogic commented Jan 31, 2018

Copy link
Copy Markdown
Member

maintenance_... is ObsPy, not Matplotlib. There are no more 2.1.z releases planned, so it's best to target master. In any case, it'll be backported if that changes.

megies force-pushed the fix_cursor_coordinates_multiple_images branch from 39f759c to 57e0f0a Compare February 1, 2018 10:03
megies changed the base branch from v2.1.x to master February 1, 2018 10:04

megies commented Feb 1, 2018

Copy link
Copy Markdown
Contributor Author

Alright, rebased on master and switched base branch..

jklymak commented Oct 6, 2018

Copy link
Copy Markdown
Member

@megies. Sorry this fell off the radar. It’s still not passing tests though I’m not sure if that is the prs fault. Can you ping if it’s done?

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

With a rebase...

since Artist.contains() returns a tuple it is not usable like
"if my_artist.contains(): do something"
megies force-pushed the fix_cursor_coordinates_multiple_images branch from 57e0f0a to d0762c3 Compare October 6, 2018 09:51

megies commented Oct 6, 2018

Copy link
Copy Markdown
Contributor Author

Rebase done.

anntzer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

anyone can merge post-CI

jklymak merged commit 52245b3 into matplotlib:master Oct 6, 2018

jklymak commented Oct 6, 2018

Copy link
Copy Markdown
Member

Thanks @megies!

QuLogic added this to the v3.1 milestone Oct 6, 2018
megies deleted the fix_cursor_coordinates_multiple_images branch October 8, 2018 08:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL