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.1k stars 2.34k forks source link

Fix creation of registers in synthesis methods #13086

Closed Cryoris closed 3 weeks ago

Cryoris commented 3 weeks ago

Summary

Fixes #13041 (and the same problem in other places, but no one complained yet 🙂).

Details and comments

With the moving the circuit creation to Rust-space, we created circuits that did not have any quantum (or classical) registers, which led to issues as #13041. This PR adds an argument add_regs to QuantumCircuit._from_circuit_data (default False) which allows to add these registers and create backward-compatible circuits. In places where the final circuit was created from circuit data, this argument is set to True.

qiskit-bot commented 3 weeks ago

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

Cryoris commented 3 weeks ago

I'm not 100% happy with this solution of compose-ing the circuit onto a new one (even if we can skip copying the ops), but I didn't find another solution to attach a register/index to each of the qubits (which we need for QuantumCircuit.__eq__ to work as before).

Another solution could be to change that __eq__ only compares bits if they both are either new style (no register/index) or old style (register/index available). But the compose solution seems less intrusive, especially for a bugfix PR we want to include in a bugfix release.

coveralls commented 3 weeks ago

Pull Request Test Coverage Report for Build 10789366864

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/quantumcircuit.py 16 21 76.19%
<!-- Total: 24 29 82.76% -->
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 90.82%
crates/qasm2/src/lex.rs 5 92.23%
crates/qasm2/src/parse.rs 6 97.61%
<!-- Total: 12 -->
Totals Coverage Status
Change from base Build 10774677144: -0.01%
Covered Lines: 73033
Relevant Lines: 81909

💛 - Coveralls
alexanderivrii commented 3 weeks ago

Thanks, @Cryoris! If I recall correctly, many of the circuit definition functions in circuit/library also create registers named q_* (and honestly I never really understood why we need this). Are you keeping this convention throughout your work on circuit library refactoring?

Cryoris commented 3 weeks ago

It now works without an additional compose and explicit call to add_register, but there's still a tiny overhead to adding registers coming from the register creation inside _from_circuit_data. However this only falls into weight when the circuit creation is super fast. For example here are timings repeated over 100 trials:

Permutation synthesis: slower (worse as size get's larger)
-- n = 10
(main) 1.91e-05 +- 5.16e-05s
(this PR) 2.26e-05 +- 5.01e-05s
-- n = 50
(main) 4.30e-05 +- 2.63e-05s
(this PR) 6.16e-05 +- 5.36e-05s
-- n = 100
(main) 8.63e-05 +- 8.49e-05s
(this PR) 1.11e-04 +- 9.73e-05s
-- n = 1000
(main) 2.78e-04 +- 1.05e-03s
(this PR) 1.63e-03 +- 3.36e-03s

Clifford: same speed
-- n = 10
(main) 1.24e-04 +- 8.60e-05s
(this PR) 1.24e-04 +- 6.16e-05s
-- n = 50
(main)  3.05e-03 +- 1.50e-04s
(this PR) 3.05e-03 +- 1.56e-04s
-- n = 100
(main) 1.80e-02 +- 7.58e-04s
(this PR) 1.80e-02 +- 4.37e-04s
-- n = 200
(main) 1.26e-01 +- 3.85e-03s
(this PR) 1.26e-01 +- 3.78e-03s

I think in practice this won't be an issue as HLS doesn't need the registers to synthesize building blocks within an existing circuit. We can add this fast path in a follow up, since that will require changing the API by adding an additional argument to the synthesis method, so we should avoid that in the backport.

Cryoris commented 3 weeks ago

@alexanderivrii for backward compatibility it would probably make sense to keep it, but let's see if that adds a significant overhead 🙂