Skip to content

FIX: make un-used ticks not be visible #10881

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

FIX: make un-used ticks not be visible #10881

wants to merge 2 commits into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Mar 26, 2018

PR Summary

Addresses #10874

This code fails master:

import matplotlib
matplotlib.use('Agg')
import matplotlib.pyplot as plt

fig = plt.figure()
ax = fig.add_subplot(111, frameon=True)
ax.plot()

plt.get_current_fig_manager().canvas.draw()

for xtick in ax.get_xticklabels():
    xtick.get_window_extent()

This was originally caused by #9452 adding string data to the tick labels that were not seen. Previously get_window_extent would bail on an empty string. But after #9452 the labels were not empty. But they were never drawn, so they never had self._renderer assigned. This makes the labels to be visible if they are not visible, and that causes get_window_extent to ignore them.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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 requested a review from anntzer March 26, 2018 17:15
@jklymak jklymak added this to the v3.0 milestone Mar 26, 2018
@jklymak
Copy link
Member Author

jklymak commented Mar 26, 2018

I don't think this is 2.2.3 material, but others may disagree...

@jklymak jklymak mentioned this pull request Mar 26, 2018
Closed
anntzer
anntzer reviewed Mar 26, 2018
View reviewed changes
lib/matplotlib/axis.py Outdated
@@ -1102,7 +1102,12 @@ def _update_ticks(self, renderer):
tick.set_label1(label)
tick.set_label2(label)
if not mtransforms.interval_contains(interval_expanded, loc):
tick.set_visible(False)
tick.label1.set_visible(False)
Copy link
Contributor

Choose a reason for hiding this comment

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

either also mark the gridline and ticks themselves as invisible, or add a comment about what's happening

Copy link
Member Author

Choose a reason for hiding this comment

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

OK< I can do all that, but its a bit ugly. See the second commit. Tempted to just revert #9452, which I think was handling a bit of an edge-case use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can just leave a comment on why it is only needed to mark the labels as not visible...

Copy link
Member Author

Choose a reason for hiding this comment

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

Preference? I don't have one...

I agree its a bit hincky to have the extra ticks. I forget why we do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have written in many places that we should be stricter and require the ticker to only return visible ticks (after all it has all the info needed to do so). But I guess that's not happening any time soon and instead we just move the issues further down the stack :/

Copy link
Member Author

Choose a reason for hiding this comment

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

Agenda item for today’s call!

anntzer
anntzer reviewed Mar 26, 2018
View reviewed changes
lib/matplotlib/axis.py Outdated
@@ -1102,7 +1102,12 @@ def _update_ticks(self, renderer):
tick.set_label1(label)
tick.set_label2(label)
if not mtransforms.interval_contains(interval_expanded, loc):
tick.set_visible(False)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this line have any effect?

@jklymak jklymak force-pushed the fix-getting-extent-invisible-ticks branch from ecda915 to 1a14655 Compare March 26, 2018 17:39
dstansby
dstansby approved these changes Apr 7, 2018
View reviewed changes
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Might be worth adding the code above as a quick smoke test?

@dstansby dstansby modified the milestones: v3.0, v2.2.3 Apr 7, 2018
@dstansby
Copy link
Member

dstansby commented Apr 7, 2018

I've put this to 2.2.3, since it's a regression in the 2.2 branch.

@efiring
Copy link
Member

efiring commented Apr 7, 2018

This is all very confusing, and it seems like a case of whack-a-mole. Isn't there a cleaner solution than putting invisible ticks into a list of ticks to draw? I think @aantzer is pointing to it in his comment #9397 (comment). I suspect the root issue is the interaction between the Locator and autoscaling, and what is needed is an earlier distinction between potential tick locations, relevant to autoscaling, and actual tick locations that will be drawn in the end. This is further complicated by the usual floating point fudge to include a tick that falls an infinitessimal distance beyond the edge.

@efiring
Copy link
Member

efiring commented Apr 7, 2018

To consolidate: the moles include #10874, #9397, and #9452.
This PR might be OK as a bugfix for 2.2.3--one last whack, hope that mole is knocked out cold--but I would not like to see it as the real solution for 3.0.

@jklymak
Copy link
Member Author

jklymak commented Apr 7, 2018

Sorry. Thought I’d opened an issue for this. Yes ticks that are returned from the locators should just be the ones thatll be drawn not a couple of extra in case of round off errors. This fix is clearly just a bandaid

@jklymak
Copy link
Member Author

jklymak commented Apr 8, 2018

Ah, right. See #5665 for recent discussion with @timhoffm planning to take the lead on improving ticks.

@jklymak jklymak mentioned this pull request Apr 8, 2018
Open
@jklymak
Copy link
Member Author

jklymak commented Apr 28, 2018

Closing in lieu of #11004, though feel free to resurrect if we want in 2.2.3

@jklymak jklymak closed this Apr 28, 2018
@jklymak jklymak deleted the fix-getting-extent-invisible-ticks branch April 28, 2018 21:32
@tobycheese tobycheese mentioned this pull request May 7, 2018
Closed
@jklymak jklymak mentioned this pull request Sep 4, 2018
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer left review comments

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
topic: ticks axis labels
Projects
None yet
Milestone
v2.2.3
Development

Successfully merging this pull request may close these issues.
4 participants
@jklymak @dstansby @efiring @anntzer