Skip to content

REF/PERF: move np.errstate out of core array_ops up to higher level #40396

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
Mar 17, 2021
Merged

REF/PERF: move np.errstate out of core array_ops up to higher level #40396

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48482ba
REF/PERF: move np.errstate out of core array_ops up to higher level
jorisvandenbossche Mar 12, 2021
4f4af54
remove in _combine_frame
jorisvandenbossche Mar 12, 2021
3996d09
Merge remote-tracking branch 'upstream/master' into ops-refactor-seterr
jorisvandenbossche Mar 17, 2021
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
3 changes: 2 additions & 1 deletion pandas/core/arrays/numpy_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,8 @@ def _cmp_method(self, other, op):
other = other._ndarray

pd_op = ops.get_array_op(op)
result = pd_op(self._ndarray, other)
with np.errstate(all="ignore"):
result = pd_op(self._ndarray, other)
Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, we probably don't need to add this one here, since when going through a DataFrame/Series op, this will also already be catched on that level (and be don't consistently catch it in other EA ops). But this keeps current behaviour (and this not used in practice anyway)


if op is divmod or op is ops.rdivmod:
a, b = result
Expand Down
3 changes: 1 addition & 2 deletions pandas/core/computation/expressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ def _evaluate_standard(op, op_str, a, b):
"""
if _TEST_MODE:
_store_test_result(False)
with np.errstate(all="ignore"):
return op(a, b)
return op(a, b)


def _can_use_numexpr(op, op_str, a, b, dtype_check):
Expand Down
36 changes: 20 additions & 16 deletions pandas/core/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6556,7 +6556,8 @@ def _dispatch_frame_op(self, right, func, axis: Optional[int] = None):
right = lib.item_from_zerodim(right)
if not is_list_like(right):
# i.e. scalar, faster than checking np.ndim(right) == 0
bm = self._mgr.apply(array_op, right=right)
with np.errstate(all="ignore"):
Copy link
Member

Choose a reason for hiding this comment

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

could just do the np.errstate once at the top of the method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is possible (as mentioned in #40396 (comment)). Personally, I think I prefer putting it just around the mgr operate call / array_op calls, but in the end it doesn't matter much (it's not that the other checks here could potentially gives such warning that would be incorrectly suppressed). So either way.

Copy link
Member

Choose a reason for hiding this comment

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

im fine with this, we can clean it up later if needbe

bm = self._mgr.apply(array_op, right=right)
return type(self)(bm)

elif isinstance(right, DataFrame):
Expand All @@ -6567,16 +6568,17 @@ def _dispatch_frame_op(self, right, func, axis: Optional[int] = None):
# _frame_arith_method_with_reindex

# TODO operate_blockwise expects a manager of the same type
bm = self._mgr.operate_blockwise(
# error: Argument 1 to "operate_blockwise" of "ArrayManager" has
# incompatible type "Union[ArrayManager, BlockManager]"; expected
# "ArrayManager"
# error: Argument 1 to "operate_blockwise" of "BlockManager" has
# incompatible type "Union[ArrayManager, BlockManager]"; expected
# "BlockManager"
right._mgr, # type: ignore[arg-type]
array_op,
)
with np.errstate(all="ignore"):
bm = self._mgr.operate_blockwise(
# error: Argument 1 to "operate_blockwise" of "ArrayManager" has
# incompatible type "Union[ArrayManager, BlockManager]"; expected
# "ArrayManager"
# error: Argument 1 to "operate_blockwise" of "BlockManager" has
# incompatible type "Union[ArrayManager, BlockManager]"; expected
# "BlockManager"
right._mgr, # type: ignore[arg-type]
array_op,
)
return type(self)(bm)

elif isinstance(right, Series) and axis == 1:
Expand All @@ -6587,16 +6589,18 @@ def _dispatch_frame_op(self, right, func, axis: Optional[int] = None):
# maybe_align_as_frame ensures we do not have an ndarray here
assert not isinstance(right, np.ndarray)

arrays = [
array_op(_left, _right)
for _left, _right in zip(self._iter_column_arrays(), right)
]
with np.errstate(all="ignore"):
arrays = [
array_op(_left, _right)
for _left, _right in zip(self._iter_column_arrays(), right)
]

elif isinstance(right, Series):
assert right.index.equals(self.index) # Handle other cases later
right = right._values

arrays = [array_op(left, right) for left in self._iter_column_arrays()]
with np.errstate(all="ignore"):
arrays = [array_op(left, right) for left in self._iter_column_arrays()]

else:
# Remaining cases have less-obvious dispatch rules
Expand Down
3 changes: 2 additions & 1 deletion pandas/core/ops/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,8 @@ def f(self, other, axis=default_axis, level=None, fill_value=None):

if isinstance(other, ABCDataFrame):
# Another DataFrame
new_data = self._combine_frame(other, na_op, fill_value)
with np.errstate(all="ignore"):
new_data = self._combine_frame(other, na_op, fill_value)
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt we get this one for free bc it goes through _dispatch_frame_op which you've got setting np.errstaet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, indeed _combine_frame eventually always goes through _dispatch_frame as well (I got here because it's a place that uses get_array_op)


elif isinstance(other, ABCSeries):
new_data = self._dispatch_frame_op(other, op, axis=axis)
Expand Down
18 changes: 10 additions & 8 deletions pandas/core/ops/array_ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ def _masked_arith_op(x: np.ndarray, y, op):

# See GH#5284, GH#5035, GH#19448 for historical reference
if mask.any():
with np.errstate(all="ignore"):
result[mask] = op(xrav[mask], yrav[mask])
result[mask] = op(xrav[mask], yrav[mask])

else:
if not is_scalar(y):
Expand All @@ -126,8 +125,7 @@ def _masked_arith_op(x: np.ndarray, y, op):
mask = np.where(y == 1, False, mask)

if mask.any():
with np.errstate(all="ignore"):
result[mask] = op(xrav[mask], y)
result[mask] = op(xrav[mask], y)

result = maybe_upcast_putmask(result, ~mask)
result = result.reshape(x.shape) # 2D compat
Expand Down Expand Up @@ -179,6 +177,9 @@ def arithmetic_op(left: ArrayLike, right: Any, op):
"""
Evaluate an arithmetic operation `+`, `-`, `*`, `/`, `//`, `%`, `**`, ...

Note: the caller is responsible for ensuring that numpy warnings are
suppressed (with np.errstate(all="ignore")) if needed.

Parameters
----------
left : np.ndarray or ExtensionArray
Expand All @@ -204,8 +205,7 @@ def arithmetic_op(left: ArrayLike, right: Any, op):
res_values = op(lvalues, rvalues)

else:
with np.errstate(all="ignore"):
res_values = _na_arithmetic_op(lvalues, rvalues, op)
res_values = _na_arithmetic_op(lvalues, rvalues, op)

return res_values

Expand All @@ -214,6 +214,9 @@ def comparison_op(left: ArrayLike, right: Any, op) -> ArrayLike:
"""
Evaluate a comparison operation `=`, `!=`, `>=`, `>`, `<=`, or `<`.

Note: the caller is responsible for ensuring that numpy warnings are
suppressed (with np.errstate(all="ignore")) if needed.

Parameters
----------
left : np.ndarray or ExtensionArray
Expand Down Expand Up @@ -265,8 +268,7 @@ def comparison_op(left: ArrayLike, right: Any, op) -> ArrayLike:
with warnings.catch_warnings():
# suppress warnings from numpy about element-wise comparison
warnings.simplefilter("ignore", DeprecationWarning)
with np.errstate(all="ignore"):
Copy link
Member

Choose a reason for hiding this comment

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

should the catch_warnings here be done at a higher level too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, but that might need to be done more targeted for the specific type of op (since we don't do anything similar in arithmetic_op), and thus this will be more complicated. So would like to start with np.errstate.

res_values = _na_arithmetic_op(lvalues, rvalues, op, is_cmp=True)
res_values = _na_arithmetic_op(lvalues, rvalues, op, is_cmp=True)

return res_values

Expand Down
6 changes: 4 additions & 2 deletions pandas/core/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5067,7 +5067,8 @@ def _cmp_method(self, other, op):
lvalues = self._values
rvalues = extract_array(other, extract_numpy=True)

res_values = ops.comparison_op(lvalues, rvalues, op)
with np.errstate(all="ignore"):
res_values = ops.comparison_op(lvalues, rvalues, op)

return self._construct_result(res_values, name=res_name)

Expand All @@ -5087,7 +5088,8 @@ def _arith_method(self, other, op):

lvalues = self._values
rvalues = extract_array(other, extract_numpy=True)
result = ops.arithmetic_op(lvalues, rvalues, op)
with np.errstate(all="ignore"):
result = ops.arithmetic_op(lvalues, rvalues, op)
Copy link
Member

Choose a reason for hiding this comment

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

do we not need this for _logical_method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we don't suppress any warnings in ops.logical_op, so I left the current behaviour as is.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense


return self._construct_result(result, name=res_name)

Expand Down