Raise on __eq__ binary operations between unsupported types.#11609
Raise on __eq__ binary operations between unsupported types.#11609bdice wants to merge 4 commits intorapidsai:branch-24.06from
Conversation
| if op == "__eq__": | ||
| raise TypeError( | ||
| "'==' not supported between instances of " | ||
| f"'{type(self).__name__}' and '{type(other).__name__}'" | ||
| ) | ||
| if op == "__ne__": | ||
| raise TypeError( | ||
| "'!=' not supported between instances of " | ||
| f"'{type(self).__name__}' and '{type(other).__name__}'" | ||
| ) | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Would it be cleaner to leave BinaryOperand._binaryop as-is, and just patch the __eq__ and __ne__ methods? Or do we need to change all three methods?
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-23.02 #11609 +/- ##
===============================================
Coverage ? 88.21%
===============================================
Files ? 137
Lines ? 22592
Branches ? 0
===============================================
Hits ? 19930
Misses ? 2662
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
This PR has been labeled |
|
@bdice now that we have full nested type support is this PR still relevant? |
|
This is still an issue -- the wrong error is raised when comparing nested types. >>> cudf.Series([[1, 2, 3], [4, 5]]) == cudf.Series([{"a": 1}, {"a": 2}])
...
TypeError: object of type 'bool' has no len()
>>> pd.Series([[1, 2, 3], [4, 5]]) == pd.Series([{"a": 1}, {"a": 2}])
0 False
1 False
dtype: boolThis is also a problem for non-nested equality comparisons: >>> cudf.Series(["a", "b"]) == cudf.Series([1, 2])
...
TypeError: object of type 'bool' has no len()
>>> pd.Series(["a", "b"]) == pd.Series([1, 2])
0 False
1 False
dtype: bool |
|
I'm going to try and refactor cudf/cpp/src/binaryop/compiled/binary_ops.cu Line 399 in b8503bc cudf::binary_operation that works with lists and structs. That should help unblock the lists/structs cases in this PR.
|
|
A lot of code has been shuffled around since this PR was opened, so at this point it will probably be easier to recreate rather than rebasing. I've opened #18860 to track this issue for future work. |
3 participants
Description
This PR fixes a bug where calling
__eq__/==binary operations in unsupported cases (Series of lists, Series of structs -- soon to be supported) called through toobject.__eq__instead of raising as expected, resulting in a cryptic errorTypeError: object of type 'bool' has no len(). In this PR, we alter theBinaryOperandclass to have a default__eq__method (actually a descriptor) that is defined as a mixinOperation, so that classes inheriting from this mixin can override its behavior.To-do:
==raises when called with Series of lists and Series of structs (not yet supported in Python)cudf.Series(["a", "b"]) == cudf.Series([1, 2])raises the same errorChecklist