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

Let Formatters format all ticks at once. by anntzer · Pull Request #12909 · matplotlib/matplotlib · GitHub

Repository navigation

Let Formatters format all ticks at once. - #12909

Merged
efiring merged 3 commits into
matplotlib:masterfrom
anntzer:format_ticks
Jan 29, 2019
Merged

efiring merged 3 commits into
matplotlib:masterfrom
anntzer:format_ticks

Conversation

anntzer commented Nov 30, 2018

Copy link
Copy Markdown
Contributor

This is useful especially when labels are interdependent (e.g.
ConciseDateFormatter, but also LogFormatter which currently relies on
set_locs, etc.). xref #10841, maybe of interest for @jklymak.

Because set_major_formatter/set_minor_formatter (somewhat unusually)
enforces that its argument is a Formatter subclass, even third-party
formatters will inherit from the base class and have this method
defined.

PR Summary

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 commented Nov 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

I like this and agree it makes a lot more sense, but...

If someone calls formatter() for some reason (old custom axis of some sort?) it still needs to do the interdependent calculation, doesn’t it? I’m just not at all clear that we could move the interdependent code from LogFormatter.__call__ into this function without breaking someone’s code. I’m also not clear what now goes in __call__

anntzer commented Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

For ConciseDateFormatter, you'd just write a stub __call__ that calls format_ticks and extracts the single relevant value (perhaps the base __call__ could even check whether format_ticks is overridden by the subclass (the check is necessary to avoid infinite recursion between format_ticks and __call__...), and, if it is, defer to it -- it's not that hard to do). Yes, that's a bit slower, but that only affects third party callers and simplifies the implementation quite a bit so I think it's worth it.
With this design, one would need to implement either __call__ or format_ticks, but not both (ignoring issues such as formatting cursor values, which can also be fixed). For LogFormatter we just need to check whether it's indeed better to move the logic to format_ticks (and again just make __call__ defer to it).

jklymak commented Nov 30, 2018

Copy link
Copy Markdown
Member

So, can you implement LogFormatter with the new method?

tacaswell added this to the v3.1 milestone Dec 3, 2018

tacaswell 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

modulo an api_changes or whats_new entry.

jklymak commented Dec 4, 2018

Copy link
Copy Markdown
Member

As per above - needs change log and ideally a real-world example...

efiring 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

Apart from the one suggested API change, and the need for an API note, I like this.

Comment thread lib/matplotlib/ticker.py

anntzer commented Jan 1, 2019

Copy link
Copy Markdown
Contributor Author

The real-world example is basically ConciseDateFormatter (happy to rewrite it using this API after the ConciseDateFormatter is merged).

jklymak commented Jan 2, 2019

Copy link
Copy Markdown
Member

Happy to do it the other way around if that makes more sense.

anntzer commented Jan 2, 2019

Copy link
Copy Markdown
Contributor Author

I think it makes more sense to get ConciseDate in first and then we'll be better able to see whether this PR fixes the design properly, or if we need some other API.

jklymak commented Jan 28, 2019

Copy link
Copy Markdown
Member

ping @anntzer ConciseDateFormatter is now in, so this can be looked at again?

I'm not sure how this plays with the issues of the minor locator ignoring major ticks. They are separate issues, right?

anntzer commented Jan 28, 2019

Copy link
Copy Markdown
Contributor Author

Pushed an adaptation of ConciseDateFormatter using the format_ticks API, it is a bit simpler.
This PR only touches formatters, so has nothing to do with locators.

jklymak commented Jan 28, 2019

Copy link
Copy Markdown
Member

LGTM! Still needs the API change note?

This is useful especially when labels are interdependent (e.g.
ConciseDateFormatter, but also LogFormatter which currently relies on
set_locs, etc.).

Because `set_major_formatter`/`set_minor_formatter` (somewhat unusually)
enforces that its argument is a Formatter subclass, even third-party
formatters will inherit from the base class and have this method
defined.

anntzer commented Jan 28, 2019

Copy link
Copy Markdown
Contributor Author

added whatsnew

anntzer commented Jan 28, 2019

Copy link
Copy Markdown
Contributor Author

I think I made a mistake while reimplementing ConciseDateFormatter; namely, the "compression" should be used regardless of whether show_offset is True or False and show_offset only controls whether offset_string is set. Force-pushed a fix restoring the behavior as intended by @jklymak.

jklymak commented Jan 28, 2019

Copy link
Copy Markdown
Member

ooops, thanks for catching that....

anntzer commented Jan 29, 2019

Copy link
Copy Markdown
Contributor Author

Travis failure is the unrelated, spurious wqy-zenhei.ttc test.

efiring merged commit 64c84b4 into matplotlib:master Jan 29, 2019
anntzer deleted the format_ticks branch January 29, 2019 16:24

LevN0 commented Feb 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Apologies if not the right place. This PR has the following side effect:

fig, ax = plt.subplots()

data = np.clip(randn(250, 250)*1000, 0, 1000)

cax = ax.imshow(data, norm=mpl.colors.LogNorm(vmin=1, vmax=1000))
cbar = fig.colorbar(cax, format='%d')

cbar.minorticks_off()
cbar.set_ticks([5, 10, 100])

plt.show()

Prior to this PR, minor ticks would remain off after set_ticks when they had been turned off. After this PR, they come back on.

Edit: Comment is wrong, this PR did not change this.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL