-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add fast-path construction to DAGCircuit
methods
#10753
Changes from all commits
36f0f66
51e4a8e
14b215c
20a5407
820649a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,6 @@ | |
""" | ||
from collections import OrderedDict, defaultdict, deque, namedtuple | ||
import copy | ||
import itertools | ||
import math | ||
from typing import Dict, Generator, Any, List | ||
|
||
|
@@ -569,6 +568,7 @@ def _bits_in_operation(operation): | |
Returns: | ||
Iterable[Clbit]: the :class:`.Clbit`\\ s involved. | ||
""" | ||
# If updating this, also update the fast-path checker `DAGCirucit._operation_may_have_bits`. | ||
if (condition := getattr(operation, "condition", None)) is not None: | ||
yield from condition_resources(condition).clbits | ||
if isinstance(operation, SwitchCaseOp): | ||
|
@@ -580,6 +580,22 @@ def _bits_in_operation(operation): | |
else: | ||
yield from node_resources(target).clbits | ||
|
||
@staticmethod | ||
def _operation_may_have_bits(operation) -> bool: | ||
"""Return whether a given :class:`.Operation` may contain any :class:`.Clbit` instances | ||
in itself (e.g. a control-flow operation). | ||
|
||
Args: | ||
operation (qiskit.circuit.Operation): the operation to check. | ||
""" | ||
# This is separate to `_bits_in_operation` because most of the time there won't be any bits, | ||
# so we want a fast path to be able to skip creating and testing a generator for emptiness. | ||
# | ||
# If updating this, also update `DAGCirucit._bits_in_operation`. | ||
return getattr(operation, "condition", None) is not None or isinstance( | ||
operation, SwitchCaseOp | ||
) | ||
|
||
def _increment_op(self, op): | ||
if op.name in self._op_names: | ||
self._op_names[op.name] += 1 | ||
|
@@ -592,23 +608,6 @@ def _decrement_op(self, op): | |
else: | ||
self._op_names[op.name] -= 1 | ||
|
||
def _add_op_node(self, op, qargs, cargs): | ||
"""Add a new operation node to the graph and assign properties. | ||
|
||
Args: | ||
op (qiskit.circuit.Operation): the operation associated with the DAG node | ||
qargs (list[Qubit]): list of quantum wires to attach to. | ||
cargs (list[Clbit]): list of classical wires to attach to. | ||
Returns: | ||
int: The integer node index for the new op node on the DAG | ||
""" | ||
# Add a new operation node to the graph | ||
new_node = DAGOpNode(op=op, qargs=qargs, cargs=cargs, dag=self) | ||
node_index = self._multi_graph.add_node(new_node) | ||
new_node._node_id = node_index | ||
self._increment_op(op) | ||
return node_index | ||
|
||
Comment on lines
-595
to
-611
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a fairly simple function used in only two places which already necessarily had a lot of duplication, and throws away information that the outer contexts need (the |
||
@deprecate_func( | ||
additional_msg="Instead, use :meth:`~copy_empty_like()`, which acts identically.", | ||
since="0.20.0", | ||
|
@@ -647,13 +646,18 @@ def copy_empty_like(self): | |
|
||
return target_dag | ||
|
||
def apply_operation_back(self, op, qargs=(), cargs=()): | ||
def apply_operation_back(self, op, qargs=(), cargs=(), *, check=True): | ||
"""Apply an operation to the output of the circuit. | ||
|
||
Args: | ||
op (qiskit.circuit.Operation): the operation associated with the DAG node | ||
qargs (tuple[Qubit]): qubits that op will be applied to | ||
cargs (tuple[Clbit]): cbits that op will be applied to | ||
check (bool): If ``True`` (default), this function will enforce that the | ||
:class:`.DAGCircuit` data-structure invariants are maintained (all ``qargs`` are | ||
:class:`.Qubit`\\ s, all are in the DAG, etc). If ``False``, the caller *must* | ||
uphold these invariants itself, but the cost of several checks will be skipped. | ||
This is most useful when building a new DAG from a source of known-good nodes. | ||
Returns: | ||
DAGOpNode: the node for the op that was added to the dag | ||
|
||
|
@@ -664,52 +668,74 @@ def apply_operation_back(self, op, qargs=(), cargs=()): | |
qargs = tuple(qargs) if qargs is not None else () | ||
cargs = tuple(cargs) if cargs is not None else () | ||
|
||
all_cbits = set(self._bits_in_operation(op)).union(cargs) | ||
if self._operation_may_have_bits(op): | ||
# This is the slow path; most of the time, this won't happen. | ||
all_cbits = set(self._bits_in_operation(op)).union(cargs) | ||
else: | ||
all_cbits = cargs | ||
|
||
self._check_condition(op.name, getattr(op, "condition", None)) | ||
self._check_bits(qargs, self.output_map) | ||
self._check_bits(all_cbits, self.output_map) | ||
if check: | ||
self._check_condition(op.name, getattr(op, "condition", None)) | ||
self._check_bits(qargs, self.output_map) | ||
self._check_bits(all_cbits, self.output_map) | ||
|
||
node_index = self._add_op_node(op, qargs, cargs) | ||
node = DAGOpNode(op=op, qargs=qargs, cargs=cargs, dag=self) | ||
node._node_id = self._multi_graph.add_node(node) | ||
self._increment_op(op) | ||
|
||
# Add new in-edges from predecessors of the output nodes to the | ||
# operation node while deleting the old in-edges of the output nodes | ||
# and adding new edges from the operation node to each output node | ||
|
||
al = [qargs, all_cbits] | ||
self._multi_graph.insert_node_on_in_edges_multiple( | ||
node_index, [self.output_map[q]._node_id for q in itertools.chain(*al)] | ||
node._node_id, | ||
[self.output_map[bit]._node_id for bits in (qargs, all_cbits) for bit in bits], | ||
) | ||
return self._multi_graph[node_index] | ||
return node | ||
|
||
def apply_operation_front(self, op, qargs=(), cargs=()): | ||
def apply_operation_front(self, op, qargs=(), cargs=(), *, check=True): | ||
"""Apply an operation to the input of the circuit. | ||
|
||
Args: | ||
op (qiskit.circuit.Operation): the operation associated with the DAG node | ||
qargs (tuple[Qubit]): qubits that op will be applied to | ||
cargs (tuple[Clbit]): cbits that op will be applied to | ||
check (bool): If ``True`` (default), this function will enforce that the | ||
:class:`.DAGCircuit` data-structure invariants are maintained (all ``qargs`` are | ||
:class:`.Qubit`\\ s, all are in the DAG, etc). If ``False``, the caller *must* | ||
uphold these invariants itself, but the cost of several checks will be skipped. | ||
This is most useful when building a new DAG from a source of known-good nodes. | ||
Returns: | ||
DAGOpNode: the node for the op that was added to the dag | ||
|
||
Raises: | ||
DAGCircuitError: if initial nodes connected to multiple out edges | ||
""" | ||
all_cbits = set(self._bits_in_operation(op)).union(cargs) | ||
qargs = tuple(qargs) if qargs is not None else () | ||
cargs = tuple(cargs) if cargs is not None else () | ||
|
||
if self._operation_may_have_bits(op): | ||
# This is the slow path; most of the time, this won't happen. | ||
all_cbits = set(self._bits_in_operation(op)).union(cargs) | ||
else: | ||
all_cbits = cargs | ||
|
||
self._check_condition(op.name, getattr(op, "condition", None)) | ||
self._check_bits(qargs, self.input_map) | ||
self._check_bits(all_cbits, self.input_map) | ||
node_index = self._add_op_node(op, qargs, cargs) | ||
if check: | ||
self._check_condition(op.name, getattr(op, "condition", None)) | ||
self._check_bits(qargs, self.input_map) | ||
self._check_bits(all_cbits, self.input_map) | ||
|
||
node = DAGOpNode(op=op, qargs=qargs, cargs=cargs, dag=self) | ||
node._node_id = self._multi_graph.add_node(node) | ||
self._increment_op(op) | ||
|
||
# Add new out-edges to successors of the input nodes from the | ||
# operation node while deleting the old out-edges of the input nodes | ||
# and adding new edges to the operation node from each input node | ||
al = [qargs, all_cbits] | ||
self._multi_graph.insert_node_on_out_edges_multiple( | ||
node_index, [self.input_map[q]._node_id for q in itertools.chain(*al)] | ||
node._node_id, | ||
[self.input_map[bit]._node_id for bits in (qargs, all_cbits) for bit in bits], | ||
) | ||
return self._multi_graph[node_index] | ||
return node | ||
|
||
def compose(self, other, qubits=None, clbits=None, front=False, inplace=True): | ||
"""Compose the ``other`` circuit onto the output of this circuit. | ||
|
@@ -819,7 +845,7 @@ def _reject_new_register(reg): | |
op.condition = variable_mapper.map_condition(condition, allow_reorder=True) | ||
elif isinstance(op, SwitchCaseOp): | ||
op.target = variable_mapper.map_target(op.target) | ||
dag.apply_operation_back(op, m_qargs, m_cargs) | ||
dag.apply_operation_back(op, m_qargs, m_cargs, check=False) | ||
else: | ||
raise DAGCircuitError("bad node type %s" % type(nd)) | ||
|
||
|
@@ -1191,7 +1217,7 @@ def substitute_node_with_dag(self, node, input_dag, wires=None, propagate_condit | |
node_wire_order = list(node.qargs) + list(node.cargs) | ||
# If we're not propagating it, the number of wires in the input DAG should include the | ||
# condition as well. | ||
if not propagate_condition: | ||
if not propagate_condition and self._operation_may_have_bits(node.op): | ||
node_wire_order += [ | ||
bit for bit in self._bits_in_operation(node.op) if bit not in node_cargs | ||
] | ||
|
@@ -1260,7 +1286,7 @@ def substitute_node_with_dag(self, node, input_dag, wires=None, propagate_condit | |
) | ||
new_op = copy.copy(in_node.op) | ||
new_op.condition = new_condition | ||
in_dag.apply_operation_back(new_op, in_node.qargs, in_node.cargs) | ||
in_dag.apply_operation_back(new_op, in_node.qargs, in_node.cargs, check=False) | ||
else: | ||
in_dag = input_dag | ||
|
||
|
@@ -1483,7 +1509,7 @@ def _key(x): | |
subgraph_is_classical = False | ||
if not isinstance(node, DAGOpNode): | ||
continue | ||
new_dag.apply_operation_back(node.op, node.qargs, node.cargs) | ||
new_dag.apply_operation_back(node.op, node.qargs, node.cargs, check=False) | ||
|
||
# Ignore DAGs created for empty clbits | ||
if not subgraph_is_classical: | ||
|
@@ -1801,7 +1827,7 @@ def layers(self): | |
|
||
for node in op_nodes: | ||
# this creates new DAGOpNodes in the new_layer | ||
new_layer.apply_operation_back(node.op, node.qargs, node.cargs) | ||
new_layer.apply_operation_back(node.op, node.qargs, node.cargs, check=False) | ||
|
||
# The quantum registers that have an operation in this layer. | ||
support_list = [ | ||
|
@@ -1829,7 +1855,7 @@ def serial_layers(self): | |
cargs = copy.copy(next_node.cargs) | ||
|
||
# Add node to new_layer | ||
new_layer.apply_operation_back(op, qargs, cargs) | ||
new_layer.apply_operation_back(op, qargs, cargs, check=False) | ||
# Add operation to partition | ||
if not getattr(next_node.op, "_directive", False): | ||
support_list.append(list(qargs)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This optimization is in addition to the optimization discussed in the opening comment, right? Did you benchmark this optimization? It's clear that
set(self._bits_in_operation(op)).union(cargs)
will be slower than justcargs
even when the generator is empty. But I would not be surprised if this is not measurable in benchmarks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like I didn't mention this little bit in the commit message. Removing the
_may_have_bits
check and always eagerly calling_bits_in_operation
caused the microbenchmark at the top to go from 27.7(7)ms with the current PR to 29.3(9)ms when timed just now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's worth the trouble although it adds a bit of clutter