CLN: remove unneeded try/except in nanops #52684
Conversation
pandas/core/nanops.py
Outdated
| except (AttributeError, TypeError, ValueError): | ||
| result = np.nan | ||
| result = getattr(values, meth)(axis, dtype=dtype_max) | ||
| result.fill(np.nan) |
There was a problem hiding this comment.
do we have tests that get here?
topper-123
force-pushed
the
cln_nanminmax
branch
from
d47de45 to
ea64bdf
Compare
topper-123
force-pushed
the
cln_nanminmax
branch
from
ea64bdf to
dc4d674
Compare
|
I rearranged it again. So I've just added a |
| @@ -1067,22 +1067,14 @@ def reduction( | |||
| axis: AxisInt | None = None, | |||
| skipna: bool = True, | |||
| mask: npt.NDArray[np.bool_] | None = None, | |||
| ) -> Dtype: | |||
There was a problem hiding this comment.
Can we keep this type annotation?
There was a problem hiding this comment.
This annotation was wrong and this doesn't return a Dtype but the the result of a min/max of a DataFrame/Series, i.e. typically a scalar or a ndarray (but the min/max of a Series can in principle be anything....)
IDK if we return type Scalar as annotation for Series?
There was a problem hiding this comment.
Yeah not either, but since this is an inner function the typing here probably doesn't matter as much
| dtype = values.dtype | ||
| ): | ||
| if values.size == 0: | ||
| return _na_for_min_count(values, axis) |
There was a problem hiding this comment.
I assume we have tests that get here?
There was a problem hiding this comment.
The values.size == 0 case is actually never reached in the current code:
You can see this function is wrapped in bottleneck_switch and if you go to bottleneck_switch you can see (line 125) it takes care of the values.size == 0 case, also when use_bottleneck = False (kwds.get("min_count") is None is always true for _nanminmax).
I just think this is more readable than the old code.
There was a problem hiding this comment.
Gotcha. Thanks for the explanation
|
Thanks @topper-123 |
Look like this try/except isn't needed, checking in CI if it verifies the result.