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

FIX: ignore axes that aren't visible #16175

Merged
merged 2 commits into from Jan 10, 2020

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jan 9, 2020

PR Summary

Closes #16173

Layering subplots causes the topmost subplot to capture mouse events, which seems fine. However, if the topmost subplot is made invisible (which is a trick we use in cosntrained_layout) then it would still capture the mouse event. Given that an axes with ax.get_visble() is False is never drawn, not are any of its children, it seem unlikely that anyone would want to pick on a placeholder like that so the son here is to ignore invisible axes.

Note that I also changed how constrained_layout makes the axes invisible. It passes all the tests, so this is preferable to the way I was doing it before.

Test

import matplotlib.pyplot as plt
import matplotlib.gridspec as gridspec
from matplotlib.widgets import Button

def on_click(event):
    print("hey")

fig = plt.figure(constrained_layout=True)

gs = gridspec.GridSpec(2, 1, figure=fig)

ax1 = fig.add_subplot(gs[0, 0])
ax1.text(0.5, 0.5, "ax1", va="center", ha="center")

gs2 = gs[1, 0].subgridspec(1, 2)
ax2 = fig.add_subplot(gs2[0, 1])

b = Button(ax2, 'hey')
b.on_clicked(on_click)

plt.show()

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • 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

@jklymak jklymak force-pushed the fix-mouse-ignore-non-visible-axes branch from 6ab4d31 to 8a12e8b Compare January 9, 2020 17:27
@jklymak jklymak added this to the v3.2.1 milestone Jan 9, 2020
@jklymak jklymak added topic: geometry manager LayoutEngine, Constrained layout, Tight layout Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels Jan 9, 2020
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.

I'll trust you that it fixes the issue reported on SO, but this certainly makes sense.

@jklymak
Copy link
Member Author

jklymak commented Jan 9, 2020

Is there an easy way to test button clicks?

@anntzer
Copy link
Contributor

anntzer commented Jan 9, 2020

It's slightly a pain, see

def do_event(tool, etype, button=1, xdata=0, ydata=0, key=None, step=1):
or https://github.com/anntzer/mplcursors/blob/1ac466c613613edc8660161a407380db52985b48/tests/test_mplcursors.py#L59

@jklymak
Copy link
Member Author

jklymak commented Jan 9, 2020

added test in test_axes.py

@ImportanceOfBeingErnest
Copy link
Member

Due to the fix from f215d09 this is fine.

Though in general, I think not having any extra axes in the game would be much better.
Would it be possible to remove the axes after it has been used as placeholder?

@jklymak
Copy link
Member Author

jklymak commented Jan 9, 2020

Not easily. The layout boxes used by constrained layout are attributes of the axes.

You could imagine re-architecting the whole thing so the layout box was the attribute of some sort of subplotspec object, but we don't have those (the axes also keeps track of its subplotspec). I don't think its too bad a hack, but certainly if we re-architect all the layout engines it should be considered.

Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest left a comment

Choose a reason for hiding this comment

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

I could imagine other problems occuring because of extra axes being present in the figure.
Maybe in the longer run one could imagine constrained_layout keeping track of the axes it needs, without them necessarily being part of the figure?

In any case, this fixes the bug, so approving as is.

@jklymak jklymak force-pushed the fix-mouse-ignore-non-visible-axes branch from 98c06ca to 9f8235e Compare January 10, 2020 00:09
"""
axes_list = [a for a in self.figure.get_axes()
if a.patch.contains_point(xy)]

if a.patch.contains_point(xy) and a.get_visible()]
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps reverse the order of the checks? get_visible() should be faster (just an attribute check) than contains_point()
@jklymak can self-merge with or without this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

The first check is to make sure the pick is actually in the axes (because its in pixels). So if the second check failed, and you didn't have the first, you wouldn't know if it failed because of a resolution change in the tests or because the functionality actually broke.

In normal conditions both tests should pass, so I don't think there is a typical time savings here.

@jklymak jklymak merged commit b6d3336 into matplotlib:master Jan 10, 2020
@lumberbot-app
Copy link

lumberbot-app bot commented Jan 10, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v3.2.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 b6d333642e2cf8882183ca8071ad411ae5b19b73
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am "Backport PR #16175: FIX: ignore axes that aren't visible"
  1. Push to a named branch :
git push YOURFORK v3.2.x:auto-backport-of-pr-16175-on-v3.2.x
  1. Create a PR against branch v3.2.x, I would have named this PR:

"Backport PR #16175 on branch v3.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@jklymak jklymak deleted the fix-mouse-ignore-non-visible-axes branch January 10, 2020 17:19
jklymak added a commit to jklymak/matplotlib that referenced this pull request Jan 10, 2020
jklymak added a commit to jklymak/matplotlib that referenced this pull request Jan 10, 2020
jklymak added a commit that referenced this pull request Jan 10, 2020
…3.2.x

Backport PR #16175: FIX: ignore axes that aren't visible
@QuLogic QuLogic modified the milestones: v3.2.1, v3.2.0 Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constrained_layout creates extra axes when used with subgridspec
4 participants