-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix quadratic construction time of QuantumCircuit
#11517
Conversation
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`.
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7467621907Warning: 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.
💛 - Coveralls |
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.
Looks good to me, thanks for recognizing that this was actually important to keep in constant time despite our benchmarks indicating otherwise!
* 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
Summary
The current implementation of
CircuitData.add_qubit
(dittoclbit
) has linear time complexity because it reconstructs the list on each addition, while theCircuitData.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 subsequentqc.append(..., qc.qubits)
calls were accidentally quadratic innum_qubits
due to repeated accesses ofQuantumCircuit.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 RustCircuitData
. 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 andQuantumCircuit.append
benchmarks so quadratic complexities of simple components are caught more completely in the future.To compare the following code-block:
before this PR gives
where the quadratic scaling is fairly obvious, and afterwards gives:
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:
Adding a non-zero phase to the Pauli term invokes a different return value, and before this PR this gave:
now it's:
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.