QuantumCircuit producing a wrong OpenQASM

[francabrera]

Environment

• Qiskit Terra version: 0.24.0
• Python version: 3.10.8
• Operating system: IBM Quantum Lab

What is happening?

A QuantumCircuit is producing an invalid OpenQASM 2.0 output when calling .qasm() method. There is a definition of a subroutine calling itself.

How can we reproduce the issue?

import random
from qiskit.quantum_info import Statevector
from qiskit.algorithms import AmplificationProblem, Grover

secret = random.randint(0, 7)  # the owner is randomly picked
secret_string = format(secret, "03b")  # format the owner in 3-bit string
oracle = Statevector.from_label(secret_string)  # let the oracle know the owner
problem = AmplificationProblem(oracle, is_good_state=secret_string)
grover = Grover(iterations=1)
circuit = grover.construct_circuit(problem)

circuit.qasm()

Invalid OpenQASM output:

OPENQASM 2.0;
include "qelib1.inc";
gate circuit_136 q0,q1,q2 { rz(pi/4) q0; cx q1,q0; rz(pi/4) q0; cx q2,q0; rz(pi/4) q0; cx q1,q0; rz(pi/4) q0; cx q2,q0; }
gate ucrz(param0,param1,param2,param3) q0,q1,q2 { circuit_136 q0,q1,q2; }
gate circuit_140 q0,q1 { rz(-pi/4) q0; cx q1,q0; rz(-pi/4) q0; cx q1,q0; }
gate ucrz_139619649377696(param0,param1) q0,q1 { circuit_140 q0,q1; }
gate circuit_144 q0 { rz(-pi/4) q0; }
gate ucrz_139619649377840(param0) q0 { circuit_144 q0; }
gate gate_Diagonal q0,q1,q2 { ucrz(pi,0,0,0) q0,q1,q2; ucrz_139619649377696(-pi/2,0) q1,q2; ucrz_139619649377840(-pi/4) q2; }
gate gate_Q q0,q1,q2 { gate_Q q0,q1,q2; }
qreg q[3];
h q[0];
h q[1];
h q[2];
gate_Q q[0],q[1],q[2];

gate gate_Q q0,q1,q2 { gate_Q q0,q1,q2; } <- this is the error line

What should happen?

It shouldn't produce a gate subroutine calling itself.

Any suggestions?

No response

[Dpbm]

Hey @francabrera, I've spent some time in this issue, and I think that I found the problem.

So, going deep into the .qasm() function, I found that it iterates for each operation in the circuit:

# gates from your example
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='Q', num_qubits=3, num_clbits=0, params=[])

and as well, it iterates for all internal circuits for each operation:

Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])  # the `h` gates are ignored 
Instruction(name='Q', num_qubits=3, num_clbits=0, params=[]) 

...

Instruction(name='Q', num_qubits=3, num_clbits=0, params=[]) # it was inside  the other Q Instruction

...

Instruction(name='Diagonal', num_qubits=3, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='x', num_qubits=1, num_clbits=0, params=[])
Instruction(name='x', num_qubits=1, num_clbits=0, params=[])
Instruction(name='x', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='ccx', num_qubits=3, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='x', num_qubits=1, num_clbits=0, params=[])
Instruction(name='x', num_qubits=1, num_clbits=0, params=[])
Instruction(name='x', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])

etc...

after that, the function insert the relative qasm in a dictonary.
Note: It defines the last gate and goes backward until the first gate.

The problem is:

when the function reaches the second Q instruction

Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])
Instruction(name='h', num_qubits=1, num_clbits=0, params=[])  
Instruction(name='Q', num_qubits=3, num_clbits=0, params=[]) 


Instruction(name='Q', num_qubits=3, num_clbits=0, params=[]) # this one

it defines the gate in the correct way:

gate gate_Q q0,q1,q2 { gate_Diagonal_140304252877888 q0,q1,q2; h q2; h q1; h q0; x q0; x q1; x q2; h q2; ccx q0,q1,q2; h q2; x q0; x q1; x q2; h q0; h q1; h q2; }

However, the controller dictionary, gates_to_define, doesn't update, once the internal function _qasm2_define_custom_operation (which defines these gates) acts like a recursive function. Therefore, when the first Q operation is written, there's no way to it understands that there's another defined gate with the same name, so when this is inserted into the dictionary, the correct one is overwritten by:

gate gate_Q q0,q1,q2 { gate_Q q0,q1,q2; }

Fix

My way to fix it, is changing internally _qasm2_define_custom_operation function to give for every gate an id.
To do that, I've changed this:

if operation.name in gates_to_define:
    new_name = f"{operation.name}_{id(operation)}"
    operation = operation.copy(name=new_name)
else:
        new_name = operation.name

to this:

new_name = f"{operation.name}_{id(operation)}"
operation = operation.copy(name=new_name)

After these changes, the result was:

OPENQASM 2.0;
include "qelib1.inc";
gate circuit_973_140304251687264 q0,q1,q2 { rz(-pi/4) q0; cx q1,q0; rz(-pi/4) q0; cx q2,q0; rz(pi/4) q0; cx q1,q0; rz(pi/4) q0; cx q2,q0; }
gate ucrz_140304252416832(param0,param1,param2,param3) q0,q1,q2 { circuit_973_140304251687264 q0,q1,q2; }
gate circuit_977_140304251747584 q0,q1 { rz(-pi/4) q0; cx q1,q0; rz(pi/4) q0; cx q1,q0; }
gate ucrz_140304252417408(param0,param1) q0,q1 { circuit_977_140304251747584 q0,q1; }
gate circuit_981_140304201189936 q0 { rz(pi/4) q0; }
gate ucrz_140304252417600(param0) q0 { circuit_981_140304201189936 q0; }
gate gate_Diagonal_140304252877888 q0,q1,q2 { ucrz_140304252416832(0,0,-pi,0) q0,q1,q2; ucrz_140304252417408(0,-pi/2) q1,q2; ucrz_140304252417600(pi/4) q2; }
gate gate_Q_140304201506432 q0,q1,q2 { gate_Diagonal_140304252877888 q0,q1,q2; h q2; h q1; h q0; x q0; x q1; x q2; h q2; ccx q0,q1,q2; h q2; x q0; x q1; x q2; h q0; h q1; h q2; }
gate gate_Q_140304201506528 q0,q1,q2 { gate_Q_140304201506432 q0,q1,q2; }
qreg q[3];
h q[0];
h q[1];
h q[2];
gate_Q_140304201506528 q[0],q[1],q[2];

there might be a better way to fix it, but, for now, that was my solution.

---

If you want to see my tests, you can check it here: MyTests

[Dpbm]

Update, I've tested a different way to fix.
If you check if the gate's name is in the dictionary before adding into it, you can fix the problem too

for instruction in parameterized_definition.data:
            new_operation = _qasm2_define_custom_operation(
                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};")
        
        body_qasm = " ".join(statements)
        if operation.name in gates_to_define:
            new_name = f"{operation.name}_{id(operation)}"
            operation = operation.copy(name=new_name)

However, I think that this way is a little bit ugly, since we're repeating and there's no coherence in the code. Maybe creating another function to apply the gate's name with a descriptive name, we can fix that.

On the other hand, this way can maintain the most part of the tests passing.

[jakelishman]

A problem here is that GroverOperator is breaking the Qiskit data model, really. The name field is supposed to be a unique identifier, and decompositions cannot be circular. If there are two non-parametrised instructions with the same name, we're within our data model to assume that they represent exactly the same operation (and in fact, our model of mapping to hardware requires that we assume this), and consequently the algorithm definition is circular.

The OQ2 exporter could maybe detect that case and emit an error complaining about the data model, since it shouldn't output invalid OQ2 (circular gate definitions are invalid), but to me, the real problem is GroverOperator is stateful but isn't representing that in any way that general consumers can reason about.

edit: well, more specifically, it's a problem that it defines its internal gate with the exact same name that it gives itself.

[jakelishman]

Fixed by #10286.