-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
COMPAT: Add keepdims and friends to validation #24356
Changes from all commits
daab462
0b8d2b9
82ce910
db29098
ed5308f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10834,7 +10834,12 @@ def _make_min_count_stat_function(cls, name, name1, name2, axis_descr, desc, | |
def stat_func(self, axis=None, skipna=None, level=None, numeric_only=None, | ||
min_count=0, | ||
**kwargs): | ||
nv.validate_stat_func(tuple(), kwargs, fname=name) | ||
if name == 'sum': | ||
nv.validate_sum(tuple(), kwargs) | ||
elif name == 'prod': | ||
nv.validate_prod(tuple(), kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we make this more generic? IOW have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would make sense, but it isn't really written that way right now. Right now we just have instances of CompatValidator sitting in It'd be nice to have a decorator that did it for us @validate_numpy
def mean(self, ...):
... then we don't have the duplication of the function name. But I think that's a decent sized refactor of how things are done now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok but this seems very hacky to hardcore when we already know the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed. At the time, I wrote it that way to keep things explicit, and it made it slightly easier to handle the details of each analogous That being said, I think refactoring to dispatching seems reasonable as well but would be best served for investigation and execution in a follow-up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok for now. let's followup and see if can reduce need to specify names like this. |
||
else: | ||
nv.validate_stat_func(tuple(), kwargs, fname=name) | ||
if skipna is None: | ||
skipna = True | ||
if axis is None: | ||
|
@@ -10855,7 +10860,10 @@ def _make_stat_function(cls, name, name1, name2, axis_descr, desc, f, | |
@Appender(_num_doc) | ||
def stat_func(self, axis=None, skipna=None, level=None, numeric_only=None, | ||
**kwargs): | ||
nv.validate_stat_func(tuple(), kwargs, fname=name) | ||
if name == 'median': | ||
nv.validate_median(tuple(), kwargs) | ||
else: | ||
nv.validate_stat_func(tuple(), kwargs, fname=name) | ||
if skipna is None: | ||
skipna = True | ||
if axis is None: | ||
|
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.
This is a bit complicated to get the order of the keywords correct.