Skip to content
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

Deprecate positional use of most arguments of plotting functions #27786

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

timhoffm
Copy link
Member

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.

Comment on lines +4 to +5
Many plotting functions will restrict positional arguments to the first few parameters
in the future. All further configuration parameters will have to be passed as keyword
Copy link
Member

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?)

Copy link
Member Author

@timhoffm timhoffm Feb 13, 2024

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:

  • fmt is allowed positionally - inspired by plot
  • I’ve still allowed bins in histograms because from hist(data, 20) it's conceivable that the inter is bins(debatable, but err on the permissive side)
  • With hlines I've gone to the extreme and still allowed color and linestyles because if somebody has written hlines(y, xmin, xmax, 'red', 'dashed') that's quite clear and I don't want to break their code.

@ksunden
Copy link
Member

ksunden commented Feb 13, 2024

There seems to be an order dependence in boilerplate.py that is expecting deprecators to be the outermost decorator rather than just anywhere:

meth = getattr(class_, called_name)
decorator = _api.deprecation.DECORATORS.get(meth)
# Generate the wrapper with the non-kwonly signature, as it will get
# redecorated with make_keyword_only by _copy_docstring_and_deprecators.
if decorator and decorator.func is _api.make_keyword_only:
meth = meth.__wrapped__

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).

@timhoffm timhoffm force-pushed the kwonly branch 2 times, most recently from 7e10766 to 87b8b2e Compare March 6, 2024 21:30
@ksunden ksunden removed this from the v3.9.0 milestone Mar 6, 2024
@ksunden ksunden added this to the v3.10.0 milestone Mar 6, 2024
@ksunden
Copy link
Member

ksunden commented Mar 6, 2024

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.
@@ -4636,6 +4644,7 @@ def invalid_shape_exception(csize, xsize):
colors = None # use cmap, norm after collection is created
return c, colors, edgecolors

@_api.make_keyword_only("3.9", "marker")
Copy link
Member

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.

Copy link
Member

@tacaswell tacaswell left a 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.

@tacaswell
Copy link
Member

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.

@tacaswell tacaswell merged commit 5e0aa54 into matplotlib:main Apr 8, 2024
43 of 44 checks passed
@timhoffm timhoffm deleted the kwonly branch April 8, 2024 18:02
@timhoffm
Copy link
Member Author

timhoffm commented Apr 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: changes Documentation: examples files in galleries/examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants