Skip to content

BUG: _can_use_numexpr fails when passed large Series #27773

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 4 commits into from
Aug 19, 2019
Merged

BUG: _can_use_numexpr fails when passed large Series #27773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a23578
BUG: _can_use_numexpr did not handle Series case correctly
Aug 6, 2019
6d212c7
Handle Series case correctly
ccharlesgb Aug 12, 2019
e16e893
Refactored so Series case uses dtype attribute
ccharlesgb Aug 14, 2019
0100aba
Reformatted code
ccharlesgb Aug 14, 2019
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
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v0.25.1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ Numeric
^^^^^^^
- Bug in :meth:`Series.interpolate` when using a timezone aware :class:`DatetimeIndex` (:issue:`27548`)
- Bug when printing negative floating point complex numbers would raise an ``IndexError`` (:issue:`27484`)
-
- Bug where :class:`DataFrame` arithmetic operators such as :meth:`DataFrame.mul` with a :class:`Series` with axis=1 would raise an ``AttributeError`` on :class:`DataFrame` larger than the minimum threshold to invoke numexpr (:issue:`27636`)
-

Conversion
Expand Down
7 changes: 4 additions & 3 deletions pandas/core/computation/expressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,16 +76,17 @@ def _can_use_numexpr(op, op_str, a, b, dtype_check):

# required min elements (otherwise we are adding overhead)
if np.prod(a.shape) > _MIN_ELEMENTS:

# check for dtype compatibility
dtypes = set()
for o in [a, b]:
if hasattr(o, "dtypes"):
# Series implements dtypes, check for dimension count as well
if hasattr(o, "dtypes") and o.ndim > 1:
s = o.dtypes.value_counts()
if len(s) > 1:
return False
dtypes |= set(s.index.astype(str))
elif isinstance(o, np.ndarray):
# ndarray and Series Case
elif hasattr(o, "dtype"):
dtypes |= {o.dtype.name}

# allowed are a superset
Expand Down
29 changes: 27 additions & 2 deletions pandas/tests/test_expressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def run_arithmetic(self, df, other, assert_func, check_dtype=False, test_flex=Tr
operator_name = "truediv"

if test_flex:
op = lambda x, y: getattr(df, arith)(y)
op = lambda x, y: getattr(x, arith)(y)
op.__name__ = arith
else:
op = getattr(operator, operator_name)
Expand Down Expand Up @@ -318,7 +318,6 @@ def testit():
for f in [self.frame, self.frame2, self.mixed, self.mixed2]:

for cond in [True, False]:

c = np.empty(f.shape, dtype=np.bool_)
c.fill(cond)
result = expr.where(c, f.values, f.values + 1)
Expand Down Expand Up @@ -431,3 +430,29 @@ def test_bool_ops_column_name_dtype(self, test_input, expected):
# GH 22383 - .ne fails if columns containing column name 'dtype'
result = test_input.loc[:, ["a", "dtype"]].ne(test_input.loc[:, ["a", "dtype"]])
assert_frame_equal(result, expected)

@pytest.mark.parametrize(
"arith", ("add", "sub", "mul", "mod", "truediv", "floordiv")
)
@pytest.mark.parametrize("axis", (0, 1))
def test_frame_series_axis(self, axis, arith):
# GH#26736 Dataframe.floordiv(Series, axis=1) fails
if axis == 1 and arith == "floordiv":
pytest.xfail("'floordiv' does not succeed with axis=1 #27636")

df = self.frame
if axis == 1:
other = self.frame.iloc[0, :]
else:
other = self.frame.iloc[:, 0]

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.

What does this do?

Copy link
Contributor Author

@ccharlesgb ccharlesgb Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There is an overhead in using numexpr for computations that makes it not worth using for a low number of computations. The values of this defaults to: 10,000. In this test suite we are trying to verify that numexpr is being invoked correctly however the function _can_use_numexpr checks that the objects operated on are sufficient size:

def _can_use_numexpr(op, op_str, a, b, dtype_check):
    """ return a boolean if we WILL be using numexpr """
    if op_str is not None:
        # required min elements (otherwise we are adding overhead)
        if np.prod(a.shape) > _MIN_ELEMENTS:
            # further dtype checks to check compatibility may return True
    return False

We can therefore run the test suite exclusively on objects where the number of elements > 10,000 or set the min elements to zero to always try and use numexpr.

This is what is meant by a "large DataFrame". Anything where the size of the objects is large enough to warrant using numexpr fails because of the regression, anything that is smaller than the threshold will succeed because numexpr evaluation will not even be considered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm is affecting global state, right? I worry about this leaking into other tests...

Can you use the monkeypatch fixture, and do this setting with (IIRC) monkeypatch.setattr? That way it'll be undone when the test exits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is but this is handled in the test classes teardown method on line 57 so the global state won't be affected permanently. I can move my test out of the class and handle this with monkeypatch or we can keep the setup/teardown framework already implemented in the TestExpressions class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I wasn't familiar with these tests.


op_func = getattr(df, arith)

expr.set_use_numexpr(False)
expected = op_func(other, axis=axis)
expr.set_use_numexpr(True)

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