Use retworkx instead of networkx for noise adaptive layout #5117
Conversation
This file contains hidden or 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 updates the nosie adaptive layout transpiler pass to use retworkx for its internal graph operations. Prior to this commit it was constructing 2 networkx graphs internally. These have been replaced with equivalent structures in retworkx. At the same time some of the algorithms used were generating extraneous information that was never used this is removed.
1ucian0
previously approved these changes
Sep 23, 2020
|
'on hold' for a possible performance regression @mtreinish was looking into. |
|
I've been testing this locally I still think there might be a weird edge case and something else going on. But after applying 044ff1d I used this script to benchmark the performance: import time
from qiskit.circuit.library import QuantumVolume
from qiskit.converters import circuit_to_dag
from qiskit.test.mock.backends import FakeManhattan
from qiskit.transpiler.passes import NoiseAdaptiveLayout
seed = 42
circuit = QuantumVolume(65, 65, seed=42)
circuit.measure_all()
fresh_dag = circuit_to_dag(circuit)
backend_props = FakeManhattan().properties()
start = time.time()
NoiseAdaptiveLayout(backend_props).run(fresh_dag)
stop = time.time()
print("Total: %s" % str(stop - start))On my laptop with this PR it took ~.0092 seconds with this PR. Without this PR it took ~0.028 seconds. So I think 044ff1d fixed the issue (don't ask me why though, it's still confusing to me that dict access would be faster) |
|
Here are the timings I found locally for @mtreinish 's script: |
kdk
approved these changes
Sep 25, 2020
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Sep 29, 2020
This commit fixes a performance regression introduced in Qiskit#5117 when there were a large number of gates in the circuit being run through the pass. The overhead of calling adj() a significant number of times was adding a runtime penalty. Its not clear what is causing this with retworkx but my assumption is it is related to going back and forth between rust and python. This commit mitigates most of this overhead by locally caching the list of neighbors each time we call _select_best_remaining_qubit().
mergify bot
pushed a commit
that referenced
this pull request
Oct 6, 2020
* Avoid excessive neighbor discovery in NoiseAdaptiveLayout This commit fixes a performance regression introduced in #5117 when there were a large number of gates in the circuit being run through the pass. The overhead of calling adj() a significant number of times was adding a runtime penalty. Its not clear what is causing this with retworkx but my assumption is it is related to going back and forth between rust and python. This commit mitigates most of this overhead by locally caching the list of neighbors each time we call _select_best_remaining_qubit(). * Avoid shared instance state * Appease pylint pedantry Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Nov 18, 2020
In Qiskit#5117 the noise adaptive layout pass was updated to use retworkx internally for performance. In that PR there were a couple of TODO comments added about using neighbors methods when they were released. The retworkx 0.6.0 release was pushed last week so we can update the usage and remove the TODO comments. The neighbors methods should have lower overhead since it only returns a list of node indices instead of a dictionary of (node index, edge weight). This will also get significantly faster (for large neighbors lists) with Qiskit/rustworkx#198.
mergify bot
added a commit
that referenced
this pull request
Dec 9, 2020
In #5117 the noise adaptive layout pass was updated to use retworkx internally for performance. In that PR there were a couple of TODO comments added about using neighbors methods when they were released. The retworkx 0.6.0 release was pushed last week so we can update the usage and remove the TODO comments. The neighbors methods should have lower overhead since it only returns a list of node indices instead of a dictionary of (node index, edge weight). This will also get significantly faster (for large neighbors lists) with Qiskit/rustworkx#198. Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.
Summary
This commit updates the nosie adaptive layout transpiler pass to use
retworkx for its internal graph operations. Prior to this commit it was
constructing 2 networkx graphs internally. These have been replaced with
equivalent structures in retworkx. At the same time some of the
algorithms used were generating extraneous information that was never
used this is removed.
Details and comments