Skip to content
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

Fix quadratic construction time of QuantumCircuit #11517

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

jakelishman
Copy link
Member

Summary

The current implementation of CircuitData.add_qubit (ditto clbit) has linear time complexity because it reconstructs the list on each addition, while the CircuitData.qubits getter silently clones the list on return. This was intended to avoid inadvertant Python-space modifications from getting the Rust components out of sync, but in practice, this cost being hidden in a standard attribute access makes it very easy to introduce accidental quadratic dependencies.

In this case, QuantumCircuit(num_qubits) and subsequent qc.append(..., qc.qubits) calls were accidentally quadratic in num_qubits due to repeated accesses of QuantumCircuit.qubits.

Details and comments

No changelog because this hasn't been released.

This was motivated by trying to understand why PauliBench.time_to_instruction was so affected by the Rust CircuitData. It seems like that might be one of the few places where we're really timing construction of few-hundred-qubit circuits without something else dominating the runtime; we probably should look to include larger-scale circuit-construction and QuantumCircuit.append benchmarks so quadratic complexities of simple components are caught more completely in the future.

To compare the following code-block:

from qiskit import QuantumCircuit
for n in (1_000, 10_000, 100_000):
    %timeit QuantumCircuit(n).qubits

before this PR gives

10.8 ms ± 106 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
873 ms ± 3.81 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
2min 19s ± 26.9 s per loop (mean ± std. dev. of 7 runs, 1 loop each)

where the quadratic scaling is fairly obvious, and afterwards gives:

2.72 ms ± 33.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
27.5 ms ± 843 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
277 ms ± 8.35 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

which tbh is still not as fast as I'd like, but I think the Python-space code is the culprit there, not the Rust code.

The Pauli benchmark looks something like:

from qiskit.quantum_info import Pauli

p = Pauli("X"*500)
%timeit p.to_instruction()

p = Pauli("-" + "X"*500)
%timeit p.to_instruction()

Adding a non-zero phase to the Pauli term invokes a different return value, and before this PR this gave:

123 µs ± 278 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
13.6 ms ± 87.9 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

now it's:

123 µs ± 804 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
3.65 ms ± 74.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

That's not quite as fast as pre-#10827, but it's much more in line with the performance penalty we were expecting, and doesn't scale quadratically any more.

The current implementation of `CircuitData.add_qubit` (ditto `clbit`)
has linear time complexity because it reconstructs the list on each
addition, while the `CircuitData.qubits` getter silently clones the
list on return.  This was intended to avoid inadvertant Python-space
modifications from getting the Rust components out of sync, but in
practice, this cost being hidden in a standard attribute access makes it
very easy to introduce accidental quadratic dependencies.

In this case, `QuantumCircuit(num_qubits)` and subsequent
`qc.append(..., qc.qubits)` calls were accidentally quadratic in
`num_qubits` due to repeated accesses of `QuantumCircuit.qubits`.
@jakelishman jakelishman added performance Changelog: None Do not include in changelog labels Jan 8, 2024
@jakelishman jakelishman added this to the 1.0.0 milestone Jan 8, 2024
@jakelishman jakelishman requested a review from a team as a code owner January 8, 2024 21:50
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

@mtreinish mtreinish added the Rust This PR or issue is related to Rust code in the repository label Jan 8, 2024
@coveralls
Copy link

coveralls commented Jan 8, 2024

Pull Request Test Coverage Report for Build 7467621907

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 87.555%

Totals Coverage Status
Change from base Build 7453493244: -0.003%
Covered Lines: 59464
Relevant Lines: 67916

💛 - Coveralls

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for recognizing that this was actually important to keep in constant time despite our benchmarks indicating otherwise!

crates/accelerate/src/quantum_circuit/circuit_data.rs Outdated Show resolved Hide resolved
crates/accelerate/src/quantum_circuit/circuit_data.rs Outdated Show resolved Hide resolved
@kevinhartman kevinhartman added this pull request to the merge queue Jan 10, 2024
Merged via the queue into Qiskit:main with commit cb4762f Jan 10, 2024
13 checks passed
@jakelishman jakelishman deleted the fix-circuit-qubits-scaling branch January 10, 2024 09:37
ShellyGarion pushed a commit to ShellyGarion/qiskit-terra that referenced this pull request Jan 18, 2024
* Fix quadratic construction time of `QuantumCircuit`

The current implementation of `CircuitData.add_qubit` (ditto `clbit`)
has linear time complexity because it reconstructs the list on each
addition, while the `CircuitData.qubits` getter silently clones the
list on return.  This was intended to avoid inadvertant Python-space
modifications from getting the Rust components out of sync, but in
practice, this cost being hidden in a standard attribute access makes it
very easy to introduce accidental quadratic dependencies.

In this case, `QuantumCircuit(num_qubits)` and subsequent
`qc.append(..., qc.qubits)` calls were accidentally quadratic in
`num_qubits` due to repeated accesses of `QuantumCircuit.qubits`.

* Restore explicit Python token

* Strengthen warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog performance Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants