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.

Sign up for GitHub

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

Jump to bottom

Do not unroll gates in basis in add_control #13475

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+142 −92
Open

Do not unroll gates in basis in add_control #13475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b38db38
Do not unroll gates in basis
Cryoris Nov 21, 2024
6ecc101
add tests
Cryoris Nov 21, 2024
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
209 changes: 118 additions & 91 deletions qiskit/circuit/add_control.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"""Add control to operation if supported."""
from __future__ import annotations

from math import pi
from qiskit.circuit.exceptions import CircuitError
from qiskit.circuit.library import UnitaryGate
from qiskit.transpiler import PassManager
Expand Down Expand Up @@ -92,7 +93,6 @@ def control(
Raises:
CircuitError: gate contains non-gate in definition
"""
from math import pi

# pylint: disable=cyclic-import
from qiskit.circuit import controlledgate
Expand All @@ -101,22 +101,32 @@ def control(

q_control = QuantumRegister(num_ctrl_qubits, name="control")
q_target = QuantumRegister(operation.num_qubits, name="target")
q_ancillae = None # TODO: add
controlled_circ = QuantumCircuit(q_control, q_target, name=f"c_{operation.name}")
if isinstance(operation, controlledgate.ControlledGate):
original_ctrl_state = operation.ctrl_state
operation = operation.to_mutable()
operation.ctrl_state = None

global_phase = 0
if operation.name == "x" or (
isinstance(operation, controlledgate.ControlledGate) and operation.base_gate.name == "x"
):
controlled_circ.mcx(q_control[:] + q_target[:-1], q_target[-1], q_ancillae)
if operation.definition is not None and operation.definition.global_phase:

basis = ["p", "u", "x", "z", "rx", "ry", "rz", "cx"]

if operation.name in basis:
apply_basic_controlled_gate(controlled_circ, operation, q_control, q_target[0])

# TODO: double check if this is necessary
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this todo completed? if not, how could we check if this is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet: I was wondering whether this was necessary because here we don't decompose the gate which we want to control and shouldn't be able to accumulate any global phase. I left it for the first version since the previous code had it, but I'll try removing and see if everything passes 🙂

if (
operation.definition is not None
and operation.definition.global_phase
and operation.name != "rz"
):
global_phase += operation.definition.global_phase

else:
basis = ["p", "u", "x", "z", "rx", "ry", "rz", "cx"]
if isinstance(operation, controlledgate.ControlledGate):
operation = operation.to_mutable()
operation.ctrl_state = None

unrolled_gate = _unroll_gate(operation, basis_gates=basis)
if unrolled_gate.definition.global_phase:
global_phase += unrolled_gate.definition.global_phase
Expand All @@ -130,87 +140,17 @@ def control(

for instruction in definition.data:
gate, qargs = instruction.operation, instruction.qubits
if gate.name == "x":
controlled_circ.mcx(q_control, q_target[bit_indices[qargs[0]]], q_ancillae)
elif gate.name == "rx":
controlled_circ.mcrx(
gate.definition.data[0].operation.params[0],
q_control,
q_target[bit_indices[qargs[0]]],
use_basis_gates=False,
)
elif gate.name == "ry":
controlled_circ.mcry(
gate.definition.data[0].operation.params[0],
q_control,
q_target[bit_indices[qargs[0]]],
q_ancillae,
mode="noancilla",
use_basis_gates=False,
)
elif gate.name == "rz":
controlled_circ.mcrz(
gate.definition.data[0].operation.params[0],
q_control,
q_target[bit_indices[qargs[0]]],
use_basis_gates=False,
)
continue
elif gate.name == "p":
from qiskit.circuit.library import MCPhaseGate

controlled_circ.append(
MCPhaseGate(gate.params[0], num_ctrl_qubits),
q_control[:] + [q_target[bit_indices[qargs[0]]]],
)
elif gate.name == "cx":
controlled_circ.mcx(
q_control[:] + [q_target[bit_indices[qargs[0]]]],
q_target[bit_indices[qargs[1]]],
q_ancillae,
)
elif gate.name == "u":
theta, phi, lamb = gate.params
if num_ctrl_qubits == 1:
if theta == 0 and phi == 0:
controlled_circ.cp(lamb, q_control[0], q_target[bit_indices[qargs[0]]])
else:
controlled_circ.cu(
theta, phi, lamb, 0, q_control[0], q_target[bit_indices[qargs[0]]]
)
else:
if phi == -pi / 2 and lamb == pi / 2:
controlled_circ.mcrx(
theta, q_control, q_target[bit_indices[qargs[0]]], use_basis_gates=True
)
elif phi == 0 and lamb == 0:
controlled_circ.mcry(
theta,
q_control,
q_target[bit_indices[qargs[0]]],
q_ancillae,
use_basis_gates=True,
)
elif theta == 0 and phi == 0:
controlled_circ.mcp(lamb, q_control, q_target[bit_indices[qargs[0]]])
else:
controlled_circ.mcp(lamb, q_control, q_target[bit_indices[qargs[0]]])
controlled_circ.mcry(
theta,
q_control,
q_target[bit_indices[qargs[0]]],
q_ancillae,
use_basis_gates=True,
)
controlled_circ.mcp(phi, q_control, q_target[bit_indices[qargs[0]]])
elif gate.name == "z":
controlled_circ.h(q_target[bit_indices[qargs[0]]])
controlled_circ.mcx(q_control, q_target[bit_indices[qargs[0]]], q_ancillae)
controlled_circ.h(q_target[bit_indices[qargs[0]]])
if len(qargs) == 1:
target = q_target[bit_indices[qargs[0]]]
else:
raise CircuitError(f"gate contains non-controllable instructions: {gate.name}")
if gate.definition is not None and gate.definition.global_phase:
target = [q_target[bit_indices[qarg]] for qarg in qargs]

apply_basic_controlled_gate(controlled_circ, gate, q_control, target)

if gate.definition is not None and gate.definition.global_phase and gate.name != "rz":
global_phase += gate.definition.global_phase

# apply controlled global phase
if global_phase:
if len(q_control) < 2:
Expand All @@ -228,6 +168,7 @@ def control(
new_ctrl_state = ctrl_state
base_name = operation.name
base_gate = operation

# In order to maintain some backward compatibility with gate names this
# uses a naming convention where if the number of controls is <=2 the gate
# is named like "cc<base_gate.name>", else it is named like
Expand All @@ -250,15 +191,101 @@ def control(
return cgate


def apply_basic_controlled_gate(circuit, gate, controls, target):
"""Apply a controlled version of ``gate`` to the circuit.
This implements multi-control operations for the following basis gates:
["p", "u", "x", "z", "rx", "ry", "rz", "cx"]
"""
num_ctrl_qubits = len(controls)

if gate.name == "x":
circuit.mcx(controls, target)

elif gate.name == "rx":
circuit.mcrx(
gate.definition.data[0].operation.params[0],
controls,
target,
use_basis_gates=False,
)
elif gate.name == "ry":
circuit.mcry(
gate.definition.data[0].operation.params[0],
controls,
target,
mode="noancilla",
use_basis_gates=False,
)
elif gate.name == "rz":
circuit.mcrz(
gate.definition.data[0].operation.params[0],
controls,
target,
use_basis_gates=False,
)
# continue
elif gate.name == "p":
from qiskit.circuit.library import MCPhaseGate

circuit.append(
MCPhaseGate(gate.params[0], num_ctrl_qubits),
controls[:] + [target],
)
elif gate.name == "cx":
circuit.mcx(
controls[:] + [target[0]], # CX has two targets
target[1],
)
elif gate.name == "u":
theta, phi, lamb = gate.params
if num_ctrl_qubits == 1:
if theta == 0 and phi == 0:
circuit.cp(lamb, controls[0], target)
else:
circuit.cu(theta, phi, lamb, 0, controls[0], target)
else:
if phi == -pi / 2 and lamb == pi / 2:
circuit.mcrx(theta, controls, target, use_basis_gates=True)
elif phi == 0 and lamb == 0:
circuit.mcry(
theta,
controls,
target,
use_basis_gates=True,
)
elif theta == 0 and phi == 0:
circuit.mcp(lamb, controls, target)
else:
circuit.mcp(lamb, controls, target)
circuit.mcry(
theta,
controls,
target,
use_basis_gates=True,
)
circuit.mcp(phi, controls, target)

elif gate.name == "z":
circuit.h(target)
circuit.mcx(controls, target)
circuit.h(target)

else:
raise CircuitError(f"Gate {gate} not in supported basis.")


def _gate_to_circuit(operation):
"""Converts a gate instance to a QuantumCircuit"""
if hasattr(operation, "definition") and operation.definition is not None:
return operation.definition
else:
qr = QuantumRegister(operation.num_qubits)
qc = QuantumCircuit(qr, name=operation.name)
qc.append(operation, qr)
return qc

qr = QuantumRegister(operation.num_qubits)
qc = QuantumCircuit(qr, name=operation.name)
qc.append(operation, qr)
return qc


def _unroll_gate(operation, basis_gates):
Expand Down
25 changes: 24 additions & 1 deletion test/python/circuit/test_controlled_gate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from qiskit.quantum_info.states import Statevector
import qiskit.circuit.add_control as ac
from qiskit.transpiler.passes import UnrollCustomDefinitions, BasisTranslator
from qiskit.transpiler.preset_passmanagers import generate_preset_pass_manager
from qiskit.converters.circuit_to_dag import circuit_to_dag
from qiskit.converters.dag_to_circuit import dag_to_circuit
from qiskit.quantum_info import Operator
Expand Down Expand Up @@ -1560,6 +1561,9 @@ def test_single_controlled_rotation_gates(self, gate, cgate):
op_mat = (np.cos(0.5 * self.theta) * iden - 1j * np.sin(0.5 * self.theta) * zgen).data
else:
op_mat = Operator(gate).data

print(self.ugu1)
print(cgate)
Comment on lines +1564 to +1566
Copy link
Contributor

Choose a reason for hiding this comment

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

sneaky prints!

Suggested change
print(self.ugu1)
print(cgate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh classic -- sorry about this one, I think I should add a pre-commit hook to check for these 😂

ref_mat = Operator(cgate).data
cop_mat = _compute_control_matrix(op_mat, self.num_ctrl)
self.assertTrue(matrix_equal(cop_mat, ref_mat))
Expand All @@ -1577,7 +1581,7 @@ def test_single_controlled_rotation_gates(self, gate, cgate):
elif gate.name == "rz":
self.assertLessEqual(uqc.size(), 43, f"\n{uqc}")
else:
self.assertLessEqual(uqc.size(), 20, f"\n{uqc}")
self.assertLessEqual(uqc.size(), 23, f"\n{uqc}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the size increase in this test? shouldn't the decomposition be more efficient? (I admit I haven't looked in depth so I might be missing something)


def test_composite(self):
"""Test composite gate count."""
Expand All @@ -1595,6 +1599,25 @@ def test_composite(self):
self.log.info("%s gate count: %d", uqc.name, uqc.size())
self.assertLessEqual(uqc.size(), 96, f"\n{uqc}") # this limit could be changed

def test_mcrz_complexity(self):
"""Test MCRZ is decomposed using the efficient MC-SU(2) algorithm.
Regression test of #13427.
"""
basis_gates = ["sx", "x", "rz", "ecr"]
pm = generate_preset_pass_manager(
optimization_level=3, basis_gates=basis_gates, seed_transpiler=12345
)

num_qubits = 6
angle = np.pi / 7
qc = QuantumCircuit(num_qubits)
qc.append(RZGate(angle).control(num_qubits - 1), qc.qubits)

isa_qc = pm.run(qc)

self.assertLessEqual(isa_qc.count_ops().get("ecr", 0), 65)


@ddt
class TestControlledStandardGates(QiskitTestCase):
Expand Down