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

Remove method qasm from Instruction and all of its subclasses #10399

Merged
merged 20 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
9 changes: 9 additions & 0 deletions qiskit/circuit/instruction.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
from qiskit.qobj.qasm_qobj import QasmQobjInstruction
from qiskit.circuit.parameter import ParameterExpression
from qiskit.circuit.operation import Operation
from qiskit.utils.deprecation import deprecate_func
from .tools import pi_check

_CUTOFF_PRECISION = 1e-10
Expand Down Expand Up @@ -446,6 +447,14 @@ def _qasmif(self, string):
)
return "if(%s==%d) " % (self.condition[0].name, self.condition[1]) + string

@deprecate_func(
additional_msg=(
"Correct exporting to OpenQASM 2 is the responsibility of a larger exporter; it cannot "
"safely be done on an object-by-object basis without context. No replacement will be "
"provided, because the premise is wrong."
),
since="0.25.0",
)
def qasm(self):
"""Return a default OpenQASM string for the instruction.

Expand Down
9 changes: 9 additions & 0 deletions qiskit/circuit/library/standard_gates/x.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from typing import Optional, Union, Type
from math import ceil, pi
import numpy
from qiskit.utils.deprecation import deprecate_func
from qiskit.circuit.controlledgate import ControlledGate
from qiskit.circuit.gate import Gate
from qiskit.circuit.quantumregister import QuantumRegister
Expand Down Expand Up @@ -568,6 +569,14 @@ def _define(self):

self.definition = qc

@deprecate_func(
additional_msg=(
"Correct exporting to OpenQASM 2 is the responsibility of a larger exporter; it cannot "
"safely be done on an object-by-object basis without context. No replacement will be "
"provided, because the premise is wrong."
),
since="0.25.0",
)
def qasm(self):
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
# Gross hack to override the Qiskit name with the name this gate has in Terra's version of
# 'qelib1.inc'. In general, the larger exporter mechanism should know about this to do the
Expand Down
29 changes: 27 additions & 2 deletions qiskit/circuit/quantumcircuit.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
from qiskit.exceptions import QiskitError, MissingOptionalLibraryError
from qiskit.utils.multiprocessing import is_main_process
from qiskit.circuit.instruction import Instruction
import qiskit.circuit.instruction
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
from qiskit.circuit.gate import Gate
from qiskit.circuit.parameter import Parameter
from qiskit.qasm.exceptions import QasmError
Expand All @@ -61,6 +62,7 @@
from .delay import Delay
from .measure import Measure
from .reset import Reset
from .tools import pi_check

try:
import pygments
Expand Down Expand Up @@ -1646,6 +1648,7 @@ def qasm(
MissingOptionalLibraryError: If pygments is not installed and ``formatted`` is
``True``.
QasmError: If circuit has free parameters.
CircuitError: If an operation that has no OpenQASM 2 representation is encountered.
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
"""

if self.num_parameters > 0:
Expand Down Expand Up @@ -1746,7 +1749,7 @@ def qasm(
bits_qasm = ",".join(
bit_labels[j] for j in itertools.chain(instruction.qubits, instruction.clbits)
)
instruction_qasm = f"{operation.qasm()} {bits_qasm};"
instruction_qasm = f"{_instruction_qasm2(operation)} {bits_qasm};"
instruction_calls.append(instruction_qasm)
instructions_qasm = "".join(f"{call}\n" for call in instruction_calls)
gate_definitions_qasm = "".join(f"{qasm}\n" for _, qasm in gates_to_define.values())
Expand Down Expand Up @@ -5085,7 +5088,7 @@ def _qasm2_define_custom_operation(operation, existing_gate_names, gates_to_defi
instruction.operation, existing_gate_names, gates_to_define
)
bits_qasm = ",".join(qubit_labels[q] for q in instruction.qubits)
statements.append(f"{new_operation.qasm()} {bits_qasm};")
statements.append(f"{_instruction_qasm2(new_operation)} {bits_qasm};")
body_qasm = " ".join(statements)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding "The ordering of these calls doesn't look right...". I made a couple of commits to simplify the code that show that all I've done is exchange a method call for a plain function call. However, I think this makes it clear that the call to _instruction_qasm2 could be moved inside _qasm2_define_custom_operation. This would the code a bit less complex and add a bit more encapsulation. I think this may be what you are suggesting.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, your new commits make the structure much clearer. More importantly (with regard to the "ordering" comment), they also ensure that the c3sx -> c3sqrtx also happens when the gate appears during nested gates that need definitions as well, whereas in the version I'd commented on, it didn't.

Given:

from qiskit import QuantumCircuit
from qiskit.circuit.library import C3SXGate

gate = QuantumCircuit(4, name="my_gate")
gate.append(C3SXGate(), [0, 1, 2, 3], [])

outer = QuantumCircuit(4)
outer.append(gate.to_gate(), [0, 1, 2, 3], [])

print(outer.qasm())

the output from where I commented (a2e2041) is:

OPENQASM 2.0;
include "qelib1.inc";
gate my_gate q0,q1,q2,q3 { c3sx q0,q1,q2,q3; }
qreg q[4];
my_gate q[0],q[1],q[2],q[3];

and from 6875e17, which is the current PR head, it's the correct:

OPENQASM 2.0;
include "qelib1.inc";
gate my_gate q0,q1,q2,q3 { c3sqrtx q0,q1,q2,q3; }
qreg q[4];
my_gate q[0],q[1],q[2],q[3];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the hack down to a lower level feels worse in a sense. But, it is simpler and temporarily mutating something that should not change is distasteful. I didn't realize that it is more correct in the sense of your example. It's useful to make this clear


# if an inner operation has the same name as the actual operation, it needs to be renamed
Expand Down Expand Up @@ -5199,3 +5202,25 @@ def _bit_argument_conversion_scalar(specifier, bit_sequence, bit_set, type_):
else f"Invalid bit index: '{specifier}' of type '{type(specifier)}'"
)
raise CircuitError(message)


def _instruction_qasm2(operation):
"""Return an OpenQASM 2 string for the instruction."""
if operation.name == "c3sx":
name_param = "c3sqrtx"
else:
name_param = operation.name
if operation.params:
name_param = "{}({})".format(
name_param,
",".join([pi_check(i, output="qasm", eps=1e-12) for i in operation.params]),
)
if operation.condition is not None:
if not isinstance(operation.condition[0], ClassicalRegister):
raise QasmError(
"OpenQASM 2 can only condition on registers, but got '{operation.condition[0]}'"
)
name_param = (
"if(%s==%d) " % (operation.condition[0].name, operation.condition[1]) + name_param
)
return name_param
4 changes: 4 additions & 0 deletions qiskit/extensions/hamiltonian_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from qiskit.quantum_info.operators.predicates import is_hermitian_matrix
from qiskit.extensions.exceptions import ExtensionError
from qiskit.circuit.exceptions import CircuitError
from qiskit.utils.deprecation import deprecate_func

from .unitary import UnitaryGate

Expand Down Expand Up @@ -116,6 +117,9 @@ def _define(self):
qc._append(UnitaryGate(self.to_matrix()), q[:], [])
self.definition = qc

@deprecate_func(
since="0.25.0",
)
def qasm(self):
"""Raise an error, as QASM is not defined for the HamiltonianGate."""
raise ExtensionError("HamiltonianGate has no QASM definition.")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
deprecations:
- |
The method :meth:`~qiskit.circuit.Instruction.qasm` and all overriding methods of subclasses
of `:meth:`~qiskit.circuit.Instruction` are deprecated. There is no replacement for generating
an OpenQASM 2 string for an isolated instruction.
5 changes: 5 additions & 0 deletions test/python/circuit/test_circuit_qasm.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,11 @@ def test_c3sxgate_roundtrips(self):
parsed = QuantumCircuit.from_qasm_str(qasm)
self.assertIsInstance(parsed.data[0].operation, C3SXGate)

def test_c3sxgate_qasm_deprecation_warning(self):
"""Test deprecation warning for C3SXGate."""
with self.assertWarnsRegex(DeprecationWarning, r"Correct exporting to OpenQASM 2"):
C3SXGate().qasm()

jakelishman marked this conversation as resolved.
Show resolved Hide resolved
def test_cczgate_qasm(self):
"""Test that CCZ dumps definition as a non-qelib1 gate."""
qc = QuantumCircuit(3)
Expand Down
13 changes: 5 additions & 8 deletions test/python/circuit/test_hamiltonian_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,13 @@ def test_1q_hamiltonian(self):
self.assertEqual(dnode.qargs, tuple(qc.qubits))
assert_allclose(dnode.op.to_matrix(), np.eye(2))

def test_error_on_qasm(self):
"""test that an error is thrown if qc.qasm() is called."""
qr = QuantumRegister(1, "q0")
cr = ClassicalRegister(1, "c0")
qc = QuantumCircuit(qr, cr)
def test_error_and_deprecation_warning_on_qasm(self):
"""test that an error is thrown if the method `qasm` is called."""
matrix = np.zeros((2, 2))
qc.hamiltonian(operator=matrix, time=1, qubits=qr[0])

hamiltonian_gate = HamiltonianGate(data=matrix, time=1)
with self.assertRaises(ExtensionError):
qc.qasm()
with self.assertWarns(DeprecationWarning):
hamiltonian_gate.qasm()

def test_2q_hamiltonian(self):
"""test 2 qubit hamiltonian"""
Expand Down
9 changes: 9 additions & 0 deletions test/python/circuit/test_instructions.py
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,15 @@ def test_label_type_enforcement(self):
instruction = HGate()
instruction.label = 0

def test_deprecation_warnings_qasm_methods(self):
"""Test deprecation warnings for qasm methods."""
with self.subTest("built in gates"):
with self.assertWarnsRegex(DeprecationWarning, r"Correct exporting to OpenQASM 2"):
HGate().qasm()
with self.subTest("User constructed Instruction"):
with self.assertWarnsRegex(DeprecationWarning, r"Correct exporting to OpenQASM 2"):
Instruction("v", 1, 0, [0.4, 0.5, 0.5]).qasm()


if __name__ == "__main__":
unittest.main()