-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Reimplement two_qubit_decompose.num_basis_gates in rust #11019
Conversation
* This will allow copyless convert of numpy complex matrix to faer * abs2 disappeared probably an error, so we work around here.
Existing tests pass. The following shows that from qiskit.providers.fake_provider import FakeVigo
from qiskit.quantum_info.synthesis.two_qubit_decompose import TwoQubitBasisDecomposer
from qiskit.circuit.library import CXGate
from qiskit.transpiler.passes.synthesis.unitary_synthesis import _choose_euler_basis
from qiskit.quantum_info import random_unitary
backend = FakeVigo()
conf = backend.configuration()
basis = conf.basis_gates
basis_fidelity = 1.0
euler_basis = _choose_euler_basis(basis)
pulse_optimize = None
gate = CXGate()
def make_twoq_decomposer():
backup_optimizer = TwoQubitBasisDecomposer(
gate,
basis_fidelity=basis_fidelity,
euler_basis=euler_basis,
pulse_optimize=pulse_optimize,
)
return backup_optimizer
u = random_unitary(4)
decomp = make_twoq_decomposer() Then In [2]: %timeit decomp.old_num_basis_gates(u)
96.3 µs ± 838 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
In [3]: %timeit decomp.num_basis_gates(u)
7.91 µs ± 48.5 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) |
|
||
#[pyfunction] | ||
#[pyo3(text_signature = "(basis_b, basis_fidelity, unitary, /")] | ||
pub fn _num_basis_gates(basis_b : f64, basis_fidelity : f64, unitary : PyReadonlyArray2<Complex<f64>>) -> usize { |
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.
One thing we can do here is call blocks_to_matrix
(https://github.com/Qiskit/qiskit/blob/main/crates/accelerate/src/convert_2q_block_matrix.rs#L25 ) directly from rust. The output of blocks_to_matrix
(via a layer of indirection) gets wrapped in a UnitaryGate
and then calls num_basis_gates
here: https://github.com/Qiskit/qiskit/blob/main/qiskit/transpiler/passes/optimization/consolidate_blocks.py#L134 to evaluate one of the conditions on whether synthesis is worth it or not (and if it is do the gate consolidation). So we could avoid some python call overhead by just returning this optionally directly from rust.
Although we probably can save this for a follow up to this PR.
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.
I can try to put it in here. It looks pretty straightforward, and should be done. But there are a few other issues with this PR to take care of.
One or more of the the following people are requested to review this:
|
|
EDIT: This bug has been fixed in faer 0.13.1 Some of the matrices encountered in the test suite trigger a bug (or bugs) in |
Calculating eigenvalues of 4 x 4 matrices is faster with In [16]: u = random_unitary(4).to_matrix()
In [17]: %timeit eigenvalues(u) # faer
3.83 µs ± 24.3 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
In [18]: %timeit eigvals(u) # numpy
12.5 µs ± 65.4 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
In [19]: 12.5 / 3.83
Out[19]: 3.2637075718015667 |
* Moved `eigenvalues` into module `utils`. * Access num_basis_gates via module two_qubit_decompose
Pull Request Test Coverage Report for Build 7700497765Warning: 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 |
* We introduced to_num_complex, but this required upgrading faer. We chose 0.15 * Upgrade our code, in turn, for the upgrade to faer 0.15 * Remove a function exported via pyo3 that was only used for testing * Remove `def old_num_basis_gate`
c1f8913
to
96a3f80
Compare
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.
Please note that in #11460 we deprecated quantum_info/synthesis and moved synthesis code to qiskit/synthesis.
1018bb8
to
09b4ecb
Compare
09b4ecb
to
fecdd90
Compare
We recently increased the version of the faer dependency. The newer version has an abs2 in the api named `faer_abs2`. So we can delete `myabs2`.
Following review suggestion.
This was previously at 0.15 for accidental reasons.
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.
Overall LGTM, I think this is basically ready. I just had a few very minor inline comments.
let pi = PI; | ||
let pi2 = PI / 2.0; | ||
let pi4 = PI / 4.0; |
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.
I'd make these module constants, especially because pi / 4 is reused in multiple functions. I expect it'd be compiled out but it's probably bit better to be explicit here. It'd be something like:
const PI2: f64 = PI / 2.0;
const PI4: f64 = PI / 4.0;
const PI32: f64 = 3.0 * PI2;
// The originial Python had `np.argmax`, which returns the lowest index in case two or more | ||
// values have a common maximum value. | ||
// `max_by` and `min_by` return the highest and lowest indices respectively, in case of ties. | ||
// So to reproduce `np.argmax`, we use `min_by` and switch the order of the | ||
// arguments in the comparison. |
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.
Thanks for the comment here, it explains the logic clearly and otherwise it'd be confusing to see this logic otherwise.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This commit is a follow up to Qiskit#11019 that uses the rust implementation of computing the weyl coordinates for a unitary and the transform_to_magic_basis functions everywhere. Previously they were only used internally by the num_basis_gates() rust function to estimate the number of basis gates needed for a decomposition of a given unitary. This will likely be superseded by Qiskit#11946 when that is working, but this is mainly an incremental step working towards finishin Qiskit#11946 that proves the internal rust functions work in the larger decomposer code to isolate the source of failures in Qiskit#11946.
This commit removes the weyl_coordinates() function from the private qiskit.synthesis.two_qubit.weyl module. This function's internal use was ported to rust as part of Qiskit#11019 but it left the python implementation intact while we ensured the rust implementation was reliable longer term. Since then we've ported the majority of the two qubit synthesis to rust now and the only usage of this python implementation was the unit tests. This commit removes the python implementation and the entire internal weyl module as nothing uses it anymore. A python interface is added to the rust function and the tests are updated to call that instead. Fixes: Qiskit#8459
EDIT: This PR introduces dependency on faer which depends on rustc 1.67, which was released in January 2023.
This would cause difficulties for some users who must use older versions of rustc. So this PR remains in draft mode at present.This PR does two things
qiskit/qiskit/quantum_info/synthesis/two_qubit_decompose.py
Lines 1412 to 1414 in 9843fbf
in rust
I looked at profiling with @mtreinish. It shows that a not-insignificant fraction of CPU time is spent here
At the same time, introduce a dependency on the linear algebra library faer. We are looking for a linear library to call from rust.
faer
is one of several options. A big advantage offaer
is that it is pure rust so it incurs no additional build/package/distribute burden. The author prioritizes performance and some benchmarks look good. The project was started, or at least became public, less than a year ago.This PR uses
faer
compute a determinant and eigenvalues of a 4x4 unitary matrix.Todo:
makeweyl_coordinates
return correct values. Or at least agree with current Python implementation.makenum_basis_get
return the correct values. It is currently broken. I just got the code to compile, and have not debugged. Five tests fail if you dotox -epy -- test.python.quantum_info.test_synthesis
. No other python tests fail.Currently this PR copies numpy complex matrices element by element to faer matrices. This should not be required. The author of faer told me that a version of faer that supports efficient conversion should be released tomorrow. To be clear, the version released tomorrow should support reading numpy arrays without copying.This partially addresses #8774 . Some of the obvious-looking numerical code in two-qubit synthesis is not in fact a bottleneck. However, this PR would improve performance a bit.