-
Notifications
You must be signed in to change notification settings - Fork 243
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 bitwise operations for integer types #5149
Conversation
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 gonna be so awesome. Once Daniel and I get an arbitrary cross product working, this plus aggregators will give us a path to very tight representations of variant sample matrix!
|
||
@typecheck(x=expr_oneof(expr_int32, expr_int64), y=expr_int32) | ||
def bit_rshift(x, y): | ||
"""Bitwise right-shift `x` by `y`. |
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 think we should document that this is a sign extending bitwise right shift.
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.
(which is also what python means by >>
, so we're good, I just think we should be explicit)
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.
Moreover, I think python elides a logical right shift operator (called >>>
in Java/Scala) because Python ints are arbitrary width, ergo, it's maybe not clear where the high zeros are showing up. We (hail), have fixed-width integers so a logical right shift operator can be sensibly defined.
Can we add bit_logical_rshift(x, y)
as well? Or add a logical
keyword argument? Or maybe preserve_sign
?
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.
@danking Thanks for doing the work for me ;)
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.
added logical argument and docs
Also, it looks like we didn't define the operator syntax. Sounds like an easy PR to farm out to someone else! |
32 | ||
|
||
Notes | ||
----- |
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.
Do we need two different right shift operations (sign extended version and not sign extended)? Add a note saying which version we've implemented.
hail/python/hail/expr/functions.py
Outdated
|
||
|
||
@typecheck(x=expr_oneof(expr_int32, expr_int64), y=expr_oneof(expr_int32, expr_int64)) | ||
def bit_xor(x, y): |
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 duplicated from above.
|
||
>>> hl.eval(hl.bit_lshift(1, 8)) | ||
256 | ||
|
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.
How does our code behave for the following case? 1 << 32
The Python interpreter is this:
>>> 1 << 32
4294967296
The Scala interpreter is this:
scala> 1 << 32
res6: Int = 1
It looks like Scala wraps the bits being shifted and Python uses a long integer with arbitrary size.
Python:
>>> 1 << 64
18446744073709551616
>>> 1 << 65
36893488147419103232
>>> 1 << 1000
10715086071862673209484250490600018105614048117055336074437503883703510511249361224931983788156958581275946729175531468251871452856923140435984577574698574803934567774824230985421074605062371141877954182153046474983581941267398767559165543946077062914571196477686542167660429831652624386837205668069376
Scala:
scala> 1 << 64
res7: Int = 1
scala> 1 << 65
res8: Int = 2
If we're using Scala bit operations, then we need to make the behavior difference from Python clear in the docs.
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.
Yup, clarified. Good point.
case (_: TInt64, _: TInt64) => | ||
val ll = coerce[Long](l) | ||
|
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.
delete.
@@ -93,6 +110,10 @@ object BinaryOp { | |||
case "*" | "Multiply" => Multiply() | |||
case "/" | "FloatingPointDivide" => FloatingPointDivide() | |||
case "//" | "RoundToNegInfDivide" => RoundToNegInfDivide() | |||
case "|" | "BitOr" => BitOr() | |||
case "&" | "BitAnd" => BitAnd() | |||
case "<<" | "LeftShift" => LeftShift() |
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.
Are you missing BitXOr here?
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.
yep, tests caught it too
assertEvalsTo(ApplyUnaryPrimOp(BitFlip(), i32na), null) | ||
assertEvalsTo(ApplyUnaryPrimOp(BitFlip(), I64(0xdeadbeef12345678L)), ~0xdeadbeef12345678L) | ||
assertEvalsTo(ApplyUnaryPrimOp(BitFlip(), i64na), null) | ||
|
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.
delete.
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), i32na, I32(2)), null) | ||
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), i32na, i32na), null) | ||
|
||
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), I64(5), I32(2)), 5L << 2) |
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.
Can you add tests with more edge cases and negative numbers?
bit_xor | ||
bit_lshift | ||
bit_rshift | ||
bit_flip |
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.
Nice. I think bit_not is more traditional. I also agree we should have logical and arithmetic right shifts.
@danking I intentionally didn't add this yet - there is a case I am worried about:
Right now this is a type error. With bit operators, this is the same as:
|
@tpoterba looks like you've written a very popular PR ;) Strong agree on the operator stuff. Eck, that's really a mess. |
Good comments. Addressed:
|
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), I32(5), i32na), null) | ||
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), i32na, I32(2)), null) | ||
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), i32na, i32na), null) | ||
|
||
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), I64(5), I32(2)), 5L << 2) | ||
assertEvalsTo(ApplyBinaryPrimOp(LeftShift(), I64(-5), I64(2)), -5L<< 2) |
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.
Spacing.
assertEvalsTo(ApplyBinaryPrimOp(RightShift(), I32(5), i32na), null) | ||
assertEvalsTo(ApplyBinaryPrimOp(RightShift(), i32na, I32(2)), null) | ||
assertEvalsTo(ApplyBinaryPrimOp(RightShift(), i32na, i32na), null) | ||
|
||
assertEvalsTo(ApplyBinaryPrimOp(RightShift(), I64(0xffff5), I32(2)), 0xffff5L >> 2) | ||
assertEvalsTo(ApplyBinaryPrimOp(RightShift(), I64(-5), I64(2)), -5L>> 2) |
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.
Spacing
assertEvalsTo(ApplyBinaryPrimOp(BitXOr(), I32(5), i32na), null) | ||
assertEvalsTo(ApplyBinaryPrimOp(BitXOr(), i32na, I32(2)), null) | ||
assertEvalsTo(ApplyBinaryPrimOp(BitXOr(), i32na, i32na), null) | ||
|
||
assertEvalsTo(ApplyBinaryPrimOp(BitXOr(), I64(5), I64(2)), 5L ^ 2L) | ||
assertEvalsTo(ApplyBinaryPrimOp(BitOr(), I64(-5), I64(2)), -5L ^ 2L) |
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.
Need to be BitXOr (typos)
assertEvalsTo(ApplyBinaryPrimOp(LogicalRightShift(), i32na, i32na), null) | ||
|
||
assertEvalsTo(ApplyBinaryPrimOp(LogicalRightShift(), I64(0xffff5), I32(2)), 0xffff5L >>> 2) | ||
assertEvalsTo(ApplyBinaryPrimOp(LogicalRightShift(), I64(-5), I64(2)), -5L>>> 2) |
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.
spacing
ready for another review |
No description provided.