Skip to content

Cleanup matplotlib.use #13117

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 1 commit into from
Feb 2, 2019
Merged

Cleanup matplotlib.use #13117

merged 1 commit into from
Feb 2, 2019

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Jan 6, 2019

PR Summary

@@ -1297,15 +1297,15 @@ def __exit__(self, exc_type, exc_value, exc_tb):
self.__fallback()


def use(arg, warn=False, force=True):
def use(backend, warn=False, force=True):
Copy link
Member Author

Choose a reason for hiding this comment

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

Technically, this is an API change. I'm 0.5 on this because it makes clear what the function does and what the argument is about. I doubt that someone has been doing matplotlib.use(arg='agg') so I dare go without a deprecation and backward compatibility code. (But that position is open for discussion).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs deprecation code, but an api change note perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

I doubt that someone has been doing matplotlib.use(arg='agg')

Users are very surprising ;) .

I'm -0.5 on this. It is definitely clearer, but is the improved clarity worth breaking even a handful of users and ruining their day?

Copy link
Contributor

@anntzer anntzer Jan 7, 2019

Choose a reason for hiding this comment

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

I actually think the answer is yes, it is; but if you insist

def _rename_parameter(since, old, new, func=None):
    """
    Decorator indicating that a function parameter is being renamed.

    The actual implementation of *func* should use *new*, not *old*.

    Examples
    --------

    ::
        @_rename_parameter("3.1", "bad_name", "good_name")
        def func(good_name): ...
    """

    if func is None:
        return functools.partial(_rename_parameter, since, old, new)

    new_sig = inspect.signature(func)
    old_sig = new_sig.replace(parameters=[
        param.replace(name=old) if param.name == new else param
        for param in new_sig.parameters.values()])

    def _bind_or_none(signature, *args, **kwargs):
        try:
            return signature.bind(*args, **kwargs)
        except TypeError:
            return None

    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        try:
            return func(*args, **kwargs)
        except TypeError:
            new_ba = _bind_or_none(new_sig, *args, **kwargs)
            if new_ba is not None:
                # Args match the new signature, so the TypeError was not raised
                # during argument binding; reraise it.
                raise
            old_ba = _bind_or_none(old_sig, *args, **kwargs)
            if old_ba is None:
                # Args match neither the old nor the new signature; reraise the
                # argument binding failure.
                raise
        # Move out of the except: block to avoid exception chaining.  The
        # function was called with the old signature.  Emit a deprecation
        # warning and call with the new signature.
        warn_deprecated(
            since, message=f"The {old!r} argument of "
            f"{func.__name__}() has been renamed {new!r} since "
            f"{since}; support for the old name will be dropped "
            f"%(removal)s.")
        return func(*old_ba.args,
                    **{key if key is not old else new: value
                       for key, value in old_ba.kwargs.items()})

    return wrapper

courtesy of the house.

Use:

@_rename_parameter("3.1", "arg", "backend")
def use(backend, ...): ...

(The implementation is kind of overly general but also prepares further cases I want to implement such as deleting an argument or making arguments keyword-only...)

Copy link
Member

Choose a reason for hiding this comment

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

👍 to adding this to cbook as per phone conversation.

@timhoffm timhoffm added this to the v3.1 milestone Jan 6, 2019
The first parameter of `matplotlib.use` has been renamed from *arg* to
*backend*. This will only affect cases where that parameter has been set
as a keyword argument. The common usage pattern as a positional argument
(``matplotlib.use('Qt5Agg')`` is not affected.
Copy link
Contributor

Choose a reason for hiding this comment

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

parenthesis

'standard' backend names:
backend : str
The backend to switch to. This can either be one of the standard
backend names:
Copy link
Contributor

Choose a reason for hiding this comment

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

standard backend names, which are case-insensitive: ... and remove the note below

@jklymak
Copy link
Member

jklymak commented Jan 15, 2019

OK, is this waiting for the rename decorator? Feel free to change the labels back if I'm incorrect.

@anntzer
Copy link
Contributor

anntzer commented Jan 15, 2019

would prefer so, but it's not the end of the world if this one goes in first and I rebase the other (basically depends on how strongly @tacaswell feels about the temporary API break)

@timhoffm
Copy link
Member Author

Can be reviewed, now that #13128 is in.

@@ -1184,13 +1184,13 @@ def __exit__(self, exc_type, exc_value, exc_tb):
@cbook._rename_parameter("3.1", "arg", "backend")
def use(backend, warn=False, force=True):
"""
Set the matplotlib backend to one of the known backends.
Select the matplotlib backend for rendering.
Copy link
Contributor

Choose a reason for hiding this comment

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

not just for rendering -- also for GUI toolkit integration (in fact, mostly for that purpose, as I'd guess most uses are selecting qt5agg vs gtk3agg vs tkagg rather than agg vs cairo vs pdf...)

Copy link
Member Author

Choose a reason for hiding this comment

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

The interpretation of "rendering" should be broad here, as in "Render into a Qt window". The original docstring was more or less tautological. OTOH just "Set the matplotlib backend." is a bit short and does not help if you have no clue what a backend is for.

Better descriptions are welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Select the backend used for rendering and GUI integration."?

Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

So sorry it took so long and thanks!

@anntzer anntzer merged commit 615046f into matplotlib:master Feb 2, 2019
@timhoffm timhoffm deleted the use-cleanup branch February 2, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants