Skip to content

CLN: remove unneeded try/except in nanops #52684

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 17, 2023
Merged

CLN: remove unneeded try/except in nanops #52684

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c807f8e
CLN: remove unneded try/except in nanops
topper-123 Apr 15, 2023
dc4d674
clean up min/max empty cases
topper-123 Apr 16, 2023
dadbf8b
remove return type
topper-123 Apr 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions pandas/core/nanops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,22 +1067,14 @@ def reduction(
axis: AxisInt | None = None,
skipna: bool = True,
mask: npt.NDArray[np.bool_] | None = None,
) -> Dtype:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep this type annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we have tests that get here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Thanks for the explanation


values, mask = _get_values(
values, skipna, fill_value_typ=fill_value_typ, mask=mask
)

if (axis is not None and values.shape[axis] == 0) or values.size == 0:
dtype_max = _get_dtype_max(dtype)
try:
result = getattr(values, meth)(axis, dtype=dtype_max)
result.fill(np.nan)
except (AttributeError, TypeError, ValueError):
result = np.nan
else:
result = getattr(values, meth)(axis)

result = getattr(values, meth)(axis)
result = _maybe_null_out(result, axis, mask, values.shape)
return result

Expand Down