Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

megies
Copy link
Contributor

@megiesmegies commentedJan 31, 2018
edited
Loading

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 severalImages 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:

importmatplotlib.pyplotaspltimportnumpyasnpfig,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:

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

ifmy_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 toTrue) to onlyFalse (not contained) or a dictionary (contained) that in minimal fashion is set up like{'artist': self} so that it evaluates toTrue. 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 refactorNavigationToolbar2.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 featuresNew features are documented, with examples if plot related
  • no new docsDocumentation is sphinx and numpydoc compliant
  • no new featureAdded an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • no API changeDocumented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak
Copy link
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 fromcontains() could have been better, but I think you are alos correct that changing it would be a bit of a project.

@megies
Copy link
ContributorAuthor

megies commentedJan 31, 2018
edited
Loading

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 againstv2.1.x (hmm.. I think they used to be calledmaintenance_...?).. did I miss a change in the workflow? Are bug fixes now also made againstmaster and then backported? If yes, then I can rebase, no problem.

@QuLogic
Copy link
Member

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

@megiesmegiesforce-pushed thefix_cursor_coordinates_multiple_images branch from39f759c to57e0f0aCompareFebruary 1, 2018 10:03
@megiesmegies changed the base branch fromv2.1.x tomasterFebruary 1, 2018 10:04
@megies
Copy link
ContributorAuthor

Alright, rebased onmaster and switched base branch..

@jklymak
Copy link
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?

Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

With a rebase...

since Artist.contains() returns a tuple it is not usable like"if my_artist.contains(): do something"
@megiesmegiesforce-pushed thefix_cursor_coordinates_multiple_images branch from57e0f0a tod0762c3CompareOctober 6, 2018 09:51
@megies
Copy link
ContributorAuthor

Rebase done.

Copy link
Contributor

@anntzeranntzer left a comment

Choose a reason for hiding this comment

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

anyone can merge post-CI

@jklymakjklymak merged commit52245b3 intomatplotlib:masterOct 6, 2018
@jklymak
Copy link
Member

Thanks@megies!

@QuLogicQuLogic added this to thev3.1 milestoneOct 6, 2018
@megiesmegies deleted the fix_cursor_coordinates_multiple_images branchOctober 8, 2018 08:23
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

4 participants
@megies@jklymak@QuLogic@anntzer

[8]ページ先頭

©2009-2025 Movatter.jp