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

FIX: make un-used ticks not be visible by jklymak · Pull Request #10881 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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

Closed
jklymak wants to merge 2 commits into
matplotlib:masterfrom
jklymak:fix-getting-extent-invisible-ticks
ConversationCommits (2)ChecksFiles changed
Closed

FIX: make un-used ticks not be visible#10881
jklymak wants to merge 2 commits into
matplotlib:masterfrom
jklymak:fix-getting-extent-invisible-ticks

Conversation

jklymak commented Mar 26, 2018

Copy link
Copy Markdown
Member

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 requested a review from anntzer March 26, 2018 17:15
jklymak added this to the v3.0 milestone Mar 26, 2018

jklymak commented Mar 26, 2018

Copy link
Copy Markdown
Member Author

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

jklymak mentioned this pull request Mar 26, 2018
Closed
anntzer reviewed Mar 26, 2018
View reviewed changes
Comment thread lib/matplotlib/axis.py
tick.set_label2(label)
if not mtransforms.interval_contains(interval_expanded, loc):
tick.set_visible(False)
tick.label1.set_visible(False)

anntzer Mar 26, 2018

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

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

jklymak Mar 26, 2018

Copy link
Copy Markdown
Member Author

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

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.

anntzer Mar 26, 2018

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

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

jklymak Mar 26, 2018

Copy link
Copy Markdown
Member Author

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

Preference? I don't have one...

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

anntzer Mar 26, 2018

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

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

jklymak Mar 26, 2018

Copy link
Copy Markdown
Member Author

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

Agenda item for today’s call!

anntzer reviewed Mar 26, 2018
View reviewed changes
Comment thread lib/matplotlib/axis.py Outdated
tick.set_label1(label)
tick.set_label2(label)
if not mtransforms.interval_contains(interval_expanded, loc):
tick.set_visible(False)

anntzer Mar 26, 2018

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

does this line have any effect?

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

dstansby 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

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

dstansby modified the milestones: v3.0, v2.2.3 Apr 7, 2018

dstansby commented Apr 7, 2018

Copy link
Copy Markdown
Member

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

efiring commented Apr 7, 2018

Copy link
Copy Markdown
Member

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 commented Apr 7, 2018

Copy link
Copy Markdown
Member

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 commented Apr 7, 2018

Copy link
Copy Markdown
Member Author

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 commented Apr 8, 2018

Copy link
Copy Markdown
Member Author

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

jklymak mentioned this pull request Apr 8, 2018
Open

jklymak commented Apr 28, 2018

Copy link
Copy Markdown
Member Author

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

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

Reviewers

anntzer anntzer left review comments
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

Back | FazBrowse Home | New Git URL