REF: move check for disallowed bool arithmetic ops out of numexpr-related expressions.py #41161
Conversation
…ated expressions.py
jorisvandenbossche
added
Numeric Operations
| with tm.assert_produces_warning(UserWarning): | ||
| # "evaluating in Python space because ..." | ||
| op(s, e.value) | ||
| op(s, e.value) |
There was a problem hiding this comment.
This was a wrong test, but because putting assert_produces_warning inside pytest.raises doesn't fail if it doesn't warn, this wasn't noticed (encountered this in #40325 (comment) as well).
The ops for which we raise a warning vs raise NotImplementedError are not overlapping.
| # numexpr is used or not | ||
| # with tm.assert_produces_warning(UserWarning): | ||
| # # "evaluating in Python space because ..." | ||
| op(s, e.value) |
There was a problem hiding this comment.
The fact that it is no longer warning is I think correct, since we are not using numexpr for such a small test case (so the warning about "not supported by numexpr" is not relevant for the user).
But, we should still test that we do raise the warning when numexpr is used. This can probably be done in a similar was as #40463
There was a problem hiding this comment.
Will take care of this in #41178
| @@ -205,6 +205,10 @@ def arithmetic_op(left: ArrayLike, right: Any, op): | |||
| # Timedelta is included because numexpr will fail on it, see GH#31457 | |||
| res_values = op(left, right) | |||
| else: | |||
| # TODO we should handle EAs consistently and move this check before the if/else | |||
| # (https://github.com/pandas-dev/pandas/issues/41165) | |||
| _bool_arith_check(op, left, right) | |||
There was a problem hiding this comment.
Opened an issue for this: #41165
|
looks ok, can you rebase as merged the numexpr test updates. |
…ated expressions.py (pandas-dev#41161)
…ated expressions.py (pandas-dev#41161)
The current
_bool_arith_checkhelper function inexpressions.pydoes 2 things: 1) check for operations we do not allow in pandas for boolean data (division, power) and 2) check for ops that are not supported by numexpr but have a pure python fallback.That first aspect is not related to numexpr, though: we always check this (and raise an error if appropriate), whether numexpr is used or not.
So this PR does:
array_ops.py. This is a cleaner separation of concerns, and ensures that the check is done consistently (eg if not calling floordiv through numexpr (PERF: numexpr doesn't support floordiv, so don't try #40727), or if in the future not calling expressions.py if we know we can't use numexpr, such as in PERF/REF: Check use of numexpr earlier in the DataFrame operation #41122)expressions.py, move the remaining check for unsupported ops (by numexpr) inside an except block: this way, we don't check up-front (for all ops, for all types), and only do the types check if there was actually a NotImplementedError. That avoids paying the overhead costs for all other ops.