Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Simplify (quite a bit...) _preprocess_data#10928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
2a2e584 toebcc968Comparetacaswell commentedMar 31, 2018
I would rather do this, it seems like an oversight that we did not do this for plot and friends originally. |
tacaswell commentedMar 31, 2018
This is easier to review withhttps://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 commentedMar 31, 2018
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- Document API change for return types
- Update the docstring and signature of plot and friends
Anyone can dismiss this review.
anntzer commentedMar 31, 2018
The simplications come from
|
anntzer commentedMar 31, 2018
Question: Currently, works as expected but crashes ("unrecognized character f in format string"). This PR makes both forms "work", although I'm tempted to makeneither work instead (I think the shorthand format string should, well, be a shorthand format string). Thoughts? |
anntzer commentedMar 31, 2018
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 to5868dd9Compare5b24966 to267623cComparetimhoffm commentedJun 6, 2018
anntzer commentedJun 7, 2018
rebased |
| # Uses a custom implementation of data-kwarg handling in | ||
| # _process_plot_var_args. | ||
| deffill(self,*args,**kwargs): | ||
| deffill(self,*args,data=None,**kwargs): |
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.
data should be added to the Parameters section in the docstring
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.
done
jklymak commentedJul 9, 2018
| withpytest.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 supportplot() andfill() which don't havey explicitly in the signature but always have ay 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") | ||
| deffunc_varags_replace_all(ax,*args,**kwargs): |
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.
See above re: "implicit" label_namer. Otherwise this test function is redundant with func_replace_all just above.
anntzer commentedSep 10, 2018
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 to139d156Comparetacaswell commentedSep 17, 2018
Sold on this in general and on making |
anntzer commentedNov 3, 2018
rebased |
timhoffm commentedNov 3, 2018
I'm a bit hesitant about this.
|
anntzer commentedNov 3, 2018
But the inner function actuallydoesn'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). |
timhoffm commentedNov 3, 2018
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` aslabel_namer. This is consistent with other functions that wrap `plot`(`plot` itself, `loglog`, etc.). (Alternatively, we could make allthese 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 forgeneral 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 wasno real use of "replace all known args but not unknown ones" (even ifthere was, this can easily be handled by explicitly listing the args inreplace_names). `test_function_call_with_replace_all_args` was removedas a consequence.`replace_names` no longer complains if some argument names it is givenare not present in the "explicit" signature, as long as the functionaccepts `**kwargs` -- because it may find the arguments in kwargsinstead.label_namer no longer triggers if `data` is not passed (if theargument specified by label_namer was a string, then it islikely a categorical and shouldn't be considered as a labelanyways). `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 somecases of mismatched arguments; they now trigger TypeError similarly tohow normal functions do (`test_more_args_than_pos_parameters`).
Assert that label_namer (if given) is a parameter in the functionsignature (not "possibly" present via `**kwargs`). This seems moreconsistent with the expectations of the check later; in fact we cannow just remove that check.
We only support passing them positionally; right now passing them askwargs silently does nothing. The error message is the standard one forunknown kwargs.
anntzer commentedNov 8, 2018
Added a commit to make plot() error out when x/y are passed as kwargs (this silently does nothing currently, xref#12106). |
tacaswell commentedDec 24, 2018
Thanks@anntzer ! Sorry for the slow review on this. |
Uh oh!
There was an error while loading.Please reload this page.
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