Skip to content

add_control: check for predefined gates by type instead of name #3853

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

Closed
willhbang wants to merge 11 commits into from
Closed

add_control: check for predefined gates by type instead of name #3853

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
609dda5
add_control: check for predefined gates by type instead of name
willhbang Jan 12, 2020
b814cc7
Merge branch 'master' into gate-check
ewinston Feb 19, 2020
a1cabb8
handle controlled gates with empty definitions
willhbang Mar 3, 2020
30c9fae
control(): check for u1 and u3 before unrolling
willhbang Mar 3, 2020
4556eda
Merge remote-tracking branch 'upstream/master' into gate-check
willhbang Mar 4, 2020
ba1dc81
avoid unrolling standard controlled rotation gates
willhbang Mar 22, 2020
8865cd8
revert handling for mcrz gates
willhbang Mar 25, 2020
8bcd1a1
Merge branch 'master' into gate-check
willhbang Mar 25, 2020
3e99934
drop dependency on dict preserving order for python3.5
willhbang Mar 26, 2020
a11bff7
Merge remote-tracking branch 'upstream/master' into gate-check
willhbang Mar 31, 2020
2a46dc8
Merge branch 'master' into gate-check
ewinston May 21, 2020
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
52 changes: 30 additions & 22 deletions qiskit/circuit/add_control.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

from qiskit.circuit.exceptions import CircuitError
from qiskit.extensions import UnitaryGate
from qiskit.extensions.standard import XGate, RXGate, RYGate, RZGate, U1Gate, U3Gate
from . import ControlledGate, Gate, QuantumRegister, QuantumCircuit


Expand Down Expand Up @@ -84,7 +85,6 @@ def control(operation: Union[Gate, ControlledGate],
Raises:
CircuitError: gate contains non-gate in definition
"""
from math import pi
# pylint: disable=cyclic-import
import qiskit.circuit.controlledgate as controlledgate
# pylint: disable=unused-import
Expand All @@ -97,44 +97,35 @@ def control(operation: Union[Gate, ControlledGate],
q_ancillae = None # TODO: add
qc = QuantumCircuit(q_control, q_target)

if operation.name == 'x' or (
if isinstance(operation, XGate) or (
isinstance(operation, controlledgate.ControlledGate) and
operation.base_gate.name == 'x'):
isinstance(operation.base_gate, XGate)):

@willhbang willhbang Mar 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was going to add a case for cx below, but realized it's already handled here for arbitrary controls. Is it worth doing the same for the other gate checks below?

@ewinston ewinston Mar 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might be good to do for the rotation gates as well. It would be interesting to check if it reduces gate count.

@willhbang willhbang Mar 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ewinston gave this a shot in the commit below, but it's breaking add_control for RZ and CRZ gates. Not sure where to start investigating... any ideas where to dig further?

@ewinston ewinston Mar 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this breaks but if you remove the or condition in _operation_has_base-gate the issue seems to resolve. Then it's just isinstance(operation, gate) like in original logic.

qc.mct(q_control[:] + q_target[:-1],
q_target[-1],
None,
mode='noancilla')
elif operation.name == 'rx':
elif isinstance(operation, RXGate):
qc.mcrx(operation.definition[0][0].params[0], q_control, q_target[0],
use_basis_gates=True)
elif operation.name == 'ry':
elif isinstance(operation, RYGate):
qc.mcry(operation.definition[0][0].params[0], q_control, q_target[0],
q_ancillae, use_basis_gates=True)
elif operation.name == 'rz':
elif isinstance(operation, RZGate):
qc.mcrz(operation.definition[0][0].params[0], q_control, q_target[0],
use_basis_gates=True)
elif isinstance(operation, U1Gate):
qc.mcu1(operation.definition[0][0].params[2], q_control, q_target[0])
elif isinstance(operation, U3Gate):
theta, phi, lamb = operation.params
_apply_mcu3(qc, theta, phi, lamb, q_control, q_target[0], q_ancillae)
else:
bgate = _unroll_gate(operation, ['u1', 'u3', 'cx'])
# now we have a bunch of single qubit rotation gates and cx
for rule in bgate.definition:
if rule[0].name == 'u3':
theta, phi, lamb = rule[0].params
if phi == -pi / 2 and lamb == pi / 2:
qc.mcrx(theta, q_control, q_target[rule[1][0].index],
use_basis_gates=True)
elif phi == 0 and lamb == 0:
qc.mcry(theta, q_control, q_target[rule[1][0].index],
q_ancillae, mode='noancilla', use_basis_gates=True)
elif theta == 0 and phi == 0:
qc.mcrz(lamb, q_control, q_target[rule[1][0].index],
use_basis_gates=True)
else:
qc.mcrz(lamb, q_control, q_target[rule[1][0].index],
use_basis_gates=True)
qc.mcry(theta, q_control, q_target[rule[1][0].index],
q_ancillae, use_basis_gates=True)
qc.mcrz(phi, q_control, q_target[rule[1][0].index],
use_basis_gates=True)
_apply_mcu3(qc, theta, phi, lamb, q_control, q_target[rule[1][0].index],
q_ancillae)
elif rule[0].name == 'u1':
qc.mcu1(rule[0].params[0], q_control, q_target[rule[1][0].index])
elif rule[0].name == 'cx':
Expand Down Expand Up @@ -195,3 +186,20 @@ def _unroll_gate(operation, basis_gates):
dag = circuit_to_dag(_gate_to_circuit(operation))
qc = dag_to_circuit(unroller.run(dag))
return qc.to_gate()


def _apply_mcu3(circuit, theta, phi, lamb, q_controls, q_target, q_ancillae):
from math import pi

if phi == -pi / 2 and lamb == pi / 2:
circuit.mcrx(theta, q_controls, q_target, use_basis_gates=True)
elif phi == 0 and lamb == 0:
circuit.mcry(theta, q_controls, q_target,
q_ancillae, mode='noancilla', use_basis_gates=True)
elif theta == 0 and phi == 0:
circuit.mcrz(lamb, q_controls, q_target, use_basis_gates=True)
else:
circuit.mcrz(lamb, q_controls, q_target, use_basis_gates=True)
circuit.mcry(theta, q_controls, q_target,
q_ancillae, mode='noancilla', use_basis_gates=True)
circuit.mcrz(phi, q_controls, q_target, use_basis_gates=True)
2 changes: 2 additions & 0 deletions qiskit/circuit/controlledgate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ def definition(self):
elif isinstance(self, CnotGate):
qreg = QuantumRegister(self.num_qubits, 'q')
self._definition = [(self, [qreg[0], qreg[1]], [])]
else:
return self._definition
open_rules = []
for qind, val in enumerate(bit_ctrl_state[::-1]):
if val == '0':
Expand Down
6 changes: 6 additions & 0 deletions releasenotes/notes/control-by-type-68f1e151d93daeeb.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
fixes:
- |
Fixes cases in add_control(). Non-standard gates that were unfortunately named
identically to some standard ones (in particular, X, RX, RY, RZ) would be
identified as those gates, and the control would be applied erroneously.
17 changes: 15 additions & 2 deletions test/python/circuit/test_controlled_gate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,19 @@ def test_single_controlled_composite_gate(self):
ref_mat = execute(qc, simulator).result().get_unitary(0)
self.assertTrue(matrix_equal(cop_mat, ref_mat, ignore_phase=True))

def test_multi_control_u3(self):
@data(
[0.2, -pi/2, pi/2], # handle as rx
[0.2, 0, 0], # handle as ry
[0, 0, 0.4], # handle as rz
[0.2, 0.3, 0.4],
)
def test_multi_control_u3(self, params):
"""Test the matrix representation of the controlled and controlled-controlled U3 gate."""
import qiskit.extensions.standard.u3 as u3

num_ctrl = 3
# U3 gate params
alpha, beta, gamma = 0.2, 0.3, 0.4
[alpha, beta, gamma] = params

# cnu3 gate
u3gate = u3.U3Gate(alpha, beta, gamma)
Expand Down Expand Up @@ -626,6 +632,13 @@ def test_inverse_circuit(self, num_ctrl_qubits):
np.testing.assert_array_almost_equal(result.data,
np.identity(result.dim[0]))

def test_named_circuit(self):
"""Tests control is applied to operation type, not name"""
qc = QuantumCircuit(1, name='x')
gate = qc.to_gate()

self.assertIsNone(gate.control().definition) # Not a CnotGate

@willhbang willhbang Feb 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ewinston looks like a change in #3739, specifically here:
https://github.com/Qiskit/qiskit-terra/blob/025f6f9e73572a4aa608ac6e90bb1d7bc557e51f/qiskit/circuit/controlledgate.py#L85

is causing the definition get in this test to throw with
TypeError: can only concatenate list (not "NoneType") to list
because the gate generated from this circuit has no definition.

Is that ok/expected, since the gate here is degenerate? I originally added no operations to the test circuit, since it wouldn't be relevant to the testing the name bug. If so i'll add some arbitrary gate to the circuit.

@ewinston ewinston Mar 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think in this special case, where the controlled gate has an empty _definition, the definition method of ControlledGate should perhaps also return the same. Could you add an

else:
    return self._definition

to ControlledGate.definition after the elif clause to see if that resolves it?


@data(1, 2, 3, 4, 5)
def test_controlled_unitary(self, num_ctrl_qubits):
"""Test the matrix data of an Operator, which is based on a controlled gate."""
Expand Down