-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Deprecate positional use of most arguments of plotting functions #27786
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
Conversation
9df5ed6 to
e2ecd2d
Compare
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.
Is there any way to have some measure of consistency on which parameters are positional only (like for example the data parameters?)
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 central question is: Is code using the positional parameters understandable without knowing the signature? Mostly this results in positional data parameters. But with some exceptions:
fmtis allowed positionally - inspired byplot- I’ve still allowed
binsin histograms because fromhist(data, 20)it's conceivable that the inter is bins(debatable, but err on the permissive side) - With
hlinesI've gone to the extreme and still allowed color and linestyles because if somebody has writtenhlines(y, xmin, xmax, 'red', 'dashed')that's quite clear and I don't want to break their code.
e2ecd2d to
3ca4bbe
Compare
3ca4bbe to
3edfce3
Compare
There seems to be an order dependence in boilerplate.py that is expecting deprecators to be the outermost decorator rather than just anywhere:
matplotlib/tools/boilerplate.py
Lines 160 to 165 in 1defb57
For instance, violinplot in this branch has both the _preprocess_data decorator and the make_keyword_only decorator, but the _api.deprecation.DECORATORS.get returns None:
>>> mpl.__version__ '3.9.0.dev1136+g3edfce3b0e' >>> mpl._api.deprecation.DECORATORS.get(mpl.axes.Axes.violinplot) >>> mpl._api.deprecation.DECORATORS.get(mpl.axes.Axes.violinplot.__wrapped__) functools.partial(<function make_keyword_only at 0x7f641386bce0>, '3.9', 'vert') >>> mpl._api.deprecation.DECORATORS.get(mpl.axes.Axes.violin) functools.partial(<function make_keyword_only at 0x7f641386bce0>, '3.9', 'vert')
If you reach into __wrapped__ or if you inspect violin (which has only the one decorator) the lookup works.
DECORATORS is just a dictionary that gets added to with the wrapped version of the method as keys and the decorator itself as value. Once it has been wrapped again, the connection is lost.
Thus I think the easiest corrective action to make pyplot not prematurely implement kwonly args (without warning) is to move the deprecations to be the outermost decorators... That or the boilerplate code needs to grow to search the entire meth.__wrapped__ sequence from outside in rather than only looking at the outermost (which would also make it more robust to multiple deprecatoions on the same method).
At first I thought it was some of the code I wrote in boilerplate to add type hints in, but I confirmed that I do not change the param types from the meth parameter extracted by these lines (which predate those changes).
3edfce3 to
566f0cf
Compare
566f0cf to
b8707c1
Compare
b8707c1 to
b58f4e8
Compare
b58f4e8 to
476e0ec
Compare
476e0ec to
ec12544
Compare
ec12544 to
427de70
Compare
7e10766 to
87b8b2e
Compare
In discussion with @tacaswell and @QuLogic we decided to push this to early 3.10 rather than late 3.9.
This increases maintainability for developers and disallows bad practice of passing lots of positional arguments for users. If in doubt, I've erred on the side of allowing as much positional arguments as possible as long as the intent of a call is still readable. Note: This was originally motivated by bxp() and boxplot() having many overlapping parameters but differently ordered. While at it, I think it's better to rollout the change to all plotting functions and communicate that in one go rather than having lots of individual change notices over time. Also, the freedom to reorder parameters only sets in after the deprecation. So let's start this now.
... to the error message, that gets thrown if it is not.
87b8b2e to
163758a
Compare
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 can see an arguament for pushing this one deeper
ax.scatter(x, y, s, c, 'o')
scans reasonably well, but I suspect is rare.
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.
We should be open to feedback on these if they turn out to be more disruptive than we anticipate.
The appveyor failure is issues with gobject importing so I'm going to merge over that.
I'm going to merge this (as we have branched for 3.9) to get it in as early as possible and to make the odds of finding major disruptions down stream as high as possible.
I'm torn if the deprecation note on this is exuberant enough for the scale of the change.
I'm torn if the deprecation note on this is exuberant enough for the scale of the change.
I believe most affected users will learn from the runtime warning anyway. But feel free to change the note.
In discussion with @tacaswell and @QuLogic we decided to push this to early 3.10 rather than late 3.9.
The warnings still say 3.9 👀 Should we correct them this far down the path?
It doesn't matter too much. But we should watch out that we don't shorten expiration because we noted a version number too low. So likely the simplest thing is updating.
This increases maintainability for developers and disallows bad practice of passing lots of positional arguments for users. If in doubt, I've erred on the side of allowing as much positional arguments
as possible as long as the intent of a call is still readable.
Note: This was originally motivated by bxp() and boxplot() having many overlapping parameters but differently ordered.
While at it, I think it's better to rollout the change to all plotting functions and communicate that in one go rather than having lots of individual change notices over time. Also, the freedom to reorder parameters only sets in after the deprecation. So let's start this now.