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

jlapeyre · 2023-07-06T17:39:23Z

This PR deprecates all methods named qasm for Instruction and subclasses. Instead the OpenQASM 2 strings are produced in QuantumCircuit.qasm.

~~This does little more than remove the method Instruction.qasm and fix up all the places that call it to preserve existing behavior.~~

~~Note that this method has been removed, rather than deprecated. In fact this new code relies on hasattr not finding this method. When deprecating, this would have to be fixed up.~~

EDIT: The items below have been added to this PR

Looking at

Consolidate and deprecate Instruction.qasm and friends under QuantumCircuit.qasm #10361

and related discussion, it seems that what we really want is to push this PR further by removing all methods called qasm from subclasses of Instruction.

coveralls · 2023-07-06T18:13:38Z

Pull Request Test Coverage Report for Build 5595206875

28 of 29 (96.55%) changed or added relevant lines in 4 files are covered.
10 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.02%) to 86.073%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/quantumcircuit.py	22	23	95.65%

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/hamiltonian_gate.py	2	86.76%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/qasm2/src/lex.rs	3	90.13%
qiskit/circuit/instruction.py	3	94.55%

Totals
Change from base Build 5594502856:	0.02%
Covered Lines:	72689
Relevant Lines:	84450

💛 - Coveralls

* Deprecate all `qasm` methods for Instruction and subclasses. * Move code for generating OpenQASM 2 strings to `QuantumCircuit`. * Test that deprecation warnings are raised. * Add release note.

qiskit-bot · 2023-07-07T00:49:37Z

One or more of the the following people are requested to review this:

@Cryoris
@Qiskit/terra-core
@ajavadia

jakelishman

I'll be glad to see the backs of these methods - one more place ticked off the list of instructions examining their params state. Thanks for looking at it.

qiskit/circuit/library/standard_gates/x.py

qiskit/circuit/quantumcircuit.py

jakelishman · 2023-07-13T17:27:12Z

qiskit/circuit/quantumcircuit.py

                operation = _qasm2_define_custom_operation(
                    operation, existing_gate_names, gates_to_define
                )
-
                # Insert qasm representation of the original instruction
+                if operation.name == "c3sx":
+                    operation.name = "c3sqrtx"
+                name_param = _instruction_qasm2(operation)
+                if operation.name == "c3sqrtx":
+                    operation.name = "c3sx"
                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"{name_param} {bits_qasm};"


The ordering of these calls doesn't look right - _qasm2_define_custom_operation is what actually defines the operation and it's true recursive. The special handling should be in there if anything - it already has logic to transform one operation into another for use as the definition and call point.

See #10399 (review) .

The ordering problem I meant in this comment was that there was logic to handle the rename of c3sx that only occurred at the top level QuantumCircuit.qasm, and it didn't appear in _qasm2_define_custom_operation, which meant that there was a discrepancy.

I think mostly I'm realising that the current system is pretty broken, and it's just that your refactor makes that fact a lot more obvious because the code's much clearer to follow. For example, this code results in a duplicate definition of c3sqrtx:

from qiskit import QuantumCircuit from qiskit.circuit.library import C3SXGate gate = QuantumCircuit(4, name="c3sqrtx").to_gate() outer = QuantumCircuit(4) outer.append(gate, [0, 1, 2, 3], []) print(outer.qasm())

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

since c3sqrtx is defined in Terra's custom qelib1.inc.

That's a pre-existing problem, though, so let's leave it during this refactor, and fix it separately.

qiskit/circuit/quantumcircuit.py

qiskit/extensions/hamiltonian_gate.py

test/python/circuit/test_circuit_qasm.py

qiskit/extensions/hamiltonian_gate.py

In x.py in class C3SXGate the method `qasm` has been deprecated. It's ok and simpler to deprecated code in this method. The two deprecated pieces will be removed together.

`for (param, value) in ...` was replaced with `for param, value in ...`. This commit reverts that change.

…on message * The method has_qasm2 was added in a previous commit (not in main). It is removed with this commit. * An additional message was added to the deprecation warning for HamiltonianGate.qasm. It is removed with this commit * A test has been updated to reflect these changes.

c3sqrtx is the name we need to use with OpenQASM2 for qiskit's c3sx. This makes the hack a bit less wordy and perhaps more palatble. The gate's name is no longer changed before calling a function on it, then changed back.

PR 10399 (not merged) changed a method call inside a Python f-string statement into a call outside, introducing a variable, and use of that variable inside the f-string. This commit makes a more minimal change. Now we only replace the method call with an ordinary function call.

jlapeyre · 2023-07-14T21:42:25Z

qiskit/circuit/quantumcircuit.py

@@ -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)


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.

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];

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

jakelishman

Thanks for the changes, John. I think once this is brought up to date with the current state of main, this will be ready to go, give or take a variable renaming which isn't even very important.

qiskit/circuit/library/standard_gates/x.py

jakelishman · 2023-07-17T16:55:26Z

qiskit/circuit/quantumcircuit.py

                operation = _qasm2_define_custom_operation(
                    operation, existing_gate_names, gates_to_define
                )
-
                # Insert qasm representation of the original instruction
+                if operation.name == "c3sx":
+                    operation.name = "c3sqrtx"
+                name_param = _instruction_qasm2(operation)
+                if operation.name == "c3sqrtx":
+                    operation.name = "c3sx"
                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"{name_param} {bits_qasm};"


The ordering problem I meant in this comment was that there was logic to handle the rename of c3sx that only occurred at the top level QuantumCircuit.qasm, and it didn't appear in _qasm2_define_custom_operation, which meant that there was a discrepancy.

I think mostly I'm realising that the current system is pretty broken, and it's just that your refactor makes that fact a lot more obvious because the code's much clearer to follow. For example, this code results in a duplicate definition of c3sqrtx:

from qiskit import QuantumCircuit from qiskit.circuit.library import C3SXGate gate = QuantumCircuit(4, name="c3sqrtx").to_gate() outer = QuantumCircuit(4) outer.append(gate, [0, 1, 2, 3], []) print(outer.qasm())

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

since c3sqrtx is defined in Terra's custom qelib1.inc.

That's a pre-existing problem, though, so let's leave it during this refactor, and fix it separately.

qiskit/circuit/quantumcircuit.py

test/python/circuit/test_circuit_qasm.py

qiskit/circuit/quantumcircuit.py

jakelishman · 2023-07-17T17:23:49Z

qiskit/circuit/quantumcircuit.py

@@ -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)


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];

jlapeyre · 2023-07-17T18:19:05Z

I think it's worth making the small changes you mentioned @jakelishman.

I refactored a bit more in 275f9b3
What do you think of inlining _instruction_qasm2 into _qasm2_define_custom_operation? In general I like breaking functions into smaller parts. But I'll have to check the logic to verify it's a good place to break it.

~~At a minimum _instruction_qasm2, if it is to remain a helper function, should be relocated next to _qasm2_define_custom_operation in the source.~~

The docstring states that a CircuitError is raised if a gate cannot be converted to qasm2. But this is now handled ultimately by the qasm2 exporter.

jakelishman

One rogue import aside, this looks good to merge. Thanks for sticking with it and getting the whole thing written on short notice!

qiskit/circuit/quantumcircuit.py

jlapeyre mentioned this pull request Jul 6, 2023

Consolidate and deprecate Instruction.qasm and friends under QuantumCircuit.qasm #10361

Closed

jlapeyre changed the title ~~[WIP] Remove Instruction.qasm~~ [WIP] Remove method qasm from Instruction and all of its subclasses Jul 6, 2023

Deprecate Instruction.qasm

26a3ec6

* Deprecate all `qasm` methods for Instruction and subclasses. * Move code for generating OpenQASM 2 strings to `QuantumCircuit`. * Test that deprecation warnings are raised. * Add release note.

jlapeyre force-pushed the refactor-instruction-qasm branch from 0b961b3 to 26a3ec6 Compare July 7, 2023 00:48

jlapeyre changed the title ~~[WIP] Remove method qasm from Instruction and all of its subclasses~~ Remove method qasm from Instruction and all of its subclasses Jul 7, 2023

jlapeyre marked this pull request as ready for review July 7, 2023 00:49

jlapeyre requested a review from a team as a code owner July 7, 2023 00:49

jlapeyre added 4 commits July 6, 2023 20:49

Merge branch 'main' into refactor-instruction-qasm

cbc81ae

Fix lint errors

23fe27e

Fix still more lint errors

cc19a4d

Merge branch 'main' into refactor-instruction-qasm

a2e2041

mtreinish added this to the 0.25.0 milestone Jul 10, 2023

mtreinish assigned jakelishman Jul 10, 2023

jakelishman reviewed Jul 13, 2023

View reviewed changes

mtreinish added the Changelog: Deprecation Include in "Deprecated" section of changelog label Jul 13, 2023

jlapeyre added 6 commits July 14, 2023 14:46

Revert replacing old code with new inside deprecated method

a790c38

In x.py in class C3SXGate the method `qasm` has been deprecated. It's ok and simpler to deprecated code in this method. The two deprecated pieces will be removed together.

Revert formatting change

c062ca6

`for (param, value) in ...` was replaced with `for param, value in ...`. This commit reverts that change.

Simplify hack for name c3sqrtx

5f2d5f2

c3sqrtx is the name we need to use with OpenQASM2 for qiskit's c3sx. This makes the hack a bit less wordy and perhaps more palatble. The gate's name is no longer changed before calling a function on it, then changed back.

Restore blank line inadvertently removed

6875e17

jlapeyre commented Jul 14, 2023

View reviewed changes

jakelishman reviewed Jul 17, 2023

View reviewed changes

jlapeyre added 4 commits July 17, 2023 15:08

Factor out creation of "custom" qasm2 operation statement

275f9b3

Remove addition_message from deprecation in x.py

8961c66

Update documentation for error thrown by QuantumCircuit.qasm

f78313c

The docstring states that a CircuitError is raised if a gate cannot be converted to qasm2. But this is now handled ultimately by the qasm2 exporter.

Merge branch 'main' into refactor-instruction-qasm

6a42114

jlapeyre added 3 commits July 17, 2023 15:25

Rename variable from name_param to qasm2_call

f32737d

Update call from QasmError to QASM2ExportError

93f5232

Merge branch 'main' into refactor-instruction-qasm

4058c1a

jakelishman reviewed Jul 19, 2023

View reviewed changes

qiskit/circuit/quantumcircuit.py Outdated Show resolved Hide resolved

jlapeyre added 2 commits July 19, 2023 01:02

Remove unused import

9a2db41

Merge branch 'main' into refactor-instruction-qasm

d056b81

jakelishman approved these changes Jul 19, 2023

View reviewed changes

jakelishman added this pull request to the merge queue Jul 19, 2023

Merged via the queue into Qiskit:main with commit 539557d Jul 19, 2023

jakelishman mentioned this pull request Sep 29, 2023

Many mentions to QASM should be changed to OpenQASM 2 #9351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jlapeyre commented Jul 6, 2023 •

edited

Loading

coveralls commented Jul 6, 2023 •

edited

Loading

qiskit-bot commented Jul 7, 2023

jakelishman left a comment

jakelishman Jul 13, 2023

jlapeyre Jul 14, 2023

jakelishman Jul 17, 2023

jlapeyre Jul 14, 2023

jakelishman Jul 17, 2023

jlapeyre Jul 17, 2023

jakelishman left a comment

jakelishman Jul 17, 2023

jakelishman Jul 17, 2023

jlapeyre commented Jul 17, 2023 •

edited

Loading

jakelishman left a comment

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

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

Conversation

jlapeyre commented Jul 6, 2023 • edited Loading

coveralls commented Jul 6, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5595206875

💛 - Coveralls

qiskit-bot commented Jul 7, 2023

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlapeyre commented Jul 17, 2023 • edited Loading

jakelishman left a comment

Choose a reason for hiding this comment

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

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

jlapeyre commented Jul 6, 2023 •

edited

Loading

coveralls commented Jul 6, 2023 •

edited

Loading

jlapeyre commented Jul 17, 2023 •

edited

Loading