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

Directly construct CircuitData in TwoQubitWeylDecomposition #12809

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

mtreinish
Copy link
Member

Summary

In the recently merged #12740 a path was added for constructing the CircuitData in rust when synthesizing to a QuantumCircuit directly. However, in that PR this was done through a layer of indirection by first collecting the gates into an intermediate Vec and then passing an iterator of that into
CircuitData::from_standard_gates(). However this resulted in an unecessary allocation for two Vecs and it would have been better to just directly construct the CircuitData object directly. However, to accomplish this we needed more rust space methods for creating and manipulating a CircuitData object as it's mostly being constructed from Python space or using CircuitData::from_standard_gates() with an iterator so far. This commit makes those additions and then updates the TwoQubitWeylDecomposition code to directly construct the CircuitData object instead of using an intermediate Vec.

Details and comments

In the recently merged Qiskit#12740 a path was added for constructing the
`CircuitData` in rust when synthesizing to a `QuantumCircuit`
directly. However, in that PR this was done through a layer of
indirection by first collecting the gates into an intermediate `Vec`
and then passing an iterator of that into
`CircuitData::from_standard_gates()`. However this resulted in an
unecessary allocation for two `Vec`s and it would have been better
to just directly construct the `CircuitData` object directly. However,
to accomplish this we needed more rust space methods for creating
and manipulating a `CircuitData` object as it's mostly being constructed
from Python space or using `CircuitData::from_standard_gates()` with
an iterator so far. This commit makes those additions and then updates
the `TwoQubitWeylDecomposition` code to directly construct the
`CircuitData` object instead of using an intermediate `Vec`.
@mtreinish mtreinish added Changelog: None Do not include in changelog synthesis Rust This PR or issue is related to Rust code in the repository labels Jul 24, 2024
@mtreinish mtreinish requested a review from a team as a code owner July 24, 2024 09:45
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @kevinhartman
  • @levbishop
  • @mtreinish

Comment on lines +186 to +188
let no_clbit_index = (&mut self.cargs_interner)
.intern(InternerKey::Value(Vec::new()))?
.index;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not super happy with this, it feels like unnecessary overhead on every call. It's why I didn't update from_standard_gates() to call this new method because it does this outside the loop. It feels like maybe we should have an implicit empty index that's a constant in the interner so we can avoid this.

Copy link
Member

@jakelishman jakelishman Aug 8, 2024

Choose a reason for hiding this comment

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

I have a change to the interners I want to make once the DAGCircuit PR merges that will avoid the need for the Vec here in favour of the static empty slice (which is at least a small saving), but I think the idea of the special "intern key to the 'zero' object" is a good one too. I can see it coming up a lot, and I think it's doable without losing any particular generality in the interners - we can make it like

pub struct Interner<T> { ... };
pub struct InternKey<T> {
   index: u32,
   _phantom: PhantomData<T>,
};
impl<T: Default> InternKey<T> {
    pub fn empty() -> Self {
        Self {
            val: 0,
            _phantom: PhantomData,
        }
    }
}

or something like that, and arrange that the 0 index of Interner<T: Default> always corresponds to <T as Default>::default().

@mtreinish
Copy link
Member Author

I've run a quick little benchmark script:

import statistics
import time

from qiskit.synthesis.two_qubit import TwoQubitWeylDecomposition
from qiskit.circuit.library import CXGate
from qiskit.quantum_info import random_unitary

unitaries = [random_unitary(4) for _ in range(10000)]
runtimes = []
for mat in unitaries:
    start = time.perf_counter()
    TwoQubitWeylDecomposition(mat).circuit()
    stop = time.perf_counter()
    runtimes.append(stop - start)
print(f"Mean runtime per unitary: {statistics.geometric_mean(runtimes)} sec.")
print(f"Total runtime for 10k unitary matrices: {sum(runtimes)} sec.")

to compare the performance with and without this PR. There is a consistent small speedup I'm seeing with this PR applied. With this PR:

Mean runtime per unitary: 2.8342069525664723e-05 sec.
Total runtime for 10k unitary matrices: 0.3291525247041136 sec.

on current main:

Mean runtime per unitary: 3.0709930013551664e-05 sec.
Total runtime for 10k unitary matrices: 0.3557815661188215 sec.

There is some fluctuation in the numbers obviously especially with fully random unitaries, but I haven't seen a case where the PR is slower (or the same speed) than main. Not that TwoQubitWeylDecomposition.circuit() is performance critical at all, but I wanted to put some numbers behind why we would want to do this.

crates/circuit/src/circuit_data.rs Outdated Show resolved Hide resolved
crates/circuit/src/circuit_data.rs Outdated Show resolved Hide resolved
crates/circuit/src/circuit_data.rs Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 24, 2024

Pull Request Test Coverage Report for Build 10305567368

Details

  • 95 of 99 (95.96%) changed or added relevant lines in 2 files are covered.
  • 25 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.005%) to 89.734%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/circuit_data.rs 54 58 93.1%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 90.69%
crates/qasm2/src/lex.rs 6 91.48%
crates/qasm2/src/parse.rs 18 96.69%
Totals Coverage Status
Change from base Build 10302737074: -0.005%
Covered Lines: 67363
Relevant Lines: 75070

💛 - Coveralls

@mtreinish mtreinish requested a review from jakelishman August 8, 2024 16:07
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This seems like a good idea to me, thanks for the updates.

@jakelishman jakelishman enabled auto-merge August 8, 2024 17:04
@jakelishman jakelishman added this to the 1.3 beta milestone Aug 8, 2024
@jakelishman jakelishman added this pull request to the merge queue Aug 8, 2024
Merged via the queue into Qiskit:main with commit 20b51e6 Aug 8, 2024
15 checks passed
@mtreinish mtreinish deleted the use-circuitdata-in-2q-weyl-synth branch August 9, 2024 11:19
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2024
…pacity`(backport #12943 + part of #12809) (#13038)

* Allow `CircuitData` construction from `PackedOperation`s (#12943)

* ``CircuitData::from_packed_operations``

* missing import

* remove redundant `to_vec`

(cherry picked from commit b1e7ffe)

* Add with_capacity

* Add CLBIT import

* Run cargo fmt

---------

Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Elena Peña Tapia <[email protected]>
Co-authored-by: Elena Peña Tapia <[email protected]>
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 Rust This PR or issue is related to Rust code in the repository synthesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants