Simplify (quite a bit...) _preprocess_data#10928
Simplify (quite a bit...) _preprocess_data#10928tacaswell merged 5 commits intomatplotlib:masterfrom
Conversation
2a2e584 to
ebcc968
Compare
I would rather do this, it seems like an oversight that we did not do this for plot and friends originally. |
|
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. |
Please document this as an API change. It seems a good fraction of the simplification comes from using The docstring and signature for Over all 👍 ! |
tacaswell
left a comment
There was a problem hiding this comment.
- Document API change for return types
- Update the docstring and signature of plot and friends
Anyone can dismiss this review.
|
The simplications come from
|
|
Question: Currently, works as expected but 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? |
|
The data kwarg actually already has its own documentation in the docstring of 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 |
1bd1f05 to
5868dd9
Compare
5868dd9 to
df26a24
Compare
5b24966 to
267623c
Compare
267623c to
f88f6e8
Compare
|
rebased |
| # 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): |
There was a problem hiding this comment.
data should be added to the Parameters section in the docstring
8024651 to
6758c71
Compare
6758c71 to
9df54cc
Compare
| with pytest.raises(AssertionError): | ||
| _preprocess_data(label_namer="z")(func_args) | ||
|
|
||
| # but "ok-ish", if func has kwargs -> will show up at runtime :-( |
There was a problem hiding this comment.
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).
| "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): |
There was a problem hiding this comment.
See above re: "implicit" label_namer. Otherwise this test function is redundant with func_replace_all just above.
|
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). |
e431d01 to
139d156
Compare
|
Sold on this in general and on making |
139d156 to
dcdea6d
Compare
|
rebased |
I'm a bit hesitant about this.
|
But the inner function actually doesn't take
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). |
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 |
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.
dcdea6d to
b07377f
Compare
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.
|
Added a commit to make plot() error out when x/y are passed as kwargs (this silently does nothing currently, xref #12106). |
|
Thanks @anntzer ! Sorry for the slow review on this. |
PR Summary
Public API change:
stepno longer defaults to usingyaslabel_namer. This is consistent with other functions that wrap
plot(
plotitself,loglog, etc.). (Alternatively, we could make allthese functions use
yas 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 forgeneral callable
positional_parameter_names,_plot_args_replacer, andpositional_parameter_names.test_positional_parameter_names_as_functionand tests usingplot_func_varargswere removed as a consequence.replace_all_argscan be replaced by makingreplace_names=Nonetrigger 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_argswas removedas a consequence.
edit: not anymore, we still error out in that case.replace_namesno longer complains if some argument names it is givenare not present in the "explicit" signature, as long as the function
accepts
**kwargs-- because it may find the arguments in kwargsinstead.
label_namer no longer triggers if
datais not passed (if theargument 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_runtimewas renamed totest_label_namer_only_if_dataand 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