-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 efficiency of CouplingMap.make_symmetric #9571
Conversation
Right now the CouplingMap.make_symmetric has a quadratic overhead. It was iterating over a list of each edgess' endpoints in the graph and for each edge it was iterating over the same list again to check whether the reverse edge is present or not. The overhead for this for large coupling maps can be quite large, for example with a 10497 qubit heavy hex coupling map the time it took to run this method as part of SabreLayout's initialization was 10x slower than actually running the pass (a equally sized Bernstein-Vazirani circuit). Instead of doing an O(n) contains check inside the loop this commit updates it to use a set which will be an O(1) lookup. This should address the performance issue with this method. In the future we should leverage the native rustworkx method to do this, which is being added in: Qiskit/rustworkx#814
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
The CouplingMap add_edge() method has similar overhead to check whether nodes need to be added. However, in the case of make_symmetric we don't need this checking because both endpoints are already in the graph. This commit further improves the performance of the method by just adding the reverse edge directly to the graph via the rustworkx api instead.
Pull Request Test Coverage Report for Build 4147792761
💛 - Coveralls |
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.
Seems sensible enough, and a clear complexity win!
* Improve efficiency of CouplingMap.make_symmetric Right now the CouplingMap.make_symmetric has a quadratic overhead. It was iterating over a list of each edgess' endpoints in the graph and for each edge it was iterating over the same list again to check whether the reverse edge is present or not. The overhead for this for large coupling maps can be quite large, for example with a 10497 qubit heavy hex coupling map the time it took to run this method as part of SabreLayout's initialization was 10x slower than actually running the pass (a equally sized Bernstein-Vazirani circuit). Instead of doing an O(n) contains check inside the loop this commit updates it to use a set which will be an O(1) lookup. This should address the performance issue with this method. In the future we should leverage the native rustworkx method to do this, which is being added in: Qiskit/rustworkx#814 * Further reduce the overhead by avoiding CouplingMap.add_edge() The CouplingMap add_edge() method has similar overhead to check whether nodes need to be added. However, in the case of make_symmetric we don't need this checking because both endpoints are already in the graph. This commit further improves the performance of the method by just adding the reverse edge directly to the graph via the rustworkx api instead.
* Improve efficiency of CouplingMap.make_symmetric Right now the CouplingMap.make_symmetric has a quadratic overhead. It was iterating over a list of each edgess' endpoints in the graph and for each edge it was iterating over the same list again to check whether the reverse edge is present or not. The overhead for this for large coupling maps can be quite large, for example with a 10497 qubit heavy hex coupling map the time it took to run this method as part of SabreLayout's initialization was 10x slower than actually running the pass (a equally sized Bernstein-Vazirani circuit). Instead of doing an O(n) contains check inside the loop this commit updates it to use a set which will be an O(1) lookup. This should address the performance issue with this method. In the future we should leverage the native rustworkx method to do this, which is being added in: Qiskit/rustworkx#814 * Further reduce the overhead by avoiding CouplingMap.add_edge() The CouplingMap add_edge() method has similar overhead to check whether nodes need to be added. However, in the case of make_symmetric we don't need this checking because both endpoints are already in the graph. This commit further improves the performance of the method by just adding the reverse edge directly to the graph via the rustworkx api instead. (cherry picked from commit 5afbb0b)
* Improve efficiency of CouplingMap.make_symmetric Right now the CouplingMap.make_symmetric has a quadratic overhead. It was iterating over a list of each edgess' endpoints in the graph and for each edge it was iterating over the same list again to check whether the reverse edge is present or not. The overhead for this for large coupling maps can be quite large, for example with a 10497 qubit heavy hex coupling map the time it took to run this method as part of SabreLayout's initialization was 10x slower than actually running the pass (a equally sized Bernstein-Vazirani circuit). Instead of doing an O(n) contains check inside the loop this commit updates it to use a set which will be an O(1) lookup. This should address the performance issue with this method. In the future we should leverage the native rustworkx method to do this, which is being added in: Qiskit/rustworkx#814 * Further reduce the overhead by avoiding CouplingMap.add_edge() The CouplingMap add_edge() method has similar overhead to check whether nodes need to be added. However, in the case of make_symmetric we don't need this checking because both endpoints are already in the graph. This commit further improves the performance of the method by just adding the reverse edge directly to the graph via the rustworkx api instead. (cherry picked from commit 5afbb0b) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
Right now the CouplingMap.make_symmetric has a quadratic overhead. It was iterating over a list of each edgess' endpoints in the graph and for each edge it was iterating over the same list again to check whether the reverse edge is present or not. The overhead for this for large coupling maps can be quite large, for example with a 10497 qubit heavy hex coupling map the time it took to run this method as part of SabreLayout's initialization was ~10x slower than actually running the pass (a equally sized Bernstein-Vazirani circuit). Instead of doing an O(n) contains check inside the loop this commit updates it to use a set which will be an O(1) lookup. This should address the performance issue with this method. In the future we should leverage the native rustworkx method to do this, which is being added in: Qiskit/rustworkx#814
Details and comments