Using simulator instructions breaks latex circuit drawer

[ajavadia]

What is the current behavior?

Using circuit simulator instructions (such as snapshot) cause those instructions to be added to the basis of the DAG, which in turn cause the circuit drawer to raise:

circuit_drawer(qc).show()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/salva/workspace/qiskit-sdk-py/qiskit/tools/visualization/_circuit_visualization.py", line 77, in circuit_drawer
    return latex_circuit_drawer(circuit, basis, scale, filename)
  File "/Users/salva/workspace/qiskit-sdk-py/qiskit/tools/visualization/_circuit_visualization.py", line 105, in latex_circuit_drawer
    generate_latex_source(circuit, filename=tmppath, basis=basis, scale=scale)
  File "/Users/salva/workspace/qiskit-sdk-py/qiskit/tools/visualization/_circuit_visualization.py", line 177, in generate_latex_source
    ast = Qasm(data=circuit.qasm()).parse()
  File "/Users/salva/workspace/qiskit-sdk-py/qiskit/qasm/_qasm.py", line 49, in parse
    return qasm_p.parse(self._data)
  File "/Users/salva/workspace/qiskit-sdk-py/qiskit/qasm/_qasmparser.py", line 1065, in parse
    self.parser.parse(data, lexer=self.lexer, debug=self.parse_deb)
  File "/Users/salva/workspace/qiskit-sdk-py/.venv/lib/python3.6/site-packages/ply/yacc.py", line 333, in parse
    return self.parseopt_notrack(input, lexer, debug, tracking, tokenfunc)
  File "/Users/salva/workspace/qiskit-sdk-py/.venv/lib/python3.6/site-packages/ply/yacc.py", line 1120, in parseopt_notrack
    p.callable(pslice)
  File "/Users/salva/workspace/qiskit-sdk-py/qiskit/qasm/_qasmparser.py", line 651, in p_unitary_op_4
    self.verify_as_gate(program[1], program[5], arglist=program[3])
  File "/Users/salva/workspace/qiskit-sdk-py/qiskit/qasm/_qasmparser.py", line 126, in verify_as_gate
    + "', line", str(obj.line), 'file', obj.file)
qiskit.qasm._qasmerror.QasmError: "Cannot find gate definition for 'snapshot', line 6 file "

Steps to reproduce the problem

from qiskit import QuantumRegister, ClassicalRegister, QuantumCircuit
from qiskit.tools.visualization import circuit_drawer
from qiskit.extensions import simulator
q = QuantumRegister(1)
c = ClassicalRegister(1)
qc = QuantumCircuit(q, c)
qc.h(q)
qc.snapshot('0')
qc.measure(q, c)
circuit_drawer(qc).show()

Expected behavior

[jaygambetta]

Also for the experiment.

[delapuente]

@ajavadia can you provide the steps to reproduce and the expected behavior, please?

[jaygambetta]

Since is in my bug I will. But it is pretty clear from the description.

from qiskit import QuantumRegister, ClassicalRegister, QuantumCircuit
from qiskit.tools.visualization import circuit_drawer
q = QuantumRegister(1)
c = ClassicalRegister(1)
qc = QuantumCircuit(q, c)
qc.h(q)
qc.snapshot('0')
qc.measure(q, c)
circuit_drawer(qc).show()

[delapuente]

@jaygambetta this is the output I'm getting, since I and other contributors could not be used to quantum circuits representation, what's the expected behaviour?

[jaygambetta]

hmm no, it crashes. but I think I know why. In your corrections to my code could you add

from qiskit import QuantumRegister, ClassicalRegister, QuantumCircuit
from qiskit.tools.visualization import circuit_drawer
from qiskit.extensions import simulator
q = QuantumRegister(1)
c = ClassicalRegister(1)
qc = QuantumCircuit(q, c)
qc.h(q)
qc.snapshot('0')
qc.measure(q, c)
circuit_drawer(qc).show()

and it should give you the error. I think your code is just ignoring the snapshot.

[jaygambetta]

QasmError: "Cannot find gate definition for 'snapshot', line 6 file "

[delapuente]

Thank you, Jay, for fixing the steps to reproduce. With all the information in place, I'm completing the summary and assigning to myself.

[delapuente]

@ajavadia what's the expected behaviour? It is "not crashing and displaying the circuit" or should the snapshot be displayed in some way?

[ajavadia]

@delapuente the expected behavior is that it should not crash. The reason for the crash seems to be that these simulator instructions get added to the basis of DAGCircuit, but not recognized by the drawer or the device experiment. Displaying the snapshot/barrier/etc. is tracked in a separate PR #731.

[delapuente]

What happens is that the QASM DAG parser cannot verify that snapshot is a gate because it is not in the global symbol table. The QASM the parser is trying to parse is:

OPENQASM 2.0;
include "qelib1.inc";
qreg q0[1];
creg c0[1];
snapshot(0) q0[0];

Which is, indeed, incorrect since snapshot is not defined. The correct one would be:

OPENQASM 2.0;
include "qelib1.inc";
opaque snapshot(n) q;
qreg q0[1];
creg c0[1];
snapshot(0) q0[0];

Notice the opaque definition.

A quick workaround is to make the qasm() method for snapshot to emit a comment like #snapshot(0) q0[0]; but this would cause the DAG not to include the snapshot node.

A more elaborated solution is adding support for the extensions to add their own definitions so the simulator extension can include the corresponding opaque.

Another solution could be to hardcode the opaque definition of snapshot in the qelib1.inc and in the basis.

For some reason, even if adding the opaque definition, the JSON unroller complains if the opaque is not in the basis.

@ajavadia @jaygambetta any preference?

[ajavadia]

@delapuente let's not change qelib1.inc.

I don't quite understand how the first solution would work. Why is this a qasm problem. There should be no qasm text generated, just circuit and dag.

Any gate not in the standard qelib1.inc should be dynamically added to the dag basis when the dag is created. The source of error here is that it is not found in the dag basis. Then I think we should teach the visualizer to plot snapshot the same way as it does barrier.

[delapuente]

The source of error here is that it is not found in the dag basis.

I think there is a misunderstanding on how circuit visualization run right now. DAGs are never used in the current implementation of visualizations. They use the JSON representation of the circuits but, instead of getting here through the DAG, they use the QASM AST: https://github.com/Qiskit/qiskit-terra/blob/9fb88b035a47eee2ad8e57ddf184049c35e31933/qiskit/tools/visualization/_circuit_visualization.py#L349-L355

Anyway @ajavadia, you gave me a clue!

Any gate not in the standard qelib1.inc should be dynamically added to the dag basis when the dag is created.

And I'm going to modify the JSON generation making it to use the DagUnroller instead of the QASM one.

[ajavadia]

Yeah that's a bad code, it's dumping qasm text, then reparsing it. Should use the DagUnroller.

[delapuente]

This is not solved as pointed out in #1019. Re-opening.

[delapuente]

What is happening is that dag_circuit = DAGCircuit.fromCircuit(...) in line 353 adds the unknown gates used in the original circuit to the basis without extending them, which is the desired behaviour when you want the drawer to depict your circuit with accuracy (i.e, not expanding custom gates).

But, just after, the instruction json_circuit = transpile(...) in line 354 passes the basis passed as parameter which defaults in "id,u0,u1,u2,u3,x,y,z,h,s,sdg,t,tdg,rx,ry,rz,cx,cy,cz,ch,crz,cu1,cu3,swap,ccx,cswap", provided in the lines 338 and 339 and not including the simulator instructions.

https://github.com/Qiskit/qiskit-terra/blob/596fec0aa58bdda524472d4a749b227fe7ff7a39/qiskit/tools/visualization/_circuit_visualization.py#L337-L354

If the code is changed to use the basis from the dag_circuit, functionality works but the basis parameter is ignored.

basis = ','.join(dag_circuit.basis.keys())
json_circuit = transpile(..., basis=basis, ...)

Ideally, in my opinion, we should drop the basis parameter for the circuit drawer should be limited to draw the circuit without doing any further transformation. As noted in https://github.com/Qiskit/qiskit-terra/pull/885#issuecomment-420524997, if the user wants to compile for a different basis, they should convert their circuit into a different basis, then draw the new circuit.

Unfortunately, the DAG is already coupled with the simulator instructions and treat them in a special way:

https://github.com/Qiskit/qiskit-terra/blob/596fec0aa58bdda524472d4a749b227fe7ff7a39/qiskit/dagcircuit/_dagcircuit.py#L1382-L1388

In order to not introduce more coupling, my proposal would be to add the simulator instructions to the basis before calling transpile(...). This introduces non-breaking changes and it is similar to the proposed solution without embedding the DAG instructions into the DAG unroller.

I would not hardcode the simulator instructions into the DAG unroller (nor in the DAG, to be honest) since they are not built-ins (but extensions, indeed). What should be done is to provide a proper extension mechanism for the DAG.