-
Notifications
You must be signed in to change notification settings - Fork 36
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
Preliminary representation of rvalue classical expressions in Terra #30
Conversation
This is a proposal for initial representation of classical expressions in Terra. It is simple bit-level operations only on effectively `Clbit` and `ClassicalRegister`, and is deliberately designed to be separable from the rest of Terra so it can be replaced in the future if necessary, without painful decoupling of these objects from core Terra ones. This proposal notably does not require any new public methods or trackers on `QuantumCircuit`, `Clbit` or `ClassicalRegister`. This includes _not_ having a way to store a runtime expression in a variable of `QuantumCircuit` for re-use, nor of using these expressions in the parameters to gates. These may come in the future, but are deliberately excluded from prelim exploratory representations.
I just realised that I didn't include QPY support in the RFC, but I think for the MVP use-case we can actually get away without it; the known places that would support these dynamic-circuits paths (i.e. just IBM's |
Unless something has changed recently it actually does the OQ3 export server-side and submits the circuit as a qpy payload to the ibm runtime environment which calls the op3 export as part of the runner program. |
I don't think that's the case, because iirc I was able to support switch-statement jobs to the backends back in December directly from Qiskit, without manually exporting to OQ3 myself. But I could be mistaken. |
Oh no sorry, you're right and I'm totally misremembering. I'll update to add QPY support as a requirement for release. |
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 very cool, overall.
Despite being verbose, I'm a fan of the explicit construction of expressions in Stage 1. Personally, I'm not fully sold on the use of operator overloading in Stage 2. I tend to find it somewhat confusing as a user, since it feels like a surprise when types get magically coerced.
I'm sure you've already considered this, but just in case you haven't, I wonder what you'd think about an expression building syntax that uses chaining.
Here's your example modified to demonstrate what this might look like. Note that this would be in place of using context builders:
from qiskit.circuit import Clbit, ClassicalRegister
from qiskit.circuit.classical import expr
loose = [Clbit() for _ in [None]*3]
cr1 = ClassicalRegister(2)
cr2 = ClassicalRegister(2)
# Equivalent to the current condition `(cr, 2)`:
equal_reg = expr.Value(cr1).eq(2)
# Equivalent to the current condition `(loose[0], False)`
equal_bit = expr.Value(loose[0]).eq(False)
# The above example, but using a Bool-valued unary expression:
not_bit = expr.Value(loose[0]).not()
# The comparison 0 < cr1 <= cr2
bounded_reg = expr.Value(0).lt(cr1).and(expr.Value(cr1).lt_eq(cr2))
Arguments passed to chaining functions could be either of type Expr
, or they could be our standard types, which would be coerced into Expr
. This is similar to what you propose with operator overloading, but since this only happens for chaining function args, it might give the user better context clues to what is going on (and save an indentation!). Perhaps you could also take advantage of the left-hand side's greater precedence to coerce the right-hand side to match the width in the case of fixed-width type comparisons, though I haven't thought this through.
Assuming you've already considered something like this and have a preference for the proposal as it is, I'm on board with this moving forward as is.
|
||
The `resources` field is effectively a `use-def` tracker, which will more easily enable the conversion to data-flow (`DAGCircuit`) format to evaluate the necessary wires of an instruction. | ||
The `type` field is a resolved type of the contained expression. | ||
In this initial release, the only time that `type` will not be a fully qualified type will be when representing a `expr.Value` of a `uint` literal. |
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.
Not fully-qualified in that the size isn't fixed?
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.
Yeah, exactly. Yeah, "qualified" doesn't seem like the right word here. I'm pretty sure there is a correct word for it, I just can't think of it.
I hadn't actually considered chaining at all - honestly, I hadn't really considered anything much beyond what I wrote because I was rushing to get it up before the meeting yesterday. My rough thoughts on it are:
On balance, I think I personally still prefer the magic-method arithmetic form, but I'm happy to go with the crowd. |
I remember now where I'd originally seen this sort of problem solved. In Apache Spark SQL, they use a sort of DSL for column expressions. Maybe that can serve as some inspiration for either this RFC or a final design, though I'm sure there are quite a few differences in our requirements. |
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 so far. Thanks for writing this up.
This replaces the `resources` field; there is no need for this to be manually tracked during construction, and the visitor is how general double-dynamic dispatch should be handled.
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.
Thanks for the updates, this LGTM.
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
This is a proposal for initial representation of classical expressions in Terra. It is simple bit-level operations only on effectively
Clbit
andClassicalRegister
, and is deliberately designed to be separable from the rest of Terra so it can be replaced in the future if necessary, without painful decoupling of these objects from core Terra ones.This proposal notably does not require any new public methods or trackers on
QuantumCircuit
,Clbit
orClassicalRegister
. This includes not having a way to store a runtime expression in a variable ofQuantumCircuit
for re-use, nor of using these expressions in the parameters to gates. These may come in the future, but are deliberately excluded from prelim exploratory representations.