Add PyDiGraph method to make edges symmetric #814
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds a new method make_symmetric() to PyDiGraph which will modify the graph and add a reverse edge to each edge in the graph if it is not already present.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Feb 10, 2023
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
Pull Request Test Coverage Report for Build 4981712940
💛 - Coveralls |
mergify bot
pushed a commit
to Qiskit/qiskit
that referenced
this pull request
Feb 10, 2023
* 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.
jlapeyre
reviewed
Feb 13, 2023
nbronn
pushed a commit
to nbronn/qiskit-terra
that referenced
this pull request
Feb 13, 2023
* 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.
mergify bot
pushed a commit
to Qiskit/qiskit
that referenced
this pull request
Feb 17, 2023
* 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)
mergify bot
added a commit
to Qiskit/qiskit
that referenced
this pull request
Feb 17, 2023
* 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>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
mtreinish
changed the title
jlapeyre
reviewed
May 11, 2023
|
Other than the comment above, this looks really thorough. LGTM. |
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
enavarro51
approved these changes
May 15, 2023
|
Agreed. LGTM. |
IvanIsCoding
pushed a commit
to IvanIsCoding/rustworkx
that referenced
this pull request
May 26, 2023
* Add PyDiGraph method to make edges symmetric This commit adds a new method make_symmetric() to PyDiGraph which will modify the graph and add a reverse edge to each edge in the graph if it is not already present. * Simplyify logic * Add initial tests * Update docstring wording Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> * Add release notes * Expand testing * Fix tests and cycle checking * Remove stray debug prints * Update src/digraph.rs Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> * Fix lint --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: Edwin Navarro <enavarro@comcast.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds a new method make_symmetric() to PyDiGraph which will modify the graph and add a reverse edge to each edge in the graph if it is not already present.
TODO: