Simplify (quite a bit...) _preprocess_data #10928

anntzer · 2018-03-31T02:29:35Z

PR Summary

Public API change: step no longer defaults to using y as
label_namer. This is consistent with other functions that wrap plot
(plot itself, loglog, etc.). (Alternatively, we could make all
these functions use y as label_namer; I don't really care either way.)

The plot-specific data kwarg logic was moved to
_process_plot_var_args, dropping the need for
general callable positional_parameter_names,
_plot_args_replacer, and positional_parameter_names.
test_positional_parameter_names_as_function and tests using
plot_func_varargs were removed as a consequence.

replace_all_args can be replaced by making replace_names=None
trigger replacement of all args, even the "unknown" ones. There was
no real use of "replace all known args but not unknown ones" (even if
there was, this can easily be handled by explicitly listing the args in
replace_names). test_function_call_with_replace_all_args was removed
as a consequence.

replace_names no longer complains if some argument names it is given
are not present in the "explicit" signature, as long as the function
accepts **kwargs -- because it may find the arguments in kwargs
instead. edit: not anymore, we still error out in that case.

label_namer no longer triggers if data is not passed (if the
argument specified by label_namer was a string, then it is
likely a categorical and shouldn't be considered as a label
anyways). test_label_problems_at_runtime was renamed to
test_label_namer_only_if_data and modified accordingly.

Calling data-replaced functions used to trigger RuntimeError in some
cases of mismatched arguments; they now trigger TypeError similarly to
how normal functions do (test_more_args_than_pos_parameters).

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

tacaswell · 2018-03-31T16:51:57Z

(Alternatively, we could make all
these functions use y as label_namer; I don't really care either way.)

I would rather do this, it seems like an oversight that we did not do this for plot and friends originally.

tacaswell · 2018-03-31T18:36:28Z

This is easier to review with https://github.com/matplotlib/matplotlib/pull/10928/files?w=1 Most of the whitespace change is due to using partial instead of an extra layer of inner function to create the paramterized decorator.

tacaswell · 2018-03-31T19:13:29Z

Calling data-replaced functions used to trigger RuntimeError in some
cases of mismatched arguments; they now trigger TypeError similarly to
how normal functions do (test_more_args_than_pos_parameters).

Please document this as an API change.

It seems a good fraction of the simplification comes from using signature.bind and not re-implementing functional that core python provides?

The docstring and signature for plot and friends needs to be updated as they are no longer done by the decorator.

Over all 👍 !

tacaswell

Document API change for return types
Update the docstring and signature of plot and friends

Anyone can dismiss this review.

anntzer · 2018-03-31T20:35:49Z

The simplications come from

don't do the work that signature.bind does
move the plot-specific logic to its own implementation rather than attempt to have a single overgeneralized version
dropping replace_all_args and simplifying the replace_names logic (either replace this, this, this and this named argument, or replace all of them) to restrict them to what we need them to do

anntzer · 2018-03-31T22:07:53Z

Question: Currently,

plot([1, 2], "foo", data={"foo": "y"})

works as expected but

plot([1, 2], [3, 4], "foo", data={"foo": "y"})

crashes ("unrecognized character f in format string"). This PR makes both forms "work", although I'm tempted to make neither work instead (I think the shorthand format string should, well, be a shorthand format string). Thoughts?

anntzer · 2018-03-31T22:38:38Z

The data kwarg actually already has its own documentation in the docstring of plot:

        data : indexable object, optional
            An object with labelled data. If given, provide the label names to
            plot in *x* and *y*.

            .. note::
                Technically there's a slight ambiguity in calls where the
                second label is a valid *fmt*. `plot('n', 'o', data=obj)`
                could be `plt(x, y)` or `plt(y, fmt)`. In such cases,
                the former interpretation is chosen, but a warning is issued.
                You may suppress the warning by adding an empty format string
                `plot('n', 'o', '', data=obj)`.

The same note, without the PS, is present in step; it is not present at all in loglog and friends. While these could be rationalized I think this is out of the scope of this PR.

Added label_namer support for plot and friends. While doing so, moved from using the terribly named get_label to a private _label_from_arg which checks whether its second argument is a string, and, if not so, defaults to None instead (to share that logic between the general and plot-specific implementations).

timhoffm · 2018-06-06T22:03:30Z

Any chances that this will go in soon? Otherwise I propose to preliminary merge #10872 to fix #11389.

anntzer · 2018-06-07T05:30:59Z

rebased

timhoffm · 2018-06-07T11:35:37Z

lib/matplotlib/axes/_axes.py

@@ -4533,7 +4533,7 @@ def barbs(self, *args, **kw):

    # Uses a custom implementation of data-kwarg handling in
    # _process_plot_var_args.
-    def fill(self, *args, **kwargs):
+    def fill(self, *args, data=None, **kwargs):


data should be added to the Parameters section in the docstring

jklymak · 2018-07-09T20:49:22Z

What is the status of this and #10872. Should #10872 go in as a stopgap?

anntzer · 2018-09-10T09:57:50Z

lib/matplotlib/tests/test_preprocess_data.py

@@ -75,36 +58,6 @@ def func_no_ax_args(*args, **kwargs): pass
    with pytest.raises(AssertionError):
        _preprocess_data(label_namer="z")(func_args)

-    # but "ok-ish", if func has kwargs -> will show up at runtime :-(


Deleted these as we don't support "implicit" label_name ("hoping that they'll show up in kwargs") anymore (this was basically present only to support plot() and fill() which don't have y explicitly in the signature but always have a y argument; this PR changes these functions to have their own preprocessor).

anntzer · 2018-09-10T09:58:39Z

lib/matplotlib/tests/test_preprocess_data.py

@@ -205,42 +158,6 @@ def func_replace_all(ax, x, y, ls="x", label=None, w="NOT"):
        func_replace_all(None, x="a", y="b", w="x", label="text", data=data) ==
        "x: [1, 2], y: [8, 9], ls: x, w: xyz, label: text")

-    @_preprocess_data(label_namer="y")
-    def func_varags_replace_all(ax, *args, **kwargs):


See above re: "implicit" label_namer. Otherwise this test function is redundant with func_replace_all just above.

anntzer · 2018-09-10T10:08:44Z

As a side point, after this PR, we may want to consider the possibility of (... being very prudent here :) ...) making _preprocess_data part of the public API (perhaps under a better name, e.g. process_data_kwarg, up to bikeshedding).
This will both allow others to reuse the functionality, and possibly to move out the spectral plotting functions out to their own mpl_toolkit :p (like everyone else they depend on _preprocess data to massage their arguments).

tacaswell · 2018-09-17T19:58:32Z

Sold on this in general and on making preprocess_data public.

anntzer · 2018-11-03T16:48:28Z

rebased

timhoffm · 2018-11-03T18:02:22Z

Sold on this in general and on making preprocess_data public.

I'm a bit hesitant about this. _preprocess_data seems still a bit too clever to be released in the wild.

I'd rather strip out the signature manipulation and instead simply require that the wrapped function has a data parameter.
Also It shouldn't manipulate the docstring. We cannot assume that users would want the data note as we have it now. - Actually I'm not a fan of this for the matplotlib docs as well. The parameter should appear in the Parameters or Additional Parameters section like any other parameter.

anntzer · 2018-11-03T18:07:32Z

I'd rather strip out the signature manipulation and instead simply require that the wrapped function has a data parameter.

But the inner function actually doesn't take data as parameter.

Also It shouldn't manipulate the docstring. We cannot assume that users would want the data note as we have it now. - Actually I'm not a fan of this for the matplotlib docs as well. The parameter should appear in the Parameters or Additional Parameters section like any other parameter.

This has basically been discussed to death in the other docstring manipulation issues.

In any case this PR is not making the function public; let's keep that discussion for when the PR shows up (if ever).

timhoffm · 2018-11-03T18:36:35Z

I'd rather strip out the signature manipulation and instead simply require that the wrapped function has a data parameter.

But the inner function actually doesn't take data as parameter.

Yep, sorry. I was just repeating a thought from long time ago. The above comment was not the full story. Essentially, when stripping off all the fancy manipulation magic of signatures and docstrings, this could be a simple function x, y = preprocess_data(x, y, data=data). But as you said, let's keep that discussion out of here.

Public API change: `step` no longer defaults to using `y` as label_namer. This is consistent with other functions that wrap `plot` (`plot` itself, `loglog`, etc.). (Alternatively, we could make all these functions use `y` as label_namer; I don't really care either way.) The plot-specific data kwarg logic was moved to `_process_plot_var_args`, dropping the need for general callable `positional_parameter_names`, `_plot_args_replacer`, and `positional_parameter_names`. `test_positional_parameter_names_as_function` and tests using `plot_func_varargs` were removed as a consequence. `replace_all_args` can be replaced by making `replace_names=None` trigger replacement of all args, even the "unknown" ones. There was no real use of "replace all known args but not unknown ones" (even if there was, this can easily be handled by explicitly listing the args in replace_names). `test_function_call_with_replace_all_args` was removed as a consequence. `replace_names` no longer complains if some argument names it is given are not present in the "explicit" signature, as long as the function accepts `**kwargs` -- because it may find the arguments in kwargs instead. label_namer no longer triggers if `data` is not passed (if the argument specified by label_namer was a string, then it is likely a categorical and shouldn't be considered as a label anyways). `test_label_problems_at_runtime` was renamed to `test_label_namer_only_if_data` and modified accordingly. Calling data-replaced functions used to trigger RuntimeError in some cases of mismatched arguments; they now trigger TypeError similarly to how normal functions do (`test_more_args_than_pos_parameters`).

Assert that label_namer (if given) is a parameter in the function signature (not "possibly" present via `**kwargs`). This seems more consistent with the expectations of the check later; in fact we can now just remove that check.

We only support passing them positionally; right now passing them as kwargs silently does nothing. The error message is the standard one for unknown kwargs.

anntzer · 2018-11-08T10:01:44Z

Added a commit to make plot() error out when x/y are passed as kwargs (this silently does nothing currently, xref #12106).

tacaswell · 2018-12-24T18:08:05Z

Thanks @anntzer ! Sorry for the slow review on this.

anntzer added this to the v3.0 milestone Mar 31, 2018

anntzer force-pushed the process-plot-args branch 3 times, most recently from 2a2e584 to ebcc968 Compare March 31, 2018 02:42

tacaswell previously requested changes Mar 31, 2018

View reviewed changes

anntzer mentioned this pull request Mar 31, 2018

Cleanup _plot_args_replacer logic #10872

Merged

anntzer force-pushed the process-plot-args branch 3 times, most recently from 1bd1f05 to 5868dd9 Compare April 3, 2018 16:14

anntzer force-pushed the process-plot-args branch from 5868dd9 to df26a24 Compare April 12, 2018 22:20

anntzer force-pushed the process-plot-args branch 4 times, most recently from 5b24966 to 267623c Compare May 15, 2018 06:56

timhoffm mentioned this pull request Jun 6, 2018

Incorrect processing of plot_args when using data argument #11389

Closed

anntzer force-pushed the process-plot-args branch from 267623c to f88f6e8 Compare June 7, 2018 05:30

timhoffm reviewed Jun 7, 2018

View reviewed changes

anntzer force-pushed the process-plot-args branch from 8024651 to 6758c71 Compare June 7, 2018 11:51

tacaswell modified the milestones: v3.0, v3.1 Jul 7, 2018

anntzer force-pushed the process-plot-args branch from 6758c71 to 9df54cc Compare July 8, 2018 10:41

anntzer commented Sep 10, 2018

View reviewed changes

anntzer force-pushed the process-plot-args branch from e431d01 to 139d156 Compare September 17, 2018 12:54

dstansby added the status: needs rebase label Nov 3, 2018

anntzer removed the status: needs rebase label Nov 3, 2018

anntzer force-pushed the process-plot-args branch from 139d156 to dcdea6d Compare November 3, 2018 16:48

timhoffm approved these changes Nov 3, 2018

View reviewed changes

anntzer added 4 commits November 4, 2018 14:39

Inline _grab_next_args.

7b69287

Maintain data=None in plot()'s explicit signature.

6e4c5c1

Make _preprocess_data stricter regarding label_namer.

b07377f

Assert that label_namer (if given) is a parameter in the function signature (not "possibly" present via `**kwargs`). This seems more consistent with the expectations of the check later; in fact we can now just remove that check.

anntzer force-pushed the process-plot-args branch from dcdea6d to b07377f Compare November 4, 2018 13:43

Error out when x/y are passed as kwargs.

434a6a5

We only support passing them positionally; right now passing them as kwargs silently does nothing. The error message is the standard one for unknown kwargs.

anntzer mentioned this pull request Dec 10, 2018

Pandas Series not supported as data kwarg #12971

Closed

timhoffm mentioned this pull request Dec 10, 2018

Fix item check for pandas Series #12973

Closed

1 task

tacaswell merged commit 3826ee5 into matplotlib:master Dec 24, 2018

anntzer deleted the process-plot-args branch December 24, 2018 22:30

anntzer removed the status: needs review label Dec 24, 2018

jklymak mentioned this pull request May 25, 2020

dict unpacking broken for .plot in 3.2 #17502

Closed

story645 mentioned this pull request May 25, 2020

Fix dicts unpacking for .plot #17506

Merged

6 tasks

QuLogic mentioned this pull request Mar 18, 2023

Warning in test_labeled_data_unpacking.test_positional_parameter_names_as_function #5366

Closed

ksunden mentioned this pull request Apr 25, 2023

scalar categoricals are sometimes interpreted as data keys #9844

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify (quite a bit...) _preprocess_data #10928

Simplify (quite a bit...) _preprocess_data #10928

anntzer commented Mar 31, 2018 •

edited

Loading

tacaswell commented Mar 31, 2018

tacaswell commented Mar 31, 2018

tacaswell commented Mar 31, 2018

tacaswell left a comment

anntzer commented Mar 31, 2018

anntzer commented Mar 31, 2018

anntzer commented Mar 31, 2018

timhoffm commented Jun 6, 2018

anntzer commented Jun 7, 2018

timhoffm Jun 7, 2018

anntzer Jun 7, 2018

jklymak commented Jul 9, 2018

anntzer Sep 10, 2018 •

edited

Loading

anntzer Sep 10, 2018

anntzer commented Sep 10, 2018

tacaswell commented Sep 17, 2018

anntzer commented Nov 3, 2018

timhoffm commented Nov 3, 2018

anntzer commented Nov 3, 2018

timhoffm commented Nov 3, 2018

anntzer commented Nov 8, 2018

tacaswell commented Dec 24, 2018

Simplify (quite a bit...) _preprocess_data #10928

Simplify (quite a bit...) _preprocess_data #10928

Conversation

anntzer commented Mar 31, 2018 • edited Loading

PR Summary

PR Checklist

tacaswell commented Mar 31, 2018

tacaswell commented Mar 31, 2018

tacaswell commented Mar 31, 2018

tacaswell left a comment

Choose a reason for hiding this comment

anntzer commented Mar 31, 2018

anntzer commented Mar 31, 2018

anntzer commented Mar 31, 2018

timhoffm commented Jun 6, 2018

anntzer commented Jun 7, 2018

timhoffm Jun 7, 2018

Choose a reason for hiding this comment

anntzer Jun 7, 2018

Choose a reason for hiding this comment

jklymak commented Jul 9, 2018

anntzer Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

anntzer Sep 10, 2018

Choose a reason for hiding this comment

anntzer commented Sep 10, 2018

tacaswell commented Sep 17, 2018

anntzer commented Nov 3, 2018

timhoffm commented Nov 3, 2018

anntzer commented Nov 3, 2018

timhoffm commented Nov 3, 2018

anntzer commented Nov 8, 2018

tacaswell commented Dec 24, 2018

anntzer commented Mar 31, 2018 •

edited

Loading

anntzer Sep 10, 2018 •

edited

Loading