Add CircuitDag class #759
Add CircuitDag class #759
Conversation
cduck
force-pushed
the
circuitdag
branch
2 times, most recently
from
85730d1 to
7105189
Compare
|
|
||
| T = TypeVar('T') | ||
|
|
||
| class Unique(Generic[T]): |
There was a problem hiding this comment.
Add a docstring explaining why this is needed.
cirq/circuits/circuit_dag.py
Outdated
| TSelf = TypeVar('TSelf', bound='CircuitDagAbc') | ||
|
|
||
| class CircuitDagAbc(networkx.DiGraph, metaclass=abc.ABCMeta): | ||
| """A representation of a Circuit as a directed acyclic graph. Subclass |
There was a problem hiding this comment.
Move the second sentence into a separate paragraph.
cirq/circuits/circuit_dag.py
Outdated
|
|
||
| class CircuitDagAbc(networkx.DiGraph, metaclass=abc.ABCMeta): | ||
| """A representation of a Circuit as a directed acyclic graph. Subclass | ||
| CircuitDagAbc and implement can_reorder().""" |
There was a problem hiding this comment.
Explain the goal achieved by subclassing and implementing the method.
cirq/circuits/circuit_dag.py
Outdated
| super().__init__(incoming_graph_data) | ||
| self.device = device | ||
|
|
||
| @abc.abstractmethod |
There was a problem hiding this comment.
I think, given that this is a "data structure class", it makes more sense to tell it how to do things by providing constructor arguments rather than via inheritance. For example, the way you use defaultdict is not by subclassing it and implementing a make_value method; you give the constructor a make_value lambda. Do that.
cirq/circuits/circuit_dag.py
Outdated
| return get_root_node(next(iter(g.nodes))) | ||
|
|
||
| def get_next_node(succ): | ||
| if len(succ) > 0: |
There was a problem hiding this comment.
Simplify to if len(succ) or if succ.
cirq/circuits/circuit_dag.py
Outdated
| def get_first_node(): | ||
| return get_root_node(next(iter(g.nodes))) | ||
|
|
||
| def get_next_node(succ): |
There was a problem hiding this comment.
What's the type of succ and the return type?
cirq/circuits/circuit_dag.py
Outdated
| succ = g.succ[node] | ||
| g.remove_node(node) | ||
|
|
||
| if len(g.nodes) <= 0: |
There was a problem hiding this comment.
Simplify to if not len(g.nodes) or if not g.nodes if possible.
cirq/circuits/circuit_dag.py
Outdated
|
|
||
| def get_root_node(some_node): | ||
| pred = g.pred | ||
| while len(pred[some_node]) > 0: |
There was a problem hiding this comment.
Simplify to if len(pred[some_node]) or if pred[some_node].
|
I rewrote many of the docstrings also. |
cirq/circuits/circuit_dag.py
Outdated
| TSelf = TypeVar('TSelf', bound='CircuitDag') | ||
|
|
||
|
|
||
| def _default_can_reorder(op1: ops.Operation, op2: ops.Operation) -> bool: |
There was a problem hiding this comment.
Call this "_disjoint_qubits" or similar so that people reading the documentation of the method using this as a default value have a description of what it does instead of how it's used.
|
|
||
| Edges of the graph are tuples of nodes. Each edge specifies a required | ||
| application order between two operations. The first must be applied before | ||
| the second. |
There was a problem hiding this comment.
Maybe mention whether the graph is minimalist (e.g. the transitive reduction), maximalist (transitive completion), or just somewhere arbitrary in between.
cirq/circuits/circuit_dag.py
Outdated
| the second. | ||
| """ | ||
|
|
||
| default_can_reorder = staticmethod(_default_can_reorder) |
There was a problem hiding this comment.
This is a class attribute so that people can use it for themselves?
cirq/circuits/circuit_dag.py
Outdated
| device=circuit.device) | ||
|
|
||
| @classmethod | ||
| def from_ops(cls: Type[TSelf], |
There was a problem hiding this comment.
Is it necessary for these to be class methods anymore, now that there is no inheritance present?
cirq/circuits/circuit_dag.py
Outdated
| @@ -56,13 +56,15 @@ class CircuitDag(networkx.DiGraph): | |||
| Edges of the graph are tuples of nodes. Each edge specifies a required | |||
| application order between two operations. The first must be applied before | |||
| the second. | |||
|
|
|||
| The graph is maximalist (transitive completion). | |||
There was a problem hiding this comment.
I know I used it, but I don't think maximalist is a common term.
Resolves #547.
Add networkx as a dependency.