You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/10/17 00:31:59 UTC
[GitHub] [beam] TheNeuralBit commented on a change in pull request #15734: Minor: Fix `frames_test.py` equality check for non-frame outputs
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org