qiskit
qiskit copied to clipboard
Move numeric parameters from `Instruction` to the data tuple
What should we add?
This is the first step in adding full classical data and instructions to QuantumCircuit
. This is a partial step; it may well not be the only change to the data tuple that we make over the course of this, and it doesn't try to fix everything we dislike about Instruction
yet, but it's a start.
I do recognise that what I suggest here is a breaking API change, but I think some degree of that is going to be unavoidable when we're making changes to basically the entirety of our core data structure and flow. I've tried to limit the damage.
Apologies that this is a bit long, I'm trying to write down all my thoughts about this step so we can get started while I'm out writing my thesis. @kdk is managing this issue while I'm out.
Why we chose this
We chose this because it should yield some tangible benefits immediately, without needing to consider the new type system yet:
- it lets the QASM exporters always access the parameterised definition of an instruction to emit correct code (e.g. #7335, #7172, and probably some more open issues)
- it will (hopefully) remove the need to deepcopy parameterised
Instruction
instances when binding parameters in circuits, in these lines: https://github.com/Qiskit/qiskit-terra/blob/13370342bca77000bf7321d8c581d5661b1a7fb7/qiskit/circuit/quantumcircuit.py#L1232-L1236 - it will make parameter binding more efficient and less error prone, because the binding will be lazier; the re-bind will naturally only happen at the top level by replacing the
QuantumCircuit.data
list entry, i.e. these (quite inefficient) lines should be made redundant: https://github.com/Qiskit/qiskit-terra/blob/13370342bca77000bf7321d8c581d5661b1a7fb7/qiskit/circuit/quantumcircuit.py#L2680-L2691
What to do
The aim is to move any params
of existing Instruction
s that may be ParameterExpression
into a fourth element of the (instruction, qargs, cargs)
tuple. It might be convenient to do this in two steps:
- convert the 3-tuple into a data class, so that everywhere needs to use named (dotted) access (i.e.
instruction.operation
,instruction.qargs
andinstruction.cargs
). For now, it might need to be called something private (like_Instruction
) because we haven't quite sorted out the migration path from the currentInstruction
. It would be good to makeqargs
andcargs
intotuple
, notlist
at this step - the immutability will help later. - add a fourth element to the class (called
parameters
by analogy to the current name, though strictly they'd be arguments not parameters!), and teach all the neceessary parts of Terra that to get the concrete definition or matrix form, they need to pass these in. This should also be a tuple, not a list.
Moving to named access is useful for updating the class in the future; it's easier to catch a name change with pylint
than it is to catch the positional elements changing type or reordering. There may be a slight performance impact to this on the surface, but I think making this object mutable (but all its contents eventually immutable) will be much better performing in the long-run - copies of circuits and binding of parameters will become much cheaper, and getting to a point of internal immutability should let us massively reduce our memory footprint, which is great of itself but also has indirect performance advantages (even in Python).
In order to limit the immediate API breakage, it may be worth defining the Sequence
interface on this new class as if it were still the old 3-tuple, so most tuple-like access on it will continue to work in user code. I would want to test suite to pass without this interface defined, though - we need to make sure Terra is fully updated.
Without actually trying it, it's hard to predict all the consequences, but here's some potential implementation considerations and pitfalls:
-
Instruction
/Operation
will probably need to gain a new variablenum_parameters
for type checking. - For compatibility right now,
Instruction.definition
should likely be a pointer to a parameterisedQuantumCircuit
, and it will be up to the caller to bind the parameters, when they need them to be bound. We need to ensure that the parameter order is fixed and correct. - Similarly,
Gate.inverse
would ideally be defined to return a parameterisedQuantumCircuit
, which the caller will be responsible for binding when it needs to be bound. -
Instruction.__array__
(orGate.__array__
) will probably cease to exist, because the operation will not hold all the state necessary to construct a concrete array. That's deliberate - we don't want the operation (currently theInstruction
instance) to hold the state any more. - The internal
ParameterTable
structure inQuantumCircuit
will need updating. A lot of #7408 is absolutely relevant here. - The control-flow builder interface will need to start tracking
Parameter
s as well. I think there are some subleties here (especially withIfElseContext
andIfElseOp
, which manage two circuit bodies), but I've not got so far as sketching out that process yet.
Limitations during this step
We don't want to do everything at once. There are lots of things that need more thought and design, and this change is already going to be very big, so we're trying to work incrementally. Several things in this step we know aren't perfect, but it will be easier to review the effects of changes and iterate the design if we go step-by-step.
- don't attempt to move things like
ControlFlowOp
'sblocks
,ControlledGate
'sctrl_state
orUnitaryGate
's matrix out of the relevantInstruction
. For now, the newparameters
should be exactly the tuple of things that can validly beParameterExpression
. - leave
ForLoopOp
'sloop_parameter
in the class, not in the new tuple - this is because the scoping of it may be internal toForLoopOp
. - no need to fix the crazy
Instruction.broadcast_arguments
at this point - it's inconsistent and weird, and it can just stay that way - there should be no need to mess with the
Instruction
/abstractOperation
split - that's not fully in place yet, but the changes here should largely be orthogonal (except perhapsOperation
needs thenum_parameters
variable). - we will keep the split of
cargs
andparameters
. This may change in the future once we have a stronger classical type system, butClbit
is always going to be special in some way (it's the only valid l-value of ameasure
), so it's good to keep it separate. - we're not interested in solving the problem of dynamic qubit or clbit reference yet (i.e. we're not immediately trying to support the [not yet necessarily valid] OpenQASM 3 statement
my_creg[i] = measure my_qreg[i];
in this step)
Some other things considered
Using a tuple
or namedtuple
for the elements of QuantumCircuit.data
It's hard to do ParameterTable
and parameter binding efficiently if the elements are immutable - you would need a way to store a pointer into the QuantumCircuit.data
structure that can be assigned to in constant time. For list
, that's an integer index, except the index is kind of state-dependent; using that makes it impossible to insert or remove instructions in any efficient way at all - we'd have to loop through ParameterTable
each time updating everything. We also need to think about the swap to a DAG data structure - there, it's going to be hard to store an "assignable" pointer.
These issues aren't so much of a problem if the qc.data
element is mutable itself; we can use the same trick we currently do of storing it in the ParameterTable
, and mutating it in-place to replace the parameters
item. The current state of QuantumCircuit
needs to deepcopy its Instruction
s to be safe. We wouldn't need to do this any more; the new lazier pattern means that we could safely simply shallow-copy the data element that's currently a tuple, since its components should be (or at least end up being) immutable.
Moving the entirety of Instruction.params
into the tuple
We have rough desires to get the state out of Instruction
entirely, but don't have a full plan for getting there. Separating out just the ParameterExpression
bits first is something we know we'll need, because we need to start tracking data that gets fed into these slots for dynamic circuits, and there are the tangible benefits listed above to these. We will need to get these slots separately in the future in some form or another in order to make/bind QASM-ish gate my_gate(a, b, c) q1, q2 { ... }
logical statements, as a
, b
and c
are the only values that can be modified dynamically during the circuit.
If we move everything out immediately, we've mostly just shifted the issue, and we don't get most of the nice immediate benefits at the start of this issue.
@kdk: I tried to add this to the right epic on the beta projects board, but I don't seem to have permissions to change anything there.
@kdk: I tried to add this to the right epic on the beta projects board, but I don't seem to have permissions to change anything there.
Looks like you hadn't been added you to the qiskit-terra team, should be fixed now.
If I understand correctly, this removes the "quantum gate" concept from Qiskit. For example, it would no longer be possible to construct an object that represents the gate Rx(0.1)
. If this is true, I worry that eliminating such a common concept would hurt Qiskit's ease of use, as well as ease of learning.
To some degree, yeah, but it's also kind of like saying that Qiskit doesn't have a concept of Rx(0.1 on qubit 0)
. We wouldn't say a classical RISC architecture doesn't have the instruction of add 1
because it implements a general add
. The idea is to separate out operands from operations - rx
is an operation, and "qubit 0" and 0.1
are the operands. The other things in the comment explain my reasons for why doing so is beneficial/necessary for supporting dynamic circuits.
For ease of use: we don't expect the internal data structures to be the preferred way for how users interact with our systems. There's no changes planned for the interface of QuantumCircuit.rx
and friends right now. Frontend and backend are two separate concerns, and while they inform each other, they don't need to be completely identical - for example, the internal tree representation of control flow at the moment is quite tricky to work with for users, but we provide a builder interface that makes it drastically simpler. It got to the point when writing the tests for it, 95% of the time a failing test meant I'd got the tree structure wrong and the builders worked fine, but we still need to use a tree or some form of CFG internally.
There's no changes planned for the interface of QuantumCircuit.rx and friends right now.
This syntax does not work for gates which don't have a corresponding method on QuantumCircuit. This is an important use case, and one which is very commonly understood in terms of the gate concept (rather than the instruction/operation concept you refer to).
With this change, how would one obtain the matrix of a gate, say, Rx(0.1)
?
The append
interface is a bit more open to discussion, but long-term strategy is for an entirely new mechanism of building quantum circuits. It may well be the case that the circuit is constructed by a new DSL with syntax that looks like
with QuantumCircuit.build() as circuit:
RX(0.1) | (0,)
X | (1,)
...
but this is still a long way off - the internal discussions are very preliminary (and have been for a year). The first priority is to get the internal components of dynamic circuits working end-to-end, from Terra to backends, because it's no good having nice syntax if the system doesn't work. append
will work with the new system (as in append(CircuitInstruction(RXGate, (qubit[0],), (), (0.1)))
), it'll just be a little fiddlier at first.
In the internal-only structure, the current plan is to make Instruction.definition
and a new Instruction.matrix
(names not final) that take the numeric operands as parameters, so to get the matrix you'd do RXGate.matrix(0.1)
.
I appreciate that your applications think of gates as "gate + parameters" and that that's common, but equally common is the alternative of "RX
is a gate and RX(0.1)
is an application", especially at the transpilation and hardware layers. It's better for our internal data structures to be optimised for the latter, and we handle your higher-level view by syntactic sugar. The way you're thinking is a priority for us, it's just not the first priority.
I should also be clear that this won't happen overnight: we're committed to maintaining the API stability, so we'll be doing all we can to ensure that qc.append(RXGate(0.1), [0], [])
continues to work for at least a few more Terra versions. It'll just get converted to something else internally.