Skip to content

BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp #25928

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 5 commits into from
Apr 12, 2019
Merged

BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp #25928

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef777e7
BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp
alexcwatt Mar 30, 2019
5242bef
Fix test and add note to whatsnew
alexcwatt Mar 30, 2019
f0e0b3d
Attempt to fix test style
alexcwatt Mar 30, 2019
5d6ea06
Update whatsnew and add another test case
alexcwatt Mar 30, 2019
1b88337
Use parametrize to test both float32 and float64
alexcwatt Mar 30, 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
6 changes: 4 additions & 2 deletions pandas/core/computation/expr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,11 +418,13 @@ def _maybe_transform_eq_ne(self, node, left=None, right=None):

def _maybe_downcast_constants(self, left, right):
f32 = np.dtype(np.float32)
if left.is_scalar and not right.is_scalar and right.return_type == f32:
if (left.is_scalar and hasattr(left, 'value') and
not right.is_scalar and right.return_type == f32):
# right is a float32 array, left is a scalar
name = self.env.add_tmp(np.float32(left.value))
left = self.term_type(name, self.env)
if right.is_scalar and not left.is_scalar and left.return_type == f32:
if (right.is_scalar and hasattr(right, 'value') and
not left.is_scalar and left.return_type == f32):
# left is a float32 array, right is a scalar
name = self.env.add_tmp(np.float32(right.value))
right = self.term_type(name, self.env)
Expand Down
6 changes: 6 additions & 0 deletions pandas/tests/computation/test_eval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,12 @@ def test_unary_in_array(self):
-False, False, ~False, +False,
-37, 37, ~37, +37], dtype=np.object_))

def test_float_comparison_bin_op(self):
# GH 16363
df = pd.DataFrame({'x': np.array([0], dtype=np.float32)})
Copy link
Member

Choose a reason for hiding this comment

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

Is this specific to float32? Read briefly through issue and seemed like it affected other things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WillAyd The bug arises when one side returns a float32 and the other side is a scalar that doesn't have a 'value' attribute.

Copy link
Member

Choose a reason for hiding this comment

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

Does this still work if the operands are switched? i.e. -0.1 > x?

Copy link
Contributor

Choose a reason for hiding this comment

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

an you parameterize on the dtype (at least float32/float64)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this still work if the operands are switched? i.e. -0.1 > x?

I will add a test with a negative value on the left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an you parameterize on the dtype (at least float32/float64)

Would you like a test for float64? This particular bug was caused by code that handles float32's, so float64's were fine, but I can add a test to ensure that this does not become an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback Oh, I see now pytest's parametrize in some of the other tests, I will update this

res = df.eval('x < -0.1')
assert np.array_equal(res, np.array([False])), res
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for , res here? I think also causing CI failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, removing this.


def test_disallow_scalar_bool_ops(self):
exprs = '1 or 2', '1 and 2'
exprs += 'a and b', 'a or b'
Expand Down