Skip to content

Let Formatters format all ticks at once. #12909

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

Merged
merged 3 commits into from
Jan 29, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 30, 2018

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
Copy link
Member

jklymak commented Nov 30, 2018

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
Copy link
Contributor Author

anntzer commented Nov 30, 2018

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
Copy link
Member

jklymak commented Nov 30, 2018

So, can you implement LogFormatter with the new method?

@tacaswell tacaswell added this to the v3.1 milestone Dec 3, 2018
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

modulo an api_changes or whats_new entry.

@jklymak
Copy link
Member

jklymak commented Dec 4, 2018

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

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

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

@anntzer
Copy link
Contributor Author

anntzer commented Jan 1, 2019

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

@jklymak
Copy link
Member

jklymak commented Jan 2, 2019

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

@anntzer
Copy link
Contributor Author

anntzer commented Jan 2, 2019

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
Copy link
Member

jklymak commented Jan 28, 2019

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
Copy link
Contributor Author

anntzer commented Jan 28, 2019

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
Copy link
Member

jklymak commented Jan 28, 2019

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
Copy link
Contributor Author

anntzer commented Jan 28, 2019

added whatsnew

@anntzer
Copy link
Contributor Author

anntzer commented Jan 28, 2019

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
Copy link
Member

jklymak commented Jan 28, 2019

ooops, thanks for catching that....

@anntzer
Copy link
Contributor Author

anntzer commented Jan 29, 2019

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

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

LevN0 commented Feb 1, 2019

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.

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