validate min/max axis #23206
validate min/max axis #23206
Conversation
|
Hello @jbrockmendel! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #23206 +/- ##
==========================================
+ Coverage 92.19% 92.19% +<.01%
==========================================
Files 169 169
Lines 50954 50964 +10
==========================================
+ Hits 46975 46985 +10
Misses 3979 3979
Continue to review full report at Codecov.
|
|
this is not correct - why did you merge?? |
|
this is very verbose code and needs to be incorporated to the validation rather than done in the DatetimeArray |
| @@ -479,6 +481,7 @@ def max(self, axis=None, *args, **kwargs): | |||
| numpy.ndarray.max | |||
| """ | |||
| nv.validate_max(args, kwargs) | |||
| nv.validate_minmax_axis(axis) | |||
There was a problem hiding this comment.
these need to be inside these functions
the is so smelly to have to call 2 functions here
pls fix
|
Seems good to me. Shall we revert? |
|
The argument made on the previous PR is that those two validations are not really the same, so they don't necessarily need to live in the same function |
|
i disagree we have a well thought out pattern for handling these validations this should be integral to the current framework rather than simply adding another function call |
|
I think the question is whether you see this as a "numpy compatibility" check, or rather a general keyword arg validation check (as we do in many places in pandas). In the first case you are right this should be integrated, in the second case I think separate is fine, and the function should actually be moved to |
|
this should be part of the existing check the issue as written is that you have to remember to add this in addition. to _validate_min for example this is just cause for bugs laters on as written |
|
@jreback the new axis validation func assumes self.ndim==1, which nothing else in compat.numpy.functions does. Adding that assumption into a subset of that module's functions is much smellier than making an explicit function call in four place.s If we were to combine the axis check into the existing functions, two possible approaches come to mind. The first is to wrap the extant functions and pin the new one to them. You disapproved of this when implemented in #23140. The second approach is to modify the function-generating code and bake in this new check to everything in compat.functions.numpy. Besides not being clear that we want this check in all the other affected functions, these way these functions are generated is opaque to newcomers (yo!) and changing the generating code will end up much more verbose than the 4 lines of function calls in this implementation. |
* validate min/max axis closes pandas-dev#23081 * Fix copy/paste indentation mistake * Fix merge mixup
Broken off from #23140.
axiskwarg #23081git diff upstream/master -u -- "*.py" | flake8 --diff