[WIP] Create DAGDependencyV2 #11310
[WIP] Create DAGDependencyV2 #11310
Conversation
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7403861007Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
There was a problem hiding this comment.
So far I only took a superficial look at the PR and I am definitely excited about the changes: DAGDependencyV2 is on the road to become cleaner, more efficient and more functional than DAGDependency, and the classes DAGCircuit and DAGDependencyV2 start looking closer to each other. I have left a few minor in-place comments and questions, but this is not a complete review.
What significantly confuses me (and I mean qiskit code in general, not particularly this PR) is the divergence of similar methods between QuantumCircuit, DAGCircuit and DAGDependency. As the simplest example, the size function in QuantumCircuit takes the argument filter_function, the size function in DAGCircuit takes the argument recurse, and the size function in DAGDependency does not take either (the same is true for DAGDependencyV2). Not only the size function should be used differently for different representations of the quantum circuit, it's likely to return different results depending on the representation. And this is just the tip of the iceberg. It would probably be nice to have an in-depth discussion about these 3 APIs and try to find the best solution.
A related point is that the API for DAGDependency and (as of right now) DAGDependencyV2 is based on integer node ids assigned by rustworkx graphs, while the API for DAGCircuit is based on DAGNodes. It would probably be good to have the DAGDependencyV2 API also in terms of nodes and not rustworkx ids.
|
|
||
| dagdependency.calibrations = circuit.calibrations |
There was a problem hiding this comment.
This seems an oversight in the existing code as well, but shouldn't we also update the global_phase (as is done in dag_to_dagdependency_v2 code)?
There was a problem hiding this comment.
This goes to your work to coordinate the 3 classes, which involves add_calibration as well. Do these things belong in all 3 classes? And if so, can they be implemented the same way?
qiskit/dagcircuit/collect_blocks.py
Outdated
| self.dag.get_node(pred_id) | ||
| for pred_id in self.dag.direct_successors(node.node_id) | ||
| for pred_id in self.dag.get_successors(self.dag.node_map[node]) | ||
| ] |
There was a problem hiding this comment.
I have not yet looked at the internals of DAGDependencyV2, but getting the direct predecessors still significantly differs between DAGCircuit and DAGDependencyV2.
Using DAGCircuit:
[pred for pred in self.dag.predecessors(node) if isinstance(pred, DAGOpNode)]
Using DAGDependencyV2:
[self.dag.get_node(pred_id) for pred_id in self.dag.get_successors(self.dag.node_map[node])]
I am wondering if we could bring the two expressions closer.
There was a problem hiding this comment.
See _node_id reply below.
| # (C) Copyright IBM 2020. | ||
| # |
There was a problem hiding this comment.
Ha! This code was written at least 3 years ago. :)
There was a problem hiding this comment.
Actually almost 4 years ago, mostly by Romain Moyard, an author of the template matching paper.
| def add_calibration(self, gate, qubits, schedule, params=None): | ||
| """Register a low-level, custom pulse definition for the given gate. |
There was a problem hiding this comment.
This code is adapted from DAGCircuit, correct? A naive question: why should DAGDependencyV2 support this method?
| def size(self): | ||
| """Returns the number of gates in the circuit""" |
There was a problem hiding this comment.
Interesting. The size function in DAGCircuit has an additional argument recurse, while the size function in QuantumCircuit has an additional argument filter_function. Do we want to extend this function here and how?
| # Map of node to node index | ||
| self.node_map = {} |
There was a problem hiding this comment.
DAGNode/DAGOpNode have the field _node_id, which is what is used by DAGCircuit to map from a node to its rustworkx id. Can we use a similar strategy here instead of supporting node_map?
There was a problem hiding this comment.
Somehow I completely missed that _node_id was used throughout DAGCircuit. I thought it was only used minimally, so I didn't use it in DAGDependencyV2, but should have. Since it would also need to be used in the template matching code and the underscore implies some level of privacy, we'd probably want to add an @property for node_id to the DAGDependencyV2 code.
| def get_in_edges(self, node_id): | ||
| """ |
There was a problem hiding this comment.
A general comment/question about API. In this function and others, node_id is the "internal" integer id assigned by rustworkx graph, correct? Would it make sense to change the API to refer to DAGNodes (or DAGOpNodes) but not to these rustworkx ids? Is this how we already do this in DAGCircuit?
There was a problem hiding this comment.
The reason I kept it this way currently is that is the way the template matching code uses it. I should probably do a second run through all this code to convert to using node.node_id as discussed above. This should simplify a lot of the code.
| for node in self.get_nodes(): | ||
| dag._multi_graph.add_node(node.copy()) |
There was a problem hiding this comment.
When copying dag dependencies, do we always need to copy the actual nodes too? As far as I can see, DAGCircuit does not have the function copy (and maybe it should). Ok, I guess we had to copy the nodes in the old template matching code where a lot of additional information was stored on nodes (and likely this information had to change in different parts of the algorithm), but maybe now (with this additional information stripped away) we can avoid this additional copying?
Update: looking at template matching code, I see that you are ahead of this and are no longer copying DAGDependencies. Do we still need this copy function?
There was a problem hiding this comment.
I actually removed all the dag_dep copying in the template code, so the copying here is only if a user wants to use it. We definitely do not have to copy the node here.
| self.successorstovisit = {} | ||
| self.isblocked = {} | ||
| self.matchedwith = {} | ||
|
|
There was a problem hiding this comment.
Great! So these are now used instead of the information that used to be stored on DAGDepNodes. This is much much cleaner.
There was a problem hiding this comment.
The only leftover that is not purely in the template code is qindices and cindices which are computed and stored at the DAGDependencyV2 level. It's done in the add_op_node method. It's just a list of indices for the qargs and cargs for a node. It seemed easiest to store this at this level, rather than having to rebuild the list each time it's needed for a node in the template code. This can probably be optimized with a template code rework.
|
@alexanderivrii I did another pass through I added 4 new methods - I also removed several edge-related functions, such as |
Summary
Creates a new DAGDependencyV2
Details and comments
Previously
DAGDependencywas only used for the template matching transpiler pass. Additional uses are planned and there were several problems with the previous version.DAGDependencyusedDAGDepNodewhich stored a lot of information at the node level in attributes such assuccessors,qindices, andmatchedwiththat were only meant for use for template matching. These created excessive memory usage and did not take advantage of the rustworkx graph functions at runtime.DAGDependencyhad diverged significantly fromDAGCircuit, even though they share a lot of functionality in common.DAGDependencyis part of that refactoring process.The significant changes made are
DAGDependencyV2which no longer usesDAGDepNodeat all, but instead uses the existingDAGOpNodewithout modification. The makes for a cleaner and much reduced memory profile for the nodes.DAGCircuit, such asfind_bitwere added to theDAGDependencyV2and other methods were modified to more closely match the 2 dags.DAGDependencyV2and the template code.direct_successorsis nowsuccessorsandsuccessorsis nowdescendants. Similarly forpredecessorsandancestors. This mirrors the usage in rustworkx.For the future, a refactor of the template matching code is in order. One area especially is that the code mixes usage of the integer node id returned from rustworkx calls and the node object id, which results in many instances of having to map one to the other.