Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Init MaxNLocator params only once #12998

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
Merged
merged 1 commit into from
Dec 28, 2018
Merged

Init MaxNLocator params only once #12998

merged 1 commit into from
Dec 28, 2018

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Dec 16, 2018 •
edited
Loading

PR Summary

This makes creating MaxNLocator a bit faster (~3ms -> ~2ms) by calling set_params only once.

Edit:

Original PR was just: self.set_params(**{**self._default_params, **kwargs}).

Additionally, I've introduced preparatory steps to make the signature explicit, which needs a lot of deprecation logic. It's still easiest to make the signature only explicit after the deprecation period is over.

@timhoffm timhoffm added this to the v3.1 milestone Dec 16, 2018
@anntzer
Copy link
Contributor

anntzer commented Dec 16, 2018

I'd suggest just making the signature explicit

    def __init__(self, nbins=10, *,
                 steps=None,
                 integer=False,
                 symmetric=False,
                 prune=None,
                 min_n_ticks=2
                 ):

and kill default_params...

@timhoffm
Copy link
Member Author

timhoffm commented Dec 16, 2018 •
edited
Loading

Formally, making the signature explicit is backwards incompatible.

  1. MaxNLocator(20, nbins=30) will not work (currently, it's using 20). That's basically a desired change, but do we have to deprecate and warn first?
  2. default_params is public and somebody might have overwritten values.
  3. Other kwargs to MaxNLocator were silently ignored until now and would raise an error with the explicit signature.

How much do we care about these things?

@anntzer
Copy link
Contributor

anntzer commented Dec 16, 2018

I really meant "do the necessary deprecation dance to ultimately make the signature explicit".

@timhoffm timhoffm force-pushed the maxnlocator-set-defaults branch from be4008b to e881dc1 Compare December 16, 2018 20:34
anntzer
anntzer reviewed Dec 16, 2018
View reviewed changes
doc/api/next_api_changes/2018-12-16-TH-deprecations.rst Outdated
deprecated and will be removed in a future version. The defaults are not
supposed to be user-configurable.

``matplotlib.ticker.MaxNLocator`` and it's ``set_params`` method will issue
Copy link
Contributor

Choose a reason for hiding this comment

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

its

anntzer
anntzer reviewed Dec 16, 2018
View reviewed changes
lib/matplotlib/ticker.py Outdated
@@ -1905,6 +1922,11 @@ def _staircase(steps):

def set_params(self, **kwargs):
"""Set parameters within this locator."""
if set(kwargs) - self._supported_kwargs:
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just pop the kwargs at every if clause below and then check if kwargs is empty. (Note that this check will need to stay even after the deprecation period is over and the signature of init changed, as we need to distinguish between a kwarg not passed to set_params and a kwarg set to its default.)

@timhoffm timhoffm force-pushed the maxnlocator-set-defaults branch from e881dc1 to 7c46d33 Compare December 16, 2018 22:21
anntzer
anntzer reviewed Dec 16, 2018
View reviewed changes
lib/matplotlib/ticker.py Outdated
self._integer = kwargs['integer']
self._integer = kwargs.pop('integer')
if kwargs:
param = next(k for k in kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do k, _ = kwargs.popitem() or k, *_ = kwargs which are both more natural IMO, but it's not a big deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do tomorrow (and also fix the flake8 issue)

Copy link
Member Author

Choose a reason for hiding this comment

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

Comments handled.

@timhoffm timhoffm force-pushed the maxnlocator-set-defaults branch from 7c46d33 to a1179e9 Compare December 17, 2018 14:34
@timhoffm timhoffm force-pushed the maxnlocator-set-defaults branch from a1179e9 to f084e0d Compare December 17, 2018 19:57
@dstansby dstansby merged commit 0a9b5d0 into matplotlib:master Dec 28, 2018
@timhoffm timhoffm deleted the maxnlocator-set-defaults branch December 28, 2018 22:49
@QuLogic
Copy link
Member

QuLogic commented Apr 18, 2019

According to @jklymak in SciTools/cartopy#1299, this change will break some bits of Cartopy.

@jklymak jklymak mentioned this pull request Apr 20, 2019
Merged
6 tasks
@anntzer anntzer mentioned this pull request Jul 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@timhoffm @anntzer @QuLogic @dstansby
Morty Proxy This is a proxified and sanitized view of the page, visit original site.