-
Notifications
You must be signed in to change notification settings - Fork 44
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.
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
Add New Bitwise Int Operators #417
base: main
Are you sure you want to change the base?
Conversation
Initial implementation of three bitwise operations along with their test cases. All the operators are implemented in a single file, haven't added the operators to either eventset or global operators list.
All the operations are added to the event_set_ops. Bitwise operation with scalar values are not added.
Scalar implementation and test added for all the three operators. Modified event_set_ops to make sure everything gets routed properly
Both Scalar and Shift operations between EventSets added.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I left you some small requests
@@ -556,34 +556,24 @@ def __neg__(self: EventSetOrNode) -> EventSetOrNode: | |||
return multiply_scalar(input=self, value=-1) | |||
|
|||
def __invert__(self: EventSetOrNode) -> EventSetOrNode: | |||
"""Inverts a boolean [`EventSet`][temporian.EventSet] element-wise. | |||
"""Inverts a boolean or integer [`EventSet`][temporian.EventSet] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should overload the ~
operator to work with booleans and integers, I would prefer to have a different invert_bitwise
method as we discussed on the discord
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should bitwise_invert support boolean or just integer types? If it has to support both boolean and integer, we end up with the same code again.
) | ||
assertOperatorResult( | ||
self, right_shift(self.evset_1, self.evset_2), expected | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add one test with incorrect dtypes expecting an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add one test with int32 and int64, the return type should be int64
make sure to include numbers bigger than int32 as done here: https://github.com/google/temporian/blob/main/temporian/core/operators/test/test_cast.py#L24
Closes #346. Bitwise OR, Bitwise AND, Bitwise XOR, Left Shift, Right Shift, and Bitwise Invert operators added.