-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: Emit nested binary expressions for classical ops #224
fix: Emit nested binary expressions for classical ops #224
Conversation
dfc75c9
to
f724472
Compare
7ab2117
to
0d7815c
Compare
assert phir["ops"][2] == {"//": "IF ([c[0], c[1], c[2]] == 6) THEN Rz(0.5) q[0];"} | ||
assert phir["ops"][3]["condition"] == { | ||
"cop": "&", | ||
"args": [ | ||
{"cop": "==", "args": [["c", 0], 0]}, | ||
{ | ||
"cop": "&", | ||
"args": [ | ||
{"cop": "==", "args": [["c", 1], 1]}, | ||
{"cop": "==", "args": [["c", 2], 1]}, |
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.
Note endianness of pytket: #162 (comment)
Need to be careful of endianness here. The conditional
value
in pytket is little-endian, e.g. a value of 2 on the bits[a[0], a[1]]
meansa[0] == 0 && a[1] == 1
.
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, just a suggestion on edge-case handling.
@@ -356,15 +357,26 @@ def convert_classicalevalop(op: tk.ClassicalEvalOp, cmd: tk.Command) -> JsonDict | |||
|
|||
def multi_bit_condition(args: "list[UnitID]", value: int) -> JsonDict: |
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 have a feeling this will error out if len(args)
is 0 or 1. While these may be rare edge cases it would be good to handle them or at least raise an informative 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.
This would make sense, but PECOS does not seem to be able to deal with "cop": "&"
with fewer than two arguments.
ValueError: not enough values to unpack (expected 2, got 1)
Is there a way to add a "True" constant to an expression in PHIR? The AND with a single element args == [x]
could be implemented as (x == val) & (x == val)
, but in the case of args == []
you'd need to be able to output True somehow.
Not sure if we'd want to do this, though. Is it OK for pytket-phir to create valid PHIR when the conditions the user inputted would not be accepted by PECOS if done directly? It's true that in this case, the action of AND on 1 and 0 arguments is not ambiguous, but it still makes me uneasy. I'd just throw an exception with informative message.
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.
The function is only called at one place when len(args)
is not 1, but for generality (and reusability), it's certainly worth doing an edge case check. Implemented and pushed.
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.
Looks good to me too. I've checked that, with this change, the circuit from the project I was working on is converted to PHIR that can be interpreted by PECOS. Thanks!
Co-authored-by: Alec Edgington <[email protected]>
Description
Fixes:
Type of change
Checklist