-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Cleanup matplotlib.use #13117
Conversation
@@ -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): |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parenthesis
lib/matplotlib/__init__.py
Outdated
'standard' backend names: | ||
backend : str | ||
The backend to switch to. This can either be one of the standard | ||
backend names: |
There was a problem hiding this comment.
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
OK, is this waiting for the rename decorator? Feel free to change the labels back if I'm incorrect. |
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) |
Can be reviewed, now that #13128 is in. |
lib/matplotlib/__init__.py
Outdated
@@ -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. |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."?
There was a problem hiding this 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!
PR Summary