-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve CSPLayout performance by reducing constraint complexity #6263
Conversation
Co-authored-by: Christopher Zachow <[email protected]>
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.
Overall this LGTM, just a couple inline suggestions nothing big though. Also I think there is a missing feature from retworkx that this exposes if you could open an issue about it there too that would be great.
@@ -109,6 +136,10 @@ def run(self, dag): | |||
qubits.index(gate.qargs[1]))) | |||
edges = set(self.coupling_map.get_edges()) | |||
|
|||
if not self.strict_direction: | |||
cxs = {frozenset(cx) for cx in cxs} |
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 feel unnecessary as you end up iterating over the set of two qubit ops twice. Why not just have the if block around the loop and do this iteratively as you build cx?
@@ -109,6 +136,10 @@ def run(self, dag): | |||
qubits.index(gate.qargs[1]))) | |||
edges = set(self.coupling_map.get_edges()) | |||
|
|||
if not self.strict_direction: | |||
cxs = {frozenset(cx) for cx in cxs} | |||
edges = {frozenset(edge) for edge in edges} |
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.
Can you open a retworkx issue here on this https://github.com/Qiskit/retworkx/issues/new/choose I feel like there should be a native method or function on the PyDiGraph
class that CouplingMap
wraps to return the set of weakly connected edges for a graph. It'd be a bit faster too if that method exists (you might be able to hack it together inside couplingmap today with .to_undirected
https://qiskit.org/documentation/retworkx/stubs/retworkx.PyDiGraph.to_undirected.html#retworkx.PyDiGraph.to_undirected but having a native method would be best.
@@ -65,6 +66,32 @@ def recursiveBacktracking(self, # pylint: disable=invalid-name | |||
single) | |||
|
|||
|
|||
class CustomConstraint(Constraint): |
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.
Maybe rename this to be a bit more descriptive, something like ValidEdgeConstrait
or something like that.
With VF2 taking traction, I think this PR can be closed. |
Based on ideas after talking with @czachow, it was possible to specialised the
CSPLayout
pass and to reduce the complexity of the constraints to check. This gives an improvement in the performance. How much? It varies a lot, depending on the specific situation. Here is an example (based on #5694) where the goal is to find the perfect solution inToronto
(100 times, to amplify the result):The current result is: 342.403 sec
With this PR:
40.175241.838 sec (see #6263 (comment))