Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Cleanup matplotlib.use#13117
Uh oh!
There was an error while loading.Please reload this page.
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 doingmatplotlib.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?
anntzerJan 7, 2019 • edited by tacaswell
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by tacaswell
Uh oh!
There was an error while loading.Please reload this page.
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): ... """iffuncisNone:returnfunctools.partial(_rename_parameter,since,old,new)new_sig=inspect.signature(func)old_sig=new_sig.replace(parameters=[param.replace(name=old)ifparam.name==newelseparamforparaminnew_sig.parameters.values()])def_bind_or_none(signature,*args,**kwargs):try:returnsignature.bind(*args,**kwargs)exceptTypeError:returnNone@functools.wraps(func)defwrapper(*args,**kwargs):try:returnfunc(*args,**kwargs)exceptTypeError:new_ba=_bind_or_none(new_sig,*args,**kwargs)ifnew_baisnotNone:# Args match the new signature, so the TypeError was not raised# during argument binding; reraise it.raiseold_ba=_bind_or_none(old_sig,*args,**kwargs)ifold_baisNone:# 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.")returnfunc(*old_ba.args,**{keyifkeyisnotoldelsenew:valueforkey,valueinold_ba.kwargs.items()})returnwrapper
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.
Uh oh!
There was an error while loading.Please reload this page.
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
jklymak commentedJan 15, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 backendto one of the known backends. | |||
Select the matplotlib backendfor 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