Open chriseclectic opened 1 year ago
Hi @chriseclectic, it looks like my PR #7966 on natively adding Cliffords to quantum circuits broke the above functionality, sorry! Most of these should be easy to fix, for instance the very first problem is caused by that after the above changes we don't know how to build a Clifford
from a QuantumCircuit
that contains another Clifford
, and could be fixed by adding the lines
if isinstance(gate, Clifford):
return _append_circuit(clifford, gate.to_circuit(), qargs)
just before checking gate.definition
. Though it may be possible to avoid synthesizing Clifford
to circuit (which to_circuit()
method for Cliffords or definition
method for Instruction would do) and instead to "compose" the Cliffords' tables directly. If no one objects, I would be happy to take a look at these problems.
Having condition
(as any other instruction) would also fix #9145
Clifford's added to circuits should satisfy semantics of unitary Gate
Instruction
s so that circuit methods can work correctly with them.
This isn't correct - Instruction
is no longer the core primitive of QuantumCircuit.data
. Instead, we should be asking "how do we update the behaviour for the new data model?". If we try and add everything back on to Operation
, we just end up back with the Instruction
class, which is too restrictive - you could equally well say "why doesn't Clifford
have a _define
method?", but the fact that circuit instructions shouldn't need to have a canonical decomposition independent of hardware is the reason for the Operation
class to exist.
For the same reason, we absolutely should not be adding condition
attributes to Operation
; we already made the decision that conditions are not a part of an individual operation, and so we shouldn't be regressing here, especially while we're trying to get it off all the other Instruction
instances too.
Obviously there are real issues here - there's clearly behaviour that was missed when the update to Operation
happened, and we need to be going through and working out how that should fit in the new data model. In general, the solution can't be to stick if isinstance(operation, Clifford)
everywhere, because that doesn't solve the problem, it just means that the next implementor of Operation
will face the same issues. We need a general solution for what is meant to happen. For classes that do have particular accelerators for Clifford
in particular, the isinstance
check can make sense (such as in quantum_info.Operator
, potentially).
In the general case, we need to know what the replacement strategy is for code that currently relies on definition
existing (or the particular code having magic handling for non-definition
object). I think this was (or should have been) part of the discussion before Operation
was implemented - what is the one-off, I-don't-care-about-efficiency entry-point to higher-level synthesis where there's no particular hardware in mind? We should make sure that we're rock solid on what our intent is for that, and then be swapping things over to use it (and making sure it's at least as ergonomic as checking for .definition
).
@chriseclectic, are any of these problems more urgent than others? E.g., would it make sense to commit the fix for creating Cliffords from QuantumCircuits that contain Cliffords, while we are evaluating options on how to properly handle reverse_ops
, inverse
, etc?
BTW, one can always explicitly convert Clifford
objects to Instruction
and to have access to all of the old functionality, for instance the following code works fine:
qc = QuantumCircuit(1, 1)
qc.append(random_clifford(1).to_instruction(), [0]).c_if(0, 1)
I should have explained the motivation for these recent changes. There are real benefits of not converting a Clifford
to Instruction
when we are adding it onto a QuantumCircuit
, but instead keeping it as an abstract mathematical object. As one example, we know how to compose consecutive Cliffords represented as mathematical objects. A quick experiment in #9169 shows that it's about 8x faster to compose two Cliffords using Clifford.compose
function compared to synthesizing these Cliffords into circuits.
@alexanderivrii I would say are things like Clifford(circuit)
, Operator(circuit)
working is most important, and the circuit methods like inverse
, reverse
etc (conditional is probably the lowest priority, but should be fixed eventually or raise an exception that its not supported).
@jakelishman I'm not suggesting you should go back to the old implementation, but the end user behaviour should have been equivalent before introducing these as this has introduced changes without any warning which can break existing code.
There are lot of methods in QuantumCircuit and quantum_info classes (and likely other places) which assume that all circuit members are instructions, so these all need to be updated to handle these new non-instruction circuit member types, along with a lot more unit tests to cover these sorts of errors.
Chris: maybe it wasn't clear, but what you said is exactly what I meant.
The PR https://github.com/Qiskit/qiskit-terra/pull/9169 fixes Clifford(circuit)
in the case that circuit
contains other cliffords; Operator(circuit)
used to work correctly already.
I am still unsure what is the best way to fix inverse(circuit)
and reverse
(circuit). Well, for inverse(circuit)
there is a very simple fix of adding the method inverse
to the Clifford
class (which would simply call adjoint
). @chriseclectic and @ikkoham, would you agree to such a solution? Of course it would be nicer to settle on a generic solution that would not need to change the Clifford
class and work with all possible high-level-objects.
As written above, inverse(circuit)
is the same as (clifford.adjoint()).to_circuit()
In addition, since the generators of the Clifford group (H, S and CX) are all symmetric, then:
reverse(circuit)
is the same as (clifford.transpose()).to_circuit()
.
Thanks Shelly, I completely agree that explicitly adding methods like inverse
and reverse_ops
methods to the Clifford class would easily solve the two remaining problems, however I am not convinced that doing this is the right approach, because we want to separate the quantum_info/clifford
object from the methods to support it as an object on a quantum circuit. If I did not understand your suggestion, please let me know.
Environment
What is happening?
Circuits containing
Clifford
operations cannot currently be used with the same classes used to construct them.How can we reproduce the issue?
Some examples:
Initializing a
Clifford
orStabilzierState
from circuit fails with following exception:Similarly for other methods that should work with Clifford circuits,
Clifford.compose
,Pauli.evolve
, etc.These circuits cause a bunch of circuit methods to break. For example
circuit.inverse()
Reverse ops:
Conditionals:
The above examples are likely not an exhaustive list since it looks like the Clifford's added to the circuit are not valid
Instructions
, and hence circuit methods and functions that consume circuits assuming this are breaking.What should happen?
I would expect Cliffords to behave like any other unitary gate instructions and all of the above methods to work as they used to when Cliffords were greedily synthesized to gate when being added to circuits.
Any suggestions?
Clifford's added to circuits should satisfy semantics of unitary Gate
Instructions
so that circuit methods can work correctly with them