-
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
Performance improvements for collect_2q_blocks #6433
Conversation
I benchmarked the changes and I got results ranging from 1.3x to 1.5x times faster. Below is the code for one of the benchmarks. It is a Quantum Volume Circuit, unrolled to use import timeit
from qiskit import QuantumRegister, QuantumCircuit
from qiskit.circuit.library import QuantumVolume
from qiskit.converters import circuit_to_dag
from qiskit.transpiler.passes.optimization import Collect2qBlocks
from qiskit.compiler import transpile
N = 100
q = QuantumRegister(N, 'q')
qc = QuantumCircuit(q)
qc.append(
QuantumVolume(N, N//2, seed=2, classical_permutation=False), q
)
qc = transpile(qc, optimization_level=0, basis_gates=['u1', 'u2', 'u3', 'cx'])
dag = circuit_to_dag(qc)
c2q = Collect2qBlocks()
print(
timeit.timeit("c2q.run(dag)", globals=globals(), number=10)
) Time without optimizations: 3.6710015139997267 |
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 add an upgrade
release note here about the bump in the minimum required retworkx version. We should call that out in the release notes as a thing that might require end user manual updates when upgrading to the next terra release.
I added the release note. This is no longer WIP, it can be merged |
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.
LGTM, just one comment with some suggestions on the release notes. Other than that I think this is ready to go.
I also ran the asv collect_2q_blocks benchmark on this and it yielded this result:
|
I also ran a quick benchmark on https://github.com/Qiskit/qiskit/blob/master/test/benchmarks/qasm/test_eoh_qasm.qasm (which is different load on the pass because it's all 2q, so one giant block) with: import time
import statistics
from qiskit.transpiler.passes import Collect2qBlocks
from qiskit.circuit import QuantumCircuit
from qiskit.converters import circuit_to_dag
qc = QuantumCircuit.from_qasm_file('test_eoh_qasm.qasm')
dag = circuit_to_dag(qc)
collect_pass = Collect2qBlocks()
times = []
for i in range(20):
start = time.time()
collect_pass.run(dag)
stop = time.time()
times.append(stop - start)
print("Mean run time: %s" % statistics.mean(times)) which returns 0.015 seconds on |
Co-authored-by: Matthew Treinish <[email protected]>
I commited the suggestions to the release note, the need for retworkx upgrade should be clearer now
Cool, the 1.3x - 1.5x speed up remained consistent accross the benchmarks. |
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.
Oops, thanks for fixing the whitespace issue in my release note suggestion (sigh getting whitespace right in the github editor is so irritating).
Summary
Closes Qiskit/rustworkx#265
This PR contains two core improvements that boost the performance of collect_2q_blocks:
quantum_successors
andquantum_predecessors
that are more efficientis_successor
andis_predecessor
function toDAGCircuit
to efficiently check for successors/predecessors instead of getting all successors/predecessors and then doing a checkIt also contains two minor performance optimizations that are open to debate:
qargs
with_qargs
, because of@property
overhead when gettingqargs
op
with_op
, idemDetails and comments
This PR also upgrades retworkx to version 0.9