-
Notifications
You must be signed in to change notification settings - Fork 51
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 SignExtend
for two's complement sign extension
#1162
Conversation
3f9d5e1
to
a66eab8
Compare
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 was going to create this bloq as well ... this is where I stand #1163
you can incorporate my code into yours and then we can merge yours
I don't know of a way to do resizing for QFxp that would make sense. I think this belongs in bookkeeping not arithmetic + this bloq needs to implement classical action ... I added all of this to my PR which you can copy
My understanding is that bookkeeping bloqs are only for building the correct bloq graph, but do not actually perform any quantum action. But this bloq actually performs a non-trivial action, so it makes more sense to keep it in arithmetic. |
This is a reasonable set of logic to apply to the organization, thanks |
|
||
|
||
@frozen | ||
class SignExtend(Bloq): |
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.
Since we need to support the adjoint, should the bloq just be called Resize
? It's better to explicitly support resizing down and add tests than expecting Adjoint(SignExtend())
to do the right thing?
Also, if we are adding a Resize
we can explain in docstring the behavior of resizing a signed vs unsigned type and probably just support resizing both instead of supporting resizing only the signed types.
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.
Up to the consensus, but my 2 cents would be 1) resize definitely sounds more like a bookkeeping operation and hides the fact that there's a tiny bit of logic to respect the sign bit 2) the linked wiki page is called "sign extend"
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.
Ops, I missed the Wikipedia page. SignExtend LGTM then -- let's just support both signed and unsigned types and document the behavior for Adjoint / add a new Bloq for SignTruncate (Truncate seems to be used as the opposite of Extend at some places)?
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.
let's just support both signed and unsigned types
+1
my decomposition supports both directions #1163
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 SignTruncate
for the adjoint. This PR supports signed values only.
In a follow-up, we can add unsigned support using a separate bloq (say ZeroExtend
to stick to convention) and a support bloq ResizeInt
which delegates to the correct extension or truncation bloq based on the dtypes
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.
with sign extend and zero extend we will have a pattern of checking dtype before using the appropiate bloq
if isinstance(self.dtype, ...): bb.add(SignExtend...)
else: bb.add(ZeroExtend...)
how about ArithmeticResize
for a bloq that contains both logics?
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 it's fine to merge this PR and add the logic of zero extend either in the same bloq, or a new bloq, in a follow up PR. If we decide to add a new Bloq for ZeroExtend
, we can have an equivalent of a SignedOrZeroExtend
that delegates to signed or zero extend depending on the dtype.
qualtran/bloqs/arithmetic/conversions/ones_complement_to_twos_complement.py
Show resolved
Hide resolved
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.
LGTM - Please open an issue to document any future improvements needed to SignExtend
(like supporting unsigned types)
43dad28
to
d3c7b3e
Compare
Useful for a lot of arithmetic ops which allow different bitsizes. wiki