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

Add decorator to inherit keyword-only deprecations #14130

Closed

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented May 4, 2019

PR Summary

This PR allows to deprecate positional usage of arguments also for Axes/pyplot functions.

Motivation

This PR enables a straight forward way to deprecate positional usage of arguments so that we can cleanly evolve the API to a state in which only a few positional parameters are allowed per function (e.g. def set_xlabel(self, xlabel, *, fontdict=None, ...). This restriction of the API has two goals:

  • Prevent easily-breaking use by passing lots of positional arguments (don't allow bad style).
  • It gives us more freedom to change the kwarg-only parts of the signatures; e.g. removing or introducing parameters.

How it works

This augments the _make_keyword_only decorator, which couldn't be used with the above functions so far. It works like this:

  • The _make_keyword_only decorator attaches its call parameters to the wrapper function.
    This decorator can e.g. be applied to Axes.set_xlabel:

    @_make_keyword_only('fontdict')
    def set_xlabel(self, xlabel, fontdict=None, labelpad=None, **kwargs):
    
  • The new _inherit_make_keyword_only decorator reads these parameters and thus can create a wrapper for the respective pyplot function:

    @_inherit_make_keyword_only(Axes.set_xlabel):
    def xlabel(xlabel, fontdict=None, labelpad=None, **kwargs):
    

    which is similar to

    @_make_keyword_only('fontdict')
    def xlabel(xlabel, fontdict=None, labelpad=None, **kwargs):
    

    but with two advantages:

    • We don't have to explicitly state 'fontdict' again.
    • The wrapper additionally suppresses warnings from the called function (wrapper of set_xlabel).
  • boilerplate.py now applies the _inherit_make_keyword_only decorator to all generated pyplot functions. Manually written wrappers would have to add it themselves if needed.

  • There's one additional detail to the pyplot wrappers: boilerplate.py generates their signature based on the signature of the called (e.g. Axes-) function. Now, we still have to take the original signature, not the signature that was rewritten by _make_keyword_only(Axes.set_xlabel), otherwise we cannot apply _make_keyword_only to the pyplot function.

Scope

This is only the technical implementation plus tests. Actual _make_keyword_only depreactions will be handled and discussed in separate PRs.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant

@timhoffm timhoffm added this to the v3.2.0 milestone May 4, 2019
@anntzer
Copy link
Contributor

anntzer commented May 5, 2019

If we want to support this, it may be easier to use a decorator to completely swap out the body of the pyplot wrapper; something like (approximately, untested).

@pyplot_wrapper(gca, Axes.foo, ret_handler=lambda ret: ...)
def foo(...):
    pass  # see pyplot_wrapper for the actual implementation


def pyplot_wrapper(obj_getter, unbound_method, ret_handler=lambda x: x, func=None):
    if func is None: return partial(...)
    
	@docstring.copy(unbound_method)
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        ret = unbound_method(obj_getter(), *args, **kwargs)
        ret_handler(ret)
        return ret

    return wrapper

now the "body" of the pyplot function is empty, its signature is just here for static analysis tools, and the actual implementation just forwards args and kwargs "as is" to the wrapped function.

This avoids requiring boilerplate.py to have to know about the deprecating decorators.

@timhoffm
Copy link
Member Author

timhoffm commented May 9, 2019

Thanks for the feedback @pyplot_wrapper is an interesting alternative. I'm a bit concerned that it's too much magic.

Understanding what this does

@pyplot_wrapper(gca, Axes.foo, ret_handler=lambda ret: ...)
def foo(...):
    pass  # see pyplot_wrapper for the actual implementation

is more complicated than

@cbook._inherit_make_keyword_only(Axes.foo)
@docstring.copy(Axes.foo)
def foo(...):
    return gca().foo(...)

Ok, we have two decorators, but they only perform some sort of preprocessing. The actual execution logic is written out explicitly. I've often investigated what the code actually does by checking the code link in the documentation or ?? in ipython. I assume that the second form is more helpful for users trying to understand how pyplot works.

@anntzer
Copy link
Contributor

anntzer commented May 10, 2019

Well, we can "keep" the old function body in the source of pyplot.py, it's just that it's not going to be executed :) (then yes, the function body becomes a "lie", but only in the sense that we keep track of what arguments were passed positionally and what arguments were passed by keyword.)

@timhoffm
Copy link
Member Author

I don't think we should add a "lie" to our code. At some point, the written code could deviate from the actual functionality of the decorator. pass # see pyplot_wrapper for the actual implementation at least clearly states what's going on.

I'm really not sure which way is better. This should be discussed with the other developers.

@anntzer
Copy link
Contributor

anntzer commented May 19, 2019

I guess another question is, what are the cases where you would want to make a parameter keyword-only?

The case where I introduced this decorator is for pyplot.show(block=...), because I would like to change the first positional argument to be a list of figures to show instead. Indeed, the decorator helps when you want to replace a given positional argument by another one. But this should be quite rare. If we want to remove a keyword parameter, we have _delete_parameter (which has the nice side effect of turning the subsequent parameters keyword-only, but that's really just a side effect); if we want to add a keyword parameter, we can add it at the end of the signature. I also don't think that just "not allowing bad style" (of passing tons of arguments positionally) is a sufficient reason for the added complexity.

However, I may certainly be missing some other use cases. Thus, it would be nice to see some examples of the places where you intend to use the improved form of this decorator.


Also, another possibility if you want to use this decorator in pyplot is to just write the pyplot wrapper manually, instead of generating it with boilerplate.py.

@timhoffm
Copy link
Member Author

There are a number of Axes methods that should IMHO only have a limited number or positional parameters. Apart from the bad style, I cannot safely reorder kw parameters to get them in a meaningful order. For example, #14107 introduced the parameter quantiles to violinplot, which semantically belongs next to showmedians, but since there is no limit on positional arguments, this is technically a breaking change.

@anntzer
Copy link
Contributor

anntzer commented May 19, 2019

I guess I would rather go with manually writing the wrapper in that case (it's not optimal but not the end of the world either...).

diff --git i/lib/matplotlib/axes/_axes.py w/lib/matplotlib/axes/_axes.py
index 2bae8ff07..7e80921cc 100644
--- i/lib/matplotlib/axes/_axes.py
+++ w/lib/matplotlib/axes/_axes.py
@@ -7903,6 +7903,7 @@ optional.
         return im
 
     @_preprocess_data(replace_names=["dataset"])
+    @cbook._make_keyword_only("3.2", "vert")
     def violinplot(self, dataset, positions=None, vert=True, widths=0.5,
                    showmeans=False, showextrema=True, showmedians=False,
                    quantiles=None, points=100, bw_method=None):
diff --git i/lib/matplotlib/pyplot.py w/lib/matplotlib/pyplot.py
index 4f266917e..857c39ffe 100644
--- i/lib/matplotlib/pyplot.py
+++ w/lib/matplotlib/pyplot.py
@@ -2317,6 +2317,21 @@ switch_backend(rcParams["backend"])
 install_repl_displayhook()
 
 
+# Move this back to autogen after deprecation period over.
+@docstring.copy(Axes.violinplot)
+@cbook._make_keyword_only("3.2", "vert")
+def violinplot(
+        dataset, positions=None, vert=True, widths=0.5,
+        showmeans=False, showextrema=True, showmedians=False,
+        quantiles=None, points=100, bw_method=None, *, data=None):
+    return gca().violinplot(
+        dataset, positions=positions, vert=vert, widths=widths,
+        showmeans=showmeans, showextrema=showextrema,
+        showmedians=showmedians, quantiles=quantiles, points=points,
+        bw_method=bw_method, **({"data": data} if data is not None
+        else {}))
+
+
 ################# REMAINING CONTENT GENERATED BY boilerplate.py ##############
 
 
@@ -3011,20 +3026,6 @@ def triplot(*args, **kwargs):
     return gca().triplot(*args, **kwargs)
 
 
-# Autogenerated by boilerplate.py.  Do not edit as changes will be lost.
-@docstring.copy(Axes.violinplot)
-def violinplot(
-        dataset, positions=None, vert=True, widths=0.5,
-        showmeans=False, showextrema=True, showmedians=False,
-        quantiles=None, points=100, bw_method=None, *, data=None):
-    return gca().violinplot(
-        dataset, positions=positions, vert=vert, widths=widths,
-        showmeans=showmeans, showextrema=showextrema,
-        showmedians=showmedians, quantiles=quantiles, points=points,
-        bw_method=bw_method, **({"data": data} if data is not None
-        else {}))
-
-
 # Autogenerated by boilerplate.py.  Do not edit as changes will be lost.
 @docstring.copy(Axes.vlines)
 def vlines(
diff --git i/tools/boilerplate.py w/tools/boilerplate.py
index caec52474..aaeb60ff4 100644
--- i/tools/boilerplate.py
+++ w/tools/boilerplate.py
@@ -249,7 +249,7 @@ def boilerplate_gen():
         'tricontourf',
         'tripcolor',
         'triplot',
-        'violinplot',
+        # 'violinplot',
         'vlines',
         'xcorr',
         # pyplot name : real name

(or with the "fake body" approach)

@timhoffm
Copy link
Member Author

That doesn't quite work. I can only do changes of the order after the deprecation period. Thus, when I realize I want to change something, I would have to wait a year to do that.

The intended way of working would be to add kwonly deprecations now in reasonable positions, switch them to * after the deprecation and be free from then on.

@anntzer
Copy link
Contributor

anntzer commented May 19, 2019

I personally think the fake-body approach is cleaner, but let's see what others think.

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Sep 10, 2019
@tacaswell tacaswell self-requested a review September 10, 2019 20:01
@anntzer
Copy link
Contributor

anntzer commented Sep 10, 2019

Actually, it is possible to reconstruct the decorators that need to be applied to the pyplot wrappers just from the axes methods... I think the following works:

# in pyplot.py

_code_objs = {
    cbook._rename_parameter:
        cbook._rename_parameter("", "old", "new", lambda new: None).__code__,
    cbook._make_keyword_only:
        cbook._make_keyword_only("", "p", lambda p: None).__code__,
}
def _wraps(method, func=None):
    if func is None:
        return functools.partial(_wraps, method)
    decorators = [docstring.copy(method)]
    while getattr(method, "__wrapped__", None) is not None:
        for decorator_maker, code in _code_objs.items():
            if method.__code__ is code:
                kwargs = {
                    k: v.cell_contents
                    for k, v in zip(code.co_freevars, method.__closure__)}
                assert kwargs["func"] is method.__wrapped__
                kwargs.pop("func")
                decorators.append(decorator_maker(**kwargs))
        method = method.__wrapped__
    for decorator in decorators[::-1]:
        func = decorator(func)
    return func
diff --git i/tools/boilerplate.py w/tools/boilerplate.py
index caec52474..74f4a72b2 100644
--- i/tools/boilerplate.py
+++ w/tools/boilerplate.py
@@ -36,7 +36,7 @@ AUTOGEN_MSG = """
 # Autogenerated by boilerplate.py.  Do not edit as changes will be lost."""
 
 AXES_CMAPPABLE_METHOD_TEMPLATE = AUTOGEN_MSG + """
-@docstring.copy(Axes.{called_name})
+@_wraps(Axes.{called_name})
 def {name}{signature}:
     __ret = gca().{called_name}{call}
     {sci_command}
@@ -44,13 +44,13 @@ def {name}{signature}:
 """
 
 AXES_METHOD_TEMPLATE = AUTOGEN_MSG + """
-@docstring.copy(Axes.{called_name})
+@_wraps(Axes.{called_name})
 def {name}{signature}:
     return gca().{called_name}{call}
 """
 
 FIGURE_METHOD_TEMPLATE = AUTOGEN_MSG + """
-@docstring.copy(Figure.{called_name})
+@_wraps(Figure.{called_name})
 def {name}{signature}:
     return gcf().{called_name}{call}

which I guess is not too different from your solution.

@timhoffm
Copy link
Member Author

@anntzer Not following all the details, but the concept looks good! Want to make a PR? 😄

@anntzer
Copy link
Contributor

anntzer commented Sep 12, 2019

sure, #15254

@timhoffm
Copy link
Member Author

Closing in favor of #15254.

@timhoffm timhoffm closed this Sep 12, 2019
@timhoffm timhoffm mentioned this pull request Jan 11, 2020
13 tasks
@timhoffm timhoffm deleted the make_keyword_only-pyplot branch July 19, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants