Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.11k stars 2.34k forks source link

Oxidize CheckGateDirection #13042

Closed eliarbel closed 3 weeks ago

eliarbel commented 1 month ago

Summary

This PR ports the CheckGateDirection analysis pass to Rust.

Details and comments

This PR includes:

Close #12256

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 10757805126

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/dag_circuit.rs 12 13 92.31%
crates/accelerate/src/gate_direction.rs 69 73 94.52%
<!-- Total: 84 89 94.38% -->
Files with Coverage Reduction New Missed Lines %
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 1 95.42%
crates/qasm2/src/lex.rs 3 92.98%
crates/qasm2/src/parse.rs 6 97.15%
<!-- Total: 10 -->
Totals Coverage Status
Change from base Build 10746759838: -0.001%
Covered Lines: 72924
Relevant Lines: 81798

💛 - Coveralls
eliarbel commented 1 month ago

Some benchmark data, using circuits generated like this:

tqc = generate_preset_pass_manager(1, coupling_map=CouplingMap.from_heavy_hex(7))
    .run(qc)

and this:

tqc = generate_preset_pass_manager(1, GenericBackendV2(qubits))
    .run(qc)

where:

 qubits = 100
 qc = random_circuit(qubits, qubits, seed=12234)

(note that DAGCircuit.count_ops() gives different stats for the coupling-based vs. target-bases circuits but this is irrelevant for this check)

And results are measured like this:

%%timeit
check_direction_pass.run(dag)
Implementation CouplingMap Target
Python 1.48 s ± 46.5 ms 641 ms ± 11.4 ms
Rust 130 ms ± 7.02 ms 131 ms ± 5.23

So ~5-11X improvement in this check by moving to Rust. Note that CouplingMap is (still) in Python while Target is in Rust, which may explain the run-time differences in Python and the speed-up differences between the target vs coupling map cases.

qiskit-bot commented 1 month ago

One or more of the following people are relevant to this code:

eliarbel commented 1 month ago

Thanks @ElePT . There are 2 paths for this pass: one is based on coupling map and one on target. So I think it doesn't really matter for this benchmarking to have the same coupling constraints, as long as we use the same constraints in each path separately both in Python and Rust .

Anyway, FWIW, here is another comparison, this time using the same coupling constraints, basis gates and traspiler seed in all 4 runs (python/rust X coupling/target). DAGCircuit.count_ops() is now the same for all: {'rz': 27177, 'sx': 6193, 'cx': 161650, 'x': 70}:

Implementation CouplingMap Target
Python 3.2 s ± 537 ms 3.21 s ± 104 ms
Rust 263 ms ± 26.2 213 ms ± 4.2 ms

I see that I need to resolve conflicts in dag_circuit.rs now that Jake's PR #13033 is merged; Will do soon

eliarbel commented 3 weeks ago

Thanks @mtreinish for the review and very helpful comments!

Here is an updated benchmark result. The Target path became slower for some reason. I didn't explore why, I'd defer this for a later phase.

Implementation CouplingMap Target
Python 3.19 s ± 450 3.18 s ± 221
Rust 137 ms ± 6.81 ms 567 ms ± 15.6