COMPAT: Add keepdims and friends to validation #24356
Conversation
TomAugspurger
added
the
Compat
|
Hello @TomAugspurger! Thanks for submitting the PR.
|
| @@ -225,16 +226,32 @@ def validate_cum_func_with_skipna(skipna, args, kwargs, name): | |||
| STAT_FUNC_DEFAULTS = OrderedDict() | |||
| STAT_FUNC_DEFAULTS['dtype'] = None | |||
| STAT_FUNC_DEFAULTS['out'] = None | |||
|
|
|||
| PROD_DEFAULTS = SUM_DEFAULTS = STAT_FUNC_DEFAULTS.copy() | |||
There was a problem hiding this comment.
This is a bit complicated to get the order of the keywords correct.
Codecov Report
@@ Coverage Diff @@
## master #24356 +/- ##
===========================================
- Coverage 92.29% 43.01% -49.29%
===========================================
Files 162 162
Lines 51808 51825 +17
===========================================
- Hits 47817 22290 -25527
- Misses 3991 29535 +25544
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24356 +/- ##
==========================================
+ Coverage 92.29% 92.29% +<.01%
==========================================
Files 162 162
Lines 51808 51825 +17
==========================================
+ Hits 47817 47833 +16
- Misses 3991 3992 +1
Continue to review full report at Codecov.
|
| if name == 'sum': | ||
| nv.validate_sum(tuple(), kwargs) | ||
| elif name == 'prod': | ||
| nv.validate_prod(tuple(), kwargs) |
There was a problem hiding this comment.
can we make this more generic? IOW have nv.validate_statu_func dispatch based on fname?
There was a problem hiding this comment.
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 function.py, and we choose the right one to call.
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.
ok but this seems very hacky to hardcore when we already know the name
There was a problem hiding this comment.
But I think that's a decent sized refactor of how things are done now.
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 numpy function.
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.
ok for now. let's followup and see if can reduce need to specify names like this.
|
Is this OK to go? It'll let me use our regular validators in #24227 |
|
thanks @TomAugspurger |
xref #24227