Skip to content

REGR: Series.__mod__ behaves different with numexpr #36552

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 9 commits into from
Sep 30, 2020
Merged

REGR: Series.__mod__ behaves different with numexpr #36552

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
41bb091
REGR: Series.__mod__ behaves different with numexpr
simonjayhawkins Sep 22, 2020
14e8b92
Merge remote-tracking branch 'upstream/master' into numexpr-regr
simonjayhawkins Sep 23, 2020
1e2a086
add test
simonjayhawkins Sep 23, 2020
4527d23
release note
simonjayhawkins Sep 23, 2020
ccc7eb6
Merge remote-tracking branch 'upstream/master' into numexpr-regr
simonjayhawkins Sep 25, 2020
85db057
compare result element-wise with Python
simonjayhawkins Sep 25, 2020
ff9e85b
fix test
simonjayhawkins Sep 25, 2020
b904d44
Merge remote-tracking branch 'upstream/master' into numexpr-regr
simonjayhawkins Sep 30, 2020
4f9b91d
use tm.assert_equal
simonjayhawkins Sep 30, 2020
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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.1.3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Fixed regressions
- Fixed regression in :class:`IntegerArray` unary plus and minus operations raising a ``TypeError`` (:issue:`36063`)
- Fixed regression in :meth:`Series.__getitem__` incorrectly raising when the input was a tuple (:issue:`35534`)
- Fixed regression in :meth:`Series.__getitem__` incorrectly raising when the input was a frozenset (:issue:`35747`)
- Fixed regression in modulo of :class:`Index`, :class:`Series` and :class:`DataFrame` using ``numexpr`` using C not Python semantics (:issue:`36047`, :issue:`36526`)
- Fixed regression in :meth:`read_excel` with ``engine="odf"`` caused ``UnboundLocalError`` in some cases where cells had nested child nodes (:issue:`36122`, :issue:`35802`)
- Fixed regression in :class:`DataFrame` and :class:`Series` comparisons between numeric arrays and strings (:issue:`35700`, :issue:`36377`)
- Fixed regression when setting empty :class:`DataFrame` column to a :class:`Series` in preserving name of index in frame (:issue:`36527`)
Expand Down
5 changes: 4 additions & 1 deletion pandas/core/computation/expressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,10 @@ def _evaluate_numexpr(op, op_str, a, b):
roperator.rtruediv: "/",
operator.floordiv: "//",
roperator.rfloordiv: "//",
operator.mod: "%",
# we require Python semantics for mod of negative for backwards compatibility
# see https://github.com/pydata/numexpr/issues/365
# so sticking with unaccelerated for now
operator.mod: None,
roperator.rmod: "%",
operator.pow: "**",
roperator.rpow: "**",
Expand Down
2 changes: 0 additions & 2 deletions pandas/core/ops/methods.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,6 @@ def _create_methods(cls, arith_method, comp_method, bool_method, special):
mul=arith_method(cls, operator.mul, special),
truediv=arith_method(cls, operator.truediv, special),
floordiv=arith_method(cls, operator.floordiv, special),
# Causes a floating point exception in the tests when numexpr enabled,
# so for now no speedup
Copy link
Member Author

Choose a reason for hiding this comment

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

This message should have been moved in #19649

I think Ok if we have not had reports of floating point exceptions since enabled.

mod=arith_method(cls, operator.mod, special),
pow=arith_method(cls, operator.pow, special),
# not entirely sure why this is necessary, but previously was included
Expand Down
32 changes: 31 additions & 1 deletion pandas/tests/test_expressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import pytest

import pandas._testing as tm
from pandas.core.api import DataFrame
from pandas.core.api import DataFrame, Index, Series
from pandas.core.computation import expressions as expr

_frame = DataFrame(randn(10000, 4), columns=list("ABCD"), dtype="float64")
Expand Down Expand Up @@ -380,3 +380,33 @@ def test_frame_series_axis(self, axis, arith):

result = op_func(other, axis=axis)
tm.assert_frame_equal(expected, result)

@pytest.mark.parametrize(
"op",
[
"__mod__",
pytest.param("__rmod__", marks=pytest.mark.xfail(reason="GH-36552")),
"__floordiv__",
"__rfloordiv__",
],
)
@pytest.mark.parametrize(
"box, tester",
[
(DataFrame, tm.assert_frame_equal),
(Series, tm.assert_series_equal),
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to usually add the tester as assert_equal handles this

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. a bit belt and braces. will change so we re-run ci b4 merge.

(Index, tm.assert_index_equal),
],
)
@pytest.mark.parametrize("scalar", [-5, 5])
def test_python_semantics_with_numexpr_installed(self, op, box, tester, scalar):
# https://github.com/pandas-dev/pandas/issues/36047
expr._MIN_ELEMENTS = 0
Copy link
Member

Choose a reason for hiding this comment

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

I see it is done like that in other tests as well (so doesn't need to be fixed here), but that doesn't seem a very clean way to patch this, as it will influence other tests as well

Copy link
Member Author

Choose a reason for hiding this comment

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

this is an class with a teardown_method that resets _MIN_ELEMENTS

Copy link
Member Author

Choose a reason for hiding this comment

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

but yes we should replace with a fixture.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, didn't see the teardown/setup, only the import at the top ..

data = np.arange(-50, 50)
obj = box(data)
method = getattr(obj, op)
result = method(scalar)
expr.set_use_numexpr(False)
expected = method(scalar)
expr.set_use_numexpr(True)
tester(result, expected)
Copy link
Member

Choose a reason for hiding this comment

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

Can you also test against a manually constructed expected result?
(or do you know if we have elsewhere a test that covers this behaviour with negative numbers for modulo ?)

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll look though the tests again to see what we already have.

the method i used here is inspired by run_arithmetic, run_binary and also run_frame

I kept the object sizes small here so could do elementwise comparion similar to code in #36552 (comment) wdyt?

Copy link
Member

@jorisvandenbossche jorisvandenbossche Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's certainly good to test the equivalency of numpy vs numexpr behaviour. But here we discovered a specific case where they are not equal, and I think it is useful to also (in addition, can also be a separate test) test this corner case of negative values with modulo.
We might change the implementation in the future (eg assume we would take on numexpr as required dependency and always use it), and then we wouldn't necessarily catch this case.

The object size shouldn't matter since you did expr._MIN_ELEMENTS = 0? (so it's fine to use a small example for which it is easy to manually create the expected result?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The object size shouldn't matter

I meant iterating through in Python was not a resource issue.

I have added an element-wise comparison but tests are failing at the moment

>>> pd.__version__
'1.2.0.dev0+499.g3b12293cea'
>>>
>>> 5 // 0
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ZeroDivisionError: integer division or modulo by zero
>>>
>>> 5 // np.array([0, 0])[0]
<stdin>:1: RuntimeWarning: divide by zero encountered in long_scalars
0
>>>
>>> 5 // np.array(0)
<stdin>:1: RuntimeWarning: divide by zero encountered in floor_divide
0
>>>
>>> 5 // np.array([-1, 0, 1])
array([-5,  0,  5], dtype=int32)
>>>
>>> 5 // pd.Series([-1, 0, 1])
0   -5.0
1    inf
2    5.0
dtype: float64
>>>
>>> -5 // np.array([-1, 0, 1])
array([ 5,  0, -5], dtype=int32)
>>>
>>> -5 // pd.Series([-1, 0, 1])
0    5.0
1   -inf
2   -5.0
dtype: float64
>>>

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed test. this discrepancy that has surfaced is not related to the regression issue, will investigate further.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think pandas is doing the right thing here? other than returning a non-integer result for floordiv. maybe should return IntegerArray in future

>>> 5 / np.array([-1, 0, 1])
<stdin>:1: RuntimeWarning: divide by zero encountered in true_divide
array([-5., inf,  5.])
>>>
>>> 5 / pd.Series([-1, 0, 1])
0   -5.0
1    inf
2    5.0
dtype: float64
>>>

Can you also test against a manually constructed expected result?

still outstanding