Support ClExpr
#176
Support ClExpr
#176
Conversation
| c = circ.add_c_register("c", 5) | ||
| d = circ.add_c_register("d", 5) | ||
| circ.H(0) | ||
| wexpr, args = wired_clexpr_from_logic_exp(a | b, c) # type: ignore |
There was a problem hiding this comment.
I had to add # type: ignore here because it complains that the second argument c is a BitRegister, rather than list[Bit]. Is there a way around it? Is this a valid implicit cast, or should it be avoided?
There was a problem hiding this comment.
You could write c.to_list().
There was a problem hiding this comment.
All of the new tests in this file are just copy-pastes of existing tests using ClassicalExpBox, but now using ClExpr. It would be nice to add more tests, because these are minimal and only one of them is checking for correctness (and only on bit operations, rather than register ones).
Do you know of any tests from other repositories that could be used here? Any suggestions of where to get circuits to test on, for which the intended behaviour is known (and, hence, can be checked against)?
There was a problem hiding this comment.
Not really. There's a test here for the quantinuum local emulator, but it uses ClassicalExpBox with operations that are not supported here, so isn't much use.
There was a problem hiding this comment.
Looks good, just a few comments. It would be good to nail down the exact semantics of all the operations; I intentionally didn't do this in the pytket documentation because we don't seem to have an agreed semantics for extended QASM written down anywhere.
I'd recommend testing this also with the pre-release pytket 1.35.0rc0 if that's easy to do.
| @@ -239,6 +330,32 @@ def test_pytket_qir_conditional_10() -> None: | |||
| assert state.get_fidelity() == 1.0 | |||
|
|
|||
|
|
|||
| def test_pytket_qir_conditional_11() -> None: | |||
There was a problem hiding this comment.
I grabbed these tests from the pytket_qir repository (here) and did not notice that the qir in the name was no longer applicable here. Happy to rename it them test_basic_conditional_x.
| # From chat with Silas and exploring the RUS as a block matrix, we have noticed | ||
| # that the circuit is missing an X correction when this condition is satisfied |
There was a problem hiding this comment.
Is this issue tracked somewhere?
There was a problem hiding this comment.
This RUS circuit comes from a paper where they acknowledge that a "recovery operation" (i.e. correction) is required in some cases for RUS. As far as I can tell, they don't explicitly indicate what is the recovery operation required for this particular circuit (appearing in Fig 1c), but we figured it was an X correction.
AFAIK this is not tracked anywhere, but is known by people with experience on RUS.
| wexpr, args = wired_clexpr_from_logic_exp(flag[0] ^ flag[1], [flag[2]]) | ||
| circ.add_clexpr(wexpr, args) |
There was a problem hiding this comment.
Reading these tests makes me think we should add a method Circuit.add_clexpr_from_logic_exp() to combine these two steps.
There was a problem hiding this comment.
Yeah, could be a handy addition, but not strictly necessary.
| else: | ||
| # TODO: Currently not supporting ClOp's RegDiv since it does not return int, | ||
| # so I am unsure what the semantic is meant to be. | ||
| # TODO: I don't now what to do with RegNot, since input |
There was a problem hiding this comment.
The intended semantics of RegNot is bitwise-NOT, i.e. every bit of the register is flipped.
| # result = int(args_val[0] == 0) | ||
| elif expr.op in [ClOp.BitZero, ClOp.RegZero]: | ||
| result = 0 | ||
| elif expr.op in [ClOp.BitOne, ClOp.RegOne]: |
There was a problem hiding this comment.
The intended semantics of RegOne is that every bit is set to 1.
| # elif expr.op == ClOp.RegNeg: | ||
| # result = -args_val[0] | ||
| else: | ||
| # TODO: Currently not supporting ClOp's RegDiv since it does not return int, |
There was a problem hiding this comment.
I think it should return the integer quotient, i.e. floor(a/b) where a and b are unsigned integers. (If this doesn't fit in the result register, perhaps error is the kindest response.)
There was a problem hiding this comment.
But if we are leaving ADD etc unsupported for now I don't see a need to support DIV.
|
Thanks for the pointers on the semantics for the undefined operations. I will add these, and throw an error on overflow for now. I will bring up the need to agree on semantics of these extended QASM operations in the cross-team standup today. |
Description
Support for
ClExprRelated issues
#175
Checklist