TheNeuralBit commented on a change in pull request #15734:
URL: https://github.com/apache/beam/pull/15734#discussion_r730122646
##########
File path: sdks/python/apache_beam/dataframe/frames_test.py
##########
@@ -187,7 +187,7 @@ def _run_test(
else:
cmp = lambda x: np.isclose(expected, x)
else:
- cmp = expected.__eq__
+ cmp = lambda x: x == expected
Review comment:
Hm yeah I just verified this manually, I didn't try to understand _why_
it works. The issue in https://github.com/apache/beam/pull/15729 seems to be
that `actual` is an `np.bool_` and `expected` is a `bool`.
```py
In [3]: np.bool_(True).__eq__(True)
Out[3]: True
In [4]: True.__eq__(np.bool_(True))
Out[4]: NotImplemented
```
`NotImplemented` is documented
[`here`](https://docs.python.org/3/library/constants.html#NotImplemented), it
says:
> Note: When a binary (or in-place) method returns NotImplemented the
interpreter will try the reflected operation on the other type (or some other
fallback, depending on the operator). If all attempts return NotImplemented,
the interpreter will raise an appropriate exception. Incorrectly returning
NotImplemented will result in a misleading error message or the NotImplemented
value being returned to Python code.
So it seems we got to the error case by cutting out the interpreter and
calling `__eq__` directly - it doesn't have a chance to try the reflected
operation. Given all that, I think `x == expected` is the appropriate thing to
do here (but thank you for encouraging me to investigatge!)
##########
File path: sdks/python/apache_beam/dataframe/frames_test.py
##########
@@ -187,7 +187,7 @@ def _run_test(
else:
cmp = lambda x: np.isclose(expected, x)
else:
- cmp = expected.__eq__
+ cmp = lambda x: x == expected
Review comment:
Hm yeah I just verified this manually, I didn't try to understand _why_
it works. The issue in https://github.com/apache/beam/pull/15729 seems to be
that `actual` is an `np.bool_` and `expected` is a `bool`.
```py
In [3]: np.bool_(True).__eq__(True)
Out[3]: True
In [4]: True.__eq__(np.bool_(True))
Out[4]: NotImplemented
```
`NotImplemented` is documented
[`here`](https://docs.python.org/3/library/constants.html#NotImplemented), it
says:
> Note: When a binary (or in-place) method returns NotImplemented the
interpreter will try the reflected operation on the other type (or some other
fallback, depending on the operator). If all attempts return NotImplemented,
the interpreter will raise an appropriate exception. Incorrectly returning
NotImplemented will result in a misleading error message or the NotImplemented
value being returned to Python code.
So it seems we got to the error case by cutting out the interpreter and
calling `__eq__` directly - it doesn't have a chance to try the reflected
operation. Given all that, I think `x == expected` is the appropriate thing to
do here (but thank you for encouraging me to investigate!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
