collect_1q_runs should not collect atomic ops

[ajavadia]

https://github.com/Qiskit/qiskit-terra/blob/5a1768f8a0522513ca6ec2a93b36752f103b18ba/qiskit/dagcircuit/dagcircuit.py#L1380

This function was introduced recently in #5431. It currently has some filters (e.g. don't collect conditional ops or ops that act on classical bits). These filter conditions are necessary to avoid collecting nodes which cannot be simulated to a unitary, then resynthesized later. But some things currently pass through this, notably delay and reset instructions. A quick fix would be to add these to the filter. But also a more elegant solution probably exists to formally define what kind of ops are atomic (i.e. don't have a def in terms of other gates).

[paolob67]

Hi, I took a look at this one. Not sure if this is the correct way to solve it.

diff --git a/qiskit/dagcircuit/dagcircuit.py b/qiskit/dagcircuit/dagcircuit.py
index 61c9e152..c341acd7 100644
--- a/qiskit/dagcircuit/dagcircuit.py
+++ b/qiskit/dagcircuit/dagcircuit.py
@@ -1381,9 +1381,12 @@ class DAGCircuit:
         """Return a set of non-conditional runs of 1q "op" nodes."""
 
         def filter_fn(node):
+            # issue 5543 start        
             return node.type == 'op' and len(node.qargs) == 1 \
                 and len(node.cargs) == 0 and node.condition is None \
                 and not node.op.is_parameterized() \
+                and not len(node.op.decompositions) == 0 \
+            # issue 5543 end
 
         group_list = rx.collect_runs(self._multi_graph, filter_fn)
         return set(tuple(x) for x in group_list)

With the above patch I see delay() and reset() not being collected

from qiskit import QuantumCircuit
from qiskit.dagcircuit.dagnode import DAGNode
from qiskit.converters import *

qc = QuantumCircuit(1, 1)
qc.reset(0)
qc.h(0)
qc.x(0)
qc.delay(100)
qc.measure(0, 0)

dagqc = circuit_to_dag(qc)

the_runs = dagqc.collect_1q_runs()

for the_ops in the_runs:
    for the_op in the_ops:
        print(the_op.name)

qc.draw()

h
x
          ┌───┐┌───┐┌────────────────┐┌─┐
q_0: ─|0>─┤ H ├┤ X ├┤ DELAY(100[dt]) ├┤M├
          └───┘└───┘└────────────────┘└╥┘
c: 1/══════════════════════════════════╩═
                                       0 

[mtreinish]

@ajavadia would it be sufficient to add an isinstance(node.op, Gate) to the filter function? Or are there cases where we'll have custom instructions which can be defined as other gates that aren't a Gate subclass?

[taalexander]

I am also running into this with the Delay instruction after updating the mock backends in #5577.

[taalexander]

@ajavadia would it be sufficient to add an isinstance(node.op, Gate) to the filter function? Or are there cases where we'll have custom instructions which can be defined as other gates that aren't a Gate subclass?

#5570 seems like a solution that may enable the latter case as well.

[kdk]

Updating the filter function to cover all the cases here will be a little tricky. Ultimately, the behavior we'd like is something like "collect every 1q instruction that's unitary/that we can join together with Operator, remembering to break at Barriers."

#5570 is a step in the right direction, but Operator looks to Instruction.to_matrix and Instruction.definition, so a custom 1q gate would end up left out of the optimization.

@ajavadia would it be sufficient to add an isinstance(node.op, Gate) to the filter function? Or are there cases where we'll have custom instructions which can be defined as other gates that aren't a Gate subclass?

Likewise, isinstance(node.op, Gate) would miss 1q circuits converted to instructions, and incorrectly include 1q opaque gates.

I think the best path forward here is to update #5570 to check isinstance(node.op, Gate) and hasattr(node.op, '__array__') (to resolve #5556 and unblock #5577) and leave this issue open to discuss a more robust way for the transpiler to identify atomic or non-unitary instructions.