Skip to content
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

Adding Cliffords to QuantumCircuits as Operations #7966

Merged
merged 41 commits into from
Aug 10, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
068525a
Adding Clifford to a QuantumCircuit natively; adding some tests; addi…
alexanderivrii Apr 20, 2022
cd8367b
Merge branch 'main' into operation
alexanderivrii Apr 27, 2022
2ab3084
Making Instruction.condition into a property
alexanderivrii Apr 27, 2022
dffc618
improving tests
alexanderivrii Apr 27, 2022
ea56a13
adding release notes
alexanderivrii Apr 27, 2022
f3af0b6
A few changes based on review
alexanderivrii May 10, 2022
2640f5d
Merge branch 'main' into operation
alexanderivrii May 10, 2022
568d8b8
Merge branch 'main' into operation
alexanderivrii May 11, 2022
ec342e5
Merge branch 'main' into operation
alexanderivrii May 15, 2022
c16e85f
Merge branch 'main' into operation
alexanderivrii May 17, 2022
015cbe9
Merge branch 'main' into operation
alexanderivrii May 17, 2022
e35a3d5
resolving merge conflicts
alexanderivrii Jun 7, 2022
15ce409
Removing definition from Operation interface. Instead adding
alexanderivrii Jun 7, 2022
d59dd7e
minor lint fix
alexanderivrii Jun 7, 2022
51aff6d
moving all of broadcast functionality to a separate file; removing br…
alexanderivrii Jun 7, 2022
2c3b908
moving HighLevelSynthesis pass from Decompose to QuantumCircuit.decom…
alexanderivrii Jun 7, 2022
2a87222
Resolving conflcts, reverting broadcasting (temporarily breaks functi…
alexanderivrii Jul 2, 2022
14ddc7b
Fixing broadcasting for Cliffords
alexanderivrii Jul 2, 2022
4949a1d
making sure that transpile decomposes cliffords
alexanderivrii Jul 2, 2022
c1ab860
lint
alexanderivrii Jul 2, 2022
aeef894
As per code review, replacing x._direction by getattr(x, '_direction'…
alexanderivrii Jul 3, 2022
9f019c2
Removing condition from Operation API.
alexanderivrii Jul 5, 2022
c80efa9
pass over condition in transpiler code
alexanderivrii Jul 5, 2022
4b1a246
more refactoring of condition
alexanderivrii Jul 5, 2022
5ae37a5
finishing condition pass
alexanderivrii Jul 5, 2022
4a5f06f
Merge branch 'main' into operation
alexanderivrii Jul 5, 2022
34eca8c
resolving merge conflicts
alexanderivrii Aug 4, 2022
de6110d
resolving merge conflicts
alexanderivrii Aug 4, 2022
7a7b0f1
minor fixes
alexanderivrii Aug 5, 2022
76acd6b
Merge branch 'main' into operation
alexanderivrii Aug 5, 2022
64c8de3
adding OptimizeClifford pass to __init__
alexanderivrii Aug 6, 2022
b2f972a
Improving release notes
alexanderivrii Aug 6, 2022
0d583ed
considering DAG nodes in topological order; adding a simple test to s…
alexanderivrii Aug 6, 2022
bdf9325
typo
alexanderivrii Aug 6, 2022
49bc436
release notes fixes
alexanderivrii Aug 6, 2022
7f525d2
Adding TODO comment to HighLevelSynthesis pass
alexanderivrii Aug 6, 2022
d82e2e0
another attempt to fix docs build
alexanderivrii Aug 6, 2022
07733d6
Fix based on code review
alexanderivrii Aug 9, 2022
ac1c874
Merge branch 'main' into operation
alexanderivrii Aug 10, 2022
ab213f2
Only construction Operator from Instruction (as before) and from Clif…
alexanderivrii Aug 10, 2022
fb57c9d
Merge branch 'operation' of github.com:alexanderivrii/qiskit-terra in…
alexanderivrii Aug 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions qiskit/circuit/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@
Delay
Instruction
InstructionSet
Operation
EquivalenceLibrary

Control Flow Operations
Expand Down Expand Up @@ -231,6 +232,7 @@
from .controlledgate import ControlledGate
from .instruction import Instruction
from .instructionset import InstructionSet
from .operation import Operation
from .barrier import Barrier
from .delay import Delay
from .measure import Measure
Expand Down
13 changes: 12 additions & 1 deletion qiskit/circuit/instruction.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,13 @@
from qiskit.circuit.classicalregister import ClassicalRegister, Clbit
from qiskit.qobj.qasm_qobj import QasmQobjInstruction
from qiskit.circuit.parameter import ParameterExpression
from qiskit.circuit.operation import Operation
from .tools import pi_check

_CUTOFF_PRECISION = 1e-10


class Instruction:
class Instruction(Operation):
"""Generic quantum instruction."""

# Class attribute to treat like barrier for transpiler, unroller, drawer
Expand Down Expand Up @@ -546,6 +547,16 @@ def name(self, name):
"""Set the name."""
self._name = name

@property
def condition(self):
"""Returns the condition."""
return self._condition

@condition.setter
def condition(self, condition):
"""Set condition."""
self._condition = condition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instruction already as an instance variable called condition which will override this property. Since the setters and getters are no-ops anyway, we shouldn't need the properties.

@property
def num_qubits(self):
"""Return the number of qubits."""
Expand Down
6 changes: 3 additions & 3 deletions qiskit/circuit/instructionset.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
from typing import Callable, Optional, Tuple, Union

from qiskit.circuit.exceptions import CircuitError
from .instruction import Instruction
from .classicalregister import Clbit, ClassicalRegister
from .operation import Operation


# ClassicalRegister is hashable, and generally the registers in a circuit are completely fixed after
Expand Down Expand Up @@ -150,8 +150,8 @@ def __getitem__(self, i):

def add(self, gate, qargs, cargs):
"""Add an instruction and its context (where it is attached)."""
if not isinstance(gate, Instruction):
raise CircuitError("attempt to add non-Instruction" + " to InstructionSet")
if not isinstance(gate, Operation):
raise CircuitError("attempt to add non-Operation" + " to InstructionSet")
self.instructions.append(gate)
self.qargs.append(qargs)
self.cargs.append(cargs)
Expand Down
23 changes: 23 additions & 0 deletions qiskit/circuit/operation.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,26 @@ def num_qubits(self):
def num_clbits(self):
"""Number of classical bits."""
raise NotImplementedError

@abstractmethod
def broadcast_arguments(self, qargs, cargs):
"""Expanding (broadcasting) arguments."""
raise NotImplementedError

@property
@abstractmethod
def _directive(self):
"""Class attribute to treat like barrier for transpiler, unroller, drawer."""
raise NotImplementedError

@property
@abstractmethod
def condition(self):
"""Condition for when the instruction has a conditional if."""
raise NotImplementedError

@property
@abstractmethod
def definition(self):
"""Definition of the operation in terms of more basic gates."""
raise NotImplementedError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to carry this over to Operation since these are objects that we want to allow to not specify a decomposition at construction time (and especially, to not have to carry around a definition).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently multiple transpiler passes (Decompose, Unroll3qOrMore, etc.) require node.op.definition, so this is a method that had to be implemented for Cliffords and that imho Operation should support. However, the main concern is already addressed (for Cliffords): the definition circuit is not constructed at the gate creation time (and only when it's actually needed). For instance, the following code:

qc1 = QuantumCircuit(5)
qc1.append(random_clifford(3), [4, 0, 2])
qc1.append(random_clifford(3), [4, 0, 2])
qc1.append(random_clifford(3), [4, 0, 2])
qc2 = PassManager(OptimizeCliffords()).run(qc1)
qc3 = qc2.decompose()

only constructs the definition circuit for a single clifford in qc2 and only when decompose is called.

43 changes: 24 additions & 19 deletions qiskit/circuit/quantumcircuit.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
from .parametertable import ParameterTable, ParameterView
from .parametervector import ParameterVector, ParameterVectorElement
from .instructionset import InstructionSet
from .operation import Operation
from .register import Register
from .bit import Bit
from .quantumcircuitdata import QuantumCircuitData
Expand Down Expand Up @@ -1160,40 +1161,40 @@ def _resolve_classical_resource(self, specifier):

def append(
self,
instruction: Instruction,
instruction: Operation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also update the docstring below, and potentially rename the argument from instruction to operation. (There's a util function https://github.com/Qiskit/qiskit-terra/blob/04807a13a7ba53d97c37bb092324e419296e3fba/qiskit/utils/deprecation.py#L19 to simplify the process of deprecating the original kwarg.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the docstring. Would it be possible to postpone renaming/deprecation to a small follow-up PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't issue a deprecation warning until the alternative has been in place for a version. I wouldn't worry about it, though. Perhaps a long-term solution is (once we drop Python 3.7 support) to make this first argument positional-only (def append(self, instruction, /, qargs, cargs)), since append can also take a single CircuitInstruction as of #8093, but really, it's a minor problem we don't need to touch right now.

qargs: Optional[Sequence[QubitSpecifier]] = None,
cargs: Optional[Sequence[ClbitSpecifier]] = None,
) -> InstructionSet:
"""Append one or more instructions to the end of the circuit, modifying
"""Append one or more operations to the end of the circuit, modifying
the circuit in place. Expands qargs and cargs.

Args:
instruction (qiskit.circuit.Instruction): Instruction instance to append
instruction (qiskit.circuit.Operation): Operation instance to append
qargs (list(argument)): qubits to attach instruction to
cargs (list(argument)): clbits to attach instruction to

Returns:
qiskit.circuit.Instruction: a handle to the instruction that was just added
qiskit.circuit.InstructionSet: a handle to the instruction that was just added

Raises:
CircuitError: if object passed is a subclass of Instruction
CircuitError: if object passed is neither subclass nor an instance of Instruction
CircuitError: if object passed is a subclass of Operation
CircuitError: if object passed is neither subclass nor an instance of Operation
"""
# Convert input to instruction
if not isinstance(instruction, Instruction) and not hasattr(instruction, "to_instruction"):
if issubclass(instruction, Instruction):
if not isinstance(instruction, Operation) and not hasattr(instruction, "to_instruction"):
if issubclass(instruction, Operation):
raise CircuitError(
"Object is a subclass of Instruction, please add () to "
"Object is a subclass of Operation, please add () to "
"pass an instance of this object."
)

raise CircuitError(
"Object to append must be an Instruction or have a to_instruction() method."
"Object to append must be an Operation or have a to_instruction() method."
)
if not isinstance(instruction, Instruction) and hasattr(instruction, "to_instruction"):
if not isinstance(instruction, Operation) and hasattr(instruction, "to_instruction"):
instruction = instruction.to_instruction()
if not isinstance(instruction, Instruction):
raise CircuitError("object is not an Instruction.")
if not isinstance(instruction, Operation):
raise CircuitError("object is not an Operation.")

# Make copy of parameterized gate instances
if hasattr(instruction, "params"):
Expand All @@ -1218,11 +1219,11 @@ def append(

def _append(
self,
instruction: Instruction,
instruction: Operation,
qargs: Sequence[Qubit],
cargs: Sequence[Clbit],
) -> Instruction:
"""Append an instruction to the end of the circuit, modifying the circuit in place.
) -> Operation:
"""Append an operation to the end of the circuit, modifying the circuit in place.

.. warning::

Expand All @@ -1242,12 +1243,12 @@ def _append(
constructs of the control-flow builder interface.

Args:
instruction: Instruction instance to append
instruction: Operation instance to append
qargs: Qubits to attach the instruction to.
cargs: Clbits to attach the instruction to.

Returns:
Instruction: a handle to the instruction that was just added
Operation: a handle to the instruction that was just added

:meta public:
"""
Expand All @@ -1261,7 +1262,11 @@ def _append(

return instruction

def _update_parameter_table(self, instruction: Instruction) -> Instruction:
def _update_parameter_table(self, instruction: Operation) -> Operation:
# A generic Operation object at the moment does not require to have params.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is the right time to reconsider whether Operations should require params (I don't recall the exact reason they were removed from the PR implementing Operation). If not, checking isinstance(instruction, Instruction) seems more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the code to check isinstance(instruction, Instruction).

I don't remember the reason of not including params in Operation either. Possibly this would be a good idea, even though Cliffords don't use params to store stabilizer/destabilizer table. Do we want them to? Is it true that two Instructions with the same name, num_qubits, num_clbits and parameters are completely equivalent? Do we want this to be true?

if not isinstance(instruction, Instruction):
return instruction

for param_index, param in enumerate(instruction.params):
if isinstance(param, (ParameterExpression, QuantumCircuit)):
# Scoped constructs like the control-flow ops use QuantumCircuit as a parameter.
Expand Down
18 changes: 9 additions & 9 deletions qiskit/quantum_info/operators/operator.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import numpy as np

from qiskit.circuit.quantumcircuit import QuantumCircuit
from qiskit.circuit.instruction import Instruction
from qiskit.circuit.operation import Operation
from qiskit.circuit.library.standard_gates import IGate, XGate, YGate, ZGate, HGate, SGate, TGate
from qiskit.exceptions import QiskitError
from qiskit.quantum_info.operators.predicates import is_unitary_matrix, matrix_equal
Expand Down Expand Up @@ -53,7 +53,7 @@ def __init__(self, data, input_dims=None, output_dims=None):

Args:
data (QuantumCircuit or
Instruction or
Operation or
BaseOperator or
matrix): data to initialize operator.
input_dims (tuple): the input subsystem dimensions.
Expand All @@ -75,8 +75,8 @@ def __init__(self, data, input_dims=None, output_dims=None):
if isinstance(data, (list, np.ndarray)):
# Default initialization from list or numpy array matrix
self._data = np.asarray(data, dtype=complex)
elif isinstance(data, (QuantumCircuit, Instruction)):
# If the input is a Terra QuantumCircuit or Instruction we
elif isinstance(data, (QuantumCircuit, Operation)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other places where Instruction should be updated to Operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think I have updated all the places that I could find, but maybe I've still missed a few.

# If the input is a Terra QuantumCircuit or Operation we
# perform a simulation to construct the unitary operator.
# This will only work if the circuit or instruction can be
# defined in terms of unitary gate instructions which have a
Expand Down Expand Up @@ -498,7 +498,7 @@ def _einsum_matmul(cls, tensor, mat, indices, shift=0, right_mul=False):

@classmethod
def _init_instruction(cls, instruction):
"""Convert a QuantumCircuit or Instruction to an Operator."""
"""Convert a QuantumCircuit or Operation to an Operator."""
# Initialize an identity operator of the correct size of the circuit
if hasattr(instruction, "__array__"):
return Operator(np.array(instruction, dtype=complex))
Expand All @@ -514,7 +514,7 @@ def _init_instruction(cls, instruction):
@classmethod
def _instruction_to_matrix(cls, obj):
"""Return Operator for instruction if defined or None otherwise."""
if not isinstance(obj, Instruction):
if not isinstance(obj, Operation):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually correct? to_matrix() isn't defined as part of the Operation class is it? Are you just doing this so you can pass a clifford directly here to create a separate operator class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is a very good point. On the one hand, we definitely want to construct an Operator from a quantum circuit that contain a Clifford or some other higher-level object:

cliff = Clifford(...)
qc = QuantumCircuit(5)
qc.append(cliff, [0,1,2])
op = Operator(qc)

On the other hand, the current code only works because Clifford happens to have a to_matrix() method, which is not required for a general Operation. So my "fix" above would not carry-over to future higher-level objects. Thinking aloud, the main code (see https://github.com/Qiskit/qiskit-terra/blob/2bab09c1aae84e5bf38ba52fa3a272667c1887cc/qiskit/quantum_info/operators/operator.py#L529) first checks that an Operation has or can have a matrix, then if it's a Barrier, then if it has a definition, and then raises an error. So ideally (as higher-level objects do not need to carry a definition either) we should call the HighLevelSynthesis pass here (or more precisely whichever routine that uses to synthesize Cliffords). But this is ugly -- as quantum_info/Operator would depend on transpiling routines, leading to all sorts of cyclic dependencies (run-time import and pylint-disable would fix this, but nonetheless). And I don't see another way. @mtreinish , @kdk , what do you think about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about doing:

Suggested change
if not isinstance(obj, Operation):
if not isinstance(obj, Operation) and hasattr(obj, "to_matrix"):

that way we still work with operations like clifford that have a defined matrix. We probably should include this in the operation class docs too as an optional function if we start making this an implicit part of the class (I do worry about bloating operation and having it just repeat Instruction though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I see now. It should be ok that we cannot construct an Operator for an arbitrary quantum circuit, and in particular for circuits containing some higher-level objects (and we already can't do it for circuits containing conditional gates). Though, I completely agree, we shouldn't break this functionality for Cliffords.

It's also no clear to me if constructing Operators for higher-level objects based on HighLevelSynthesis is correct (i.e. what I was suggesting earlier) -- what if the synthesis method uses ancillas?

So I am happy to adopt the above suggestion (though, hmm, it should be not hasattr...).

Agreed, I would also like to avoid bloating Operation. So, if I understood your suggestion correctly, I will simply include a comment in the Operation class docs.

raise QiskitError("Input is not an instruction.")
mat = None
if hasattr(obj, "to_matrix"):
Expand Down Expand Up @@ -544,10 +544,10 @@ def _append_instruction(self, obj, qargs=None):
# circuit decomposition definition if it exists, otherwise we
# cannot compose this gate and raise an error.
if obj.definition is None:
raise QiskitError(f"Cannot apply Instruction: {obj.name}")
raise QiskitError(f"Cannot apply Operation: {obj.name}")
if not isinstance(obj.definition, QuantumCircuit):
raise QiskitError(
'Instruction "{}" '
'Operation "{}" '
"definition is {} but expected QuantumCircuit.".format(
obj.name, type(obj.definition)
)
Expand All @@ -569,7 +569,7 @@ def _append_instruction(self, obj, qargs=None):
for instr, qregs, cregs in flat_instr:
if cregs:
raise QiskitError(
f"Cannot apply instruction with classical registers: {instr.name}"
f"Cannot apply operation with classical registers: {instr.name}"
)
# Get the integer position of the flat register
if qargs is None:
Expand Down
38 changes: 37 additions & 1 deletion qiskit/quantum_info/operators/symplectic/clifford.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@
from qiskit.quantum_info.operators.scalar_op import ScalarOp
from qiskit.quantum_info.synthesis.clifford_decompose import decompose_clifford
from qiskit.quantum_info.operators.mixins import generate_apidocs, AdjointMixin
from qiskit.circuit.operation import Operation
from .stabilizer_table import StabilizerTable
from .clifford_circuits import _append_circuit


class Clifford(BaseOperator, AdjointMixin):
class Clifford(BaseOperator, AdjointMixin, Operation):
"""An N-qubit unitary operator from the Clifford group.

**Representation**
Expand Down Expand Up @@ -101,6 +102,10 @@ class Clifford(BaseOperator, AdjointMixin):
`arXiv:quant-ph/0406196 <https://arxiv.org/abs/quant-ph/0406196>`_
"""

# Fields that are currently required to add an object as an Operation.
condition = None
_directive = False

def __array__(self, dtype=None):
if dtype:
return np.asarray(self.to_matrix(), dtype=dtype)
Expand Down Expand Up @@ -134,6 +139,9 @@ def __init__(self, data, validate=True):
"Invalid Clifford. Input StabilizerTable is not a valid symplectic matrix."
)

# When required, we will compute the QuantumCircuit for this Clifford.
self._definition = None

# Initialize BaseOperator
super().__init__(num_qubits=self._table.num_qubits)

Expand Down Expand Up @@ -540,6 +548,34 @@ def _pad_with_identity(self, clifford, qargs):

return padded

def broadcast_arguments(self, qargs, cargs):
"""
Broadcasting of the arguments.
This code is currently copied from Instruction.
This will be cleaned up when broadcasting is moved
to a separate model.

Args:
qargs (List): List of quantum bit arguments.
cargs (List): List of classical bit arguments.

Yields:
Tuple(List, List): A tuple with single arguments.
"""

# [[q[0], q[1]], [c[0], c[1]]] -> [q[0], c[0]], [q[1], c[1]]
flat_qargs = [qarg for sublist in qargs for qarg in sublist]
flat_cargs = [carg for sublist in cargs for carg in sublist]
yield flat_qargs, flat_cargs

@property
def definition(self):
"""Computes and returns the circuit for this Clifford."""
if self._definition is None:
self._definition = self.to_circuit()

return self._definition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, how far do we make it through a transpile call if we don't add a definition? The long term goal is to have a transpiler pass which can synthesize Cliffords and use the transpilation context to make some intelligent choices between different synthesis methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, can to_instruction above now return self instead of converting to a circuit and then to an instruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how far do we make it through a transpile call if we don't add a definition?

I would say quite far, please see the code snippet from my answer to a previous question.

The long term goal is to have a transpiler pass which can synthesize Cliffords and use the transpilation context to make some intelligent choices between different synthesis methods.

I completely agree. This is also something that @ShellyGarion is interested in, as she wants to be able to choose at transpile time whether to apply a synthesis method that minimizes gate-count vs. depth and whether to apply a method that is better suited for all-to-all connectivity vs. linear-neighbor connectivity.

can to_instruction above now return self instead of converting to a circuit and then to an instruction?

This is an interesting suggestion (that I have not tried yet). I might be wrong about this, but I am afraid that certain methods (like constructing a unitary Operator for a quantum circuit containing a clifford) depend on to_operation translating more complex objects to simpler objects.



# Update docstrings for API docs
generate_apidocs(Clifford)
Loading