Oxidize the internals of Optimize1qGatesDecomposition

[mtreinish]

Summary

This commit rewrites the internals of the Optimize1qGatesDecomposition transpiler pass to leverage more Rust. As the size of circuits are growing the amount of time the transpiler typically spends in Optimize1qGatesDecomposition grows linearly with the number of gates. Since #9185 (which converted the angle calculation in the synthesis routine to Rust) the time spent constructing intermediate DAGCircuit objects for each possible decomposition has been dominating the total runtime of the pass. To attempt to alleviate this bottleneck this commit moves as much of the circuit construction to rust as possible. The one qubit euler decomposition is now done in Rust and a sequence of gate names along with their corresponding angles are returned to the pass with the lowest error rate is returned. The pass will then convert that sequence into a DAGCircuit object if the decomposition will be substituted into the output dag. This has the advantage of both speeding up the computation of the output circuit and also deferring the creation of DAGCircuit and Gate objects until they're actually needed.

Details and comments

TODO:

• Fix test failures
• Benchmark and tune
• Migrate all error heuristics to Rust
• Release note

Follow-up PRs:

• Update quantum info one_qubit_decompose module to leverage rust circuit construction functions: Leverage Rust circuit sequence construction for OneQubitEulerDecomposer #9583

[mtreinish]

Some quick initial benchmarks, running the asv benchmarks for this pass yielded:

Benchmarks that have improved:

       before           after         ratio
     [659063e2]       [0815082d]
     <main>       <rusty-euler-one_qubit>
-       119±0.8ms         92.9±2ms     0.78  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['rz', 'x', 'sx', 'cx', 'id'])
-         363±1ms          278±4ms     0.76  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['rz', 'x', 'sx', 'cx', 'id'])
-         269±2ms          203±1ms     0.76  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['rz', 'x', 'sx', 'cx', 'id'])
-         187±2ms          122±3ms     0.65  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id'])
-         580±5ms          372±4ms     0.64  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id'])
-         430±3ms          270±8ms     0.63  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id'])
-      62.0±0.2ms       34.1±0.6ms     0.55  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['u', 'cx', 'id'])

Benchmarks that have stayed the same:

       before           after         ratio
     [659063e2]       [0815082d]
     <main>       <rusty-euler-one_qubit>
          136±1ms          134±2ms     0.98  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['u', 'cx', 'id'])
          186±2ms          182±1ms     0.98  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['u', 'cx', 'id'])

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

[mtreinish]

Running the 1q benchmarks with the updated PR (at 81679b6)

Benchmarks that have improved:

       before           after         ratio
     [1fb00e62]       [81679b62]
     <main>       <rusty-euler-one_qubit>
-         264±1ms          159±2ms     0.60  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['rz', 'x', 'sx', 'cx', 'id'])
-         357±2ms          213±3ms     0.60  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['rz', 'x', 'sx', 'cx', 'id'])
-         117±1ms       69.6±0.9ms     0.59  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['rz', 'x', 'sx', 'cx', 'id'])
-      59.7±0.7ms       33.9±0.1ms     0.57  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['u', 'cx', 'id'])
-       185±0.8ms       59.1±0.8ms     0.32  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id'])
-         564±2ms          175±1ms     0.31  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id'])
-        420±10ms        130±0.8ms     0.31  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id'])

Benchmarks that have stayed the same:

       before           after         ratio
     [1fb00e62]       [81679b62]
     <main>       <rusty-euler-one_qubit>
          134±1ms        134±0.8ms     1.01  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['u', 'cx', 'id'])
          185±3ms          183±3ms     0.99  passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['u', 'cx', 'id'])

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

[coveralls]

Pull Request Test Coverage Report for Build 4318414588

• 541 of 619 (87.4%) changed or added relevant lines in 4 files are covered.
• 9 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.02%) to 85.321%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/euler_one_qubit_decomposer.rs	497	575	86.43%

Files with Coverage Reduction	New Missed Lines	%
src/euler_one_qubit_decomposer.rs	2	87.63%
qiskit/quantum_info/synthesis/one_qubit_decompose.py	7	93.86%

Totals
Change from base Build 4309626206:	-0.02%
Covered Lines:	67887
Relevant Lines:	79567

---

💛 - Coveralls

[jlapeyre]

Why are these two tests removed and none are added? Is this "XZX is re-written as ZXZ" no longer done? Or you thought the test isn't useful?

[mtreinish]

These tests were specifically about testing the return of the internal _error helper function that was used to evaluate the heuristic score for a particular basis's found decomposition. That function was removed in this PR as the scoring is now all done on the Rust side and mostly integrated into the pass. I could add an equivalent test to call the rust function but it didn't seem necessary since it's already covered implicitly by the other tests.

That being said I was probably just being more lazy than I needed to, and could rewrite the tests fairly easily (I just need to convert the dags used into the format the rust function expects and update the result expectation to be in the new format).

[jakelishman]

By my understanding, this is a fairly mechanical port of the existing code in quantum_info.OneQubitEulerDecomposer (or whatever it's called). My primary concern is that we're leaving that class in existence, so we've got all this fairly substantial mathematical code duplicated with no path to re-unifying them.

Is there any rearchitecture we could do to keep the quantum_info.OneQubitEulerDecomposer as a Python class, but have it use these Rust internals, so all uses of that class throughout Qiskit get improved? I'm assuming there might be some deprecation / API issues between them, since Rust will want the public/private boundary somewhere else most likely for the best performance. Either that, or can we roughly plan for how we'll get rid of the Python versions used elsewhere?

[jakelishman]

We could probably keep clippy happy here and avoid suppressing unnecessary_unwrap with something like

match xpifun {
    Some(xpifun) if mod_2pi(theta).abs() < atol => xpifun(&mut circuit),
    _ => {
        xfun(&mut circuit);
        pfun(&mut circuit, theta);
        xfun(&mut circuit);
    }
}

though I'd agree that with if let Some(x) = x && cond still unstable, it's possibly a bit premature for clippy to be warning on that.

[mtreinish]

Yeah, I bounced off that API not being stabilized and just decided to suppress clippy. I'll try playing with that syntax a bit locally and see if I can get it working as expected

[mtreinish]

Done in: b192444

[jlapeyre]

Do you have a link to a discussion of "API not being stabilized" ?

[mtreinish]

The tracking issue for the rfc is here: rust-lang/rust#53667 rust upstream hasn't finalized the feature yet. Although looking at the issue and rfc it probably won't be up for stabilization until the next rust edition.

Also, if you try using this syntax the compiler links to this issue too:

error[[E0658]](https://doc.rust-lang.org/stable/error_codes/E0658.html): `let` expressions in this position are unstable
 --> src/main.rs:5:16
  |
5 |     if true && let Some(foo) = test {
  |                ^^^^^^^^^^^^^^^^^^^^
  |
  = note: [see issue #53667 <https://github.com/rust-lang/rust/issues/53667>](https://github.com/rust-lang/rust/issues/53667) for more information

For more information about this error, try `rustc --explain E0658`.

[Eric-Arellano]

Is there any rearchitecture we could do to keep the quantum_info.OneQubitEulerDecomposer as a Python class, but have it use these Rust internals, so all uses of that class throughout Qiskit get improved?

This is an interesting idea to reduce migration risk. It'd be useful to see none of the Python call sites updated due to API changes. The only thing in Python-land that changes is the implementation of the methods belonging to quantum_info.OneQubitEulerDecomposer to call out to Rust.

We can continue to improve in followup PRs from there, such as considering porting the entire Python class to Rust, not just the implementation of its methods. Or, replace the original Python class with a better design (in Python and/or Rust).

Are the Rust functions a private API? That gives us much more flexibility when oxidizing to do things incrementally.

[mtreinish]

By my understanding, this is a fairly mechanical port of the existing code in quantum_info.OneQubitEulerDecomposer (or whatever it's called). My primary concern is that we're leaving that class in existence, so we've got all this fairly substantial mathematical code duplicated with no path to re-unifying them.

Is there any rearchitecture we could do to keep the quantum_info.OneQubitEulerDecomposer as a Python class, but have it use these Rust internals, so all uses of that class throughout Qiskit get improved? I'm assuming there might be some deprecation / API issues between them, since Rust will want the public/private boundary somewhere else most likely for the best performance. Either that, or can we roughly plan for how we'll get rid of the Python versions used elsewhere?

I have that done in the follow up PR: #9583 I realize I probably did the split of work backwards. This was mostly an artifact of my primary area of concern when I started this was the transpiler pass and the quantum info class was a secondary concern. Also the performance improvements for the QI class is less substantial because most of the heavy lifting is already in rust.

[mtreinish]

Just for reference I did some scale testing with this PR, basically just ran transpile() at optimization level 1 on random_circuit(27, d, measure=True) targeting FakeGeneva with this PR applied:

It's looking like roughly a constant 2x performance improvement compared to 0.23.1:

[jakelishman]

I'm still keen to avoid duplication between this and the Python code, but by my understanding, that's the job of #9583. This looks just about good to merge to me. The only comments below that maybe need actions are the questions about missing endpoint-clamping in mod_2pi and a potential loss of a Python option allow_non_canonical.

[jakelishman]

This is a bit repetitive - maybe something like

let mut pos: isize = indices.start;
let cond = if indices.step < 0 {
    |pos| pos > indices.stop  // `move |pos|` if needed, but I think indices.stop is Copy anyway?
} else {
    |pos| pos < indices.stop
};
while cond(pos) {
    // ...
    pos += indices.step;
}

avoids the manual repetition in the cond check?

[mtreinish]

I gave this a try locally and it's a bit more involved than I thought because you can't have two arms on an assignment like this as closures because they're all inherently different types. I tried to define it as a dyn function type instead but it wasn't as straightforward as I thought. I'm sure I could play with it a bit more and figure out a pattern that worked, but it didn't seem critical.

[jakelishman]

Oh yeah, good point. We could put the branch inside the closure? If it's too much effort, it's not worth bothering, though.

[mtreinish]

Oh I didn't try that, I was fixated on trying to use different closures. :) But, yeah it would work to branch in the closure too. But I'd rather let it merge as is, we can do this in a follow up pretty easily.

[jakelishman]

I was going to comment that f64::rem_euclid still isn't the same as Python's (rem_euclid is always non-negative, whereas Python's % matches the sign of the right-hand operator), but since we're only doing modulo a positive number, we're good.

Also, this function has lost the endpoint clamping that the Python version does. Are we safe to do that?

[mtreinish]

Yeah, this one was annoying to me too, I can leave a comment to this effect inline just for the historical record. Since it didn't matter in practice doing this was easier than the alternative which was to write our own equivalent of Python's % for rust which felt like a pain.

As for the clamping I thought it was safe to do since I didn't pass atol (I don't remember why I removed it, I think because nothing in the Optimize1qGatesDecomposition ever changed it from the default) into this function and just always used the default as 0. So the clamping wouldn't really come up because we'd never be less than 0 here. But I might have been mistaken and can add it back if you think it's important (and if my memory of why I removed the atol parameter is correct I should add it back, probably in #9583 as setting the atol is part of that class's API).

[jakelishman]

Yeah, agreed on % - we don't need to care about what happens with negative RHS because ours is a constant.

I'm happy to just add the clamping back in #9583 as appropriate - let's move to try and get this PR merged and onto the next one, where that part will actually be used.

[mtreinish]

I added a comment on this here: 95f6974

[jakelishman]

wow I'm glad this isn't a public-facing API haha

[mtreinish]

Lol, yeah this took me longer to figure out than I wanted. The compiler wasn't able to implicitly figure out the type of the other arm of the Option<T> with just a None here and T it's required to be Sized so I had to wrap it a Box because dyn types aren't sized.

[jakelishman]

Yeah, it all makes sense when I saw what you'd done, but I've never have thought of it without seeing your work!

[jakelishman]

Nice, looks good in this form, and we can move on to getting the more public Python API parts using the Rust as well now too.