Use nalgebra Matrix2 internally in the TwoQubitBasisDecomposer#15928
Use nalgebra Matrix2 internally in the TwoQubitBasisDecomposer#15928raynelfss merged 4 commits intoQiskit:mainfrom
Conversation
This commit moves to using Matrix2 as the array type used internally for the TwoQubitBasisDecomposer. Matrix2 is a fixed size stack allocated matrix type that has several performance advantages especially for matmul because the compiler can reason about a fixed number of operations and better optimize the implementation. Similarly we avoid a lot of heap allocations. This will improve the runtime performance of the two qubit basis decomposer. This is part of the ongoing effort to move to using nalgebra's fixed size matrix types Matrix4 and Matrix2 inside all of the two qubit decomposer paths. We will still use faer for the involved linear algebra such as eigenvalue decomposition where it is faster and more numerically stable. This doesn't get us all the way to this goal, it's just another step on the journey. There are still places in the module that are using ndarray as the array types, this is mostly because they're used with either the Weyl decomposition or the one qubit euler decomposition. In particular there are a couple of duplicate methods either prefixed or postfixed with nalgebra to either return or convert to/from an nalgebra object which are temporary while we're in the middle of the transition. The goal is to remove these as we migrate the rest of the two qubit decomposers to be using nalgebra for the storage type.
mtreinish
added
performance
Changelog: None
|
One or more of the following people are relevant to this code:
|
|
I ran some asv benchmarks and there wasn't much of an improvement. Asv didn't have any confidence in a significant change in the numbers it reported. But as I said in the commit message this is just one step towards enabling using |
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
I couldn't find anything particularly concerning in the code here. I just had a very minor comment.
ShellyGarion
left a comment
ShellyGarion
left a comment
There was a problem hiding this comment.
I have some minor comments.
In addition, is there a plan to convert the other two-qubit decomosers (TwoQubitWyelDecomposer and TwoQubitControlledUDecomposer) into nalgeba as well?
| use super::common::{ | ||
| DEFAULT_FIDELITY, IPZ, TraceToFidelity, rx_matrix, rz_matrix, transpose_conjugate, | ||
| }; | ||
| use super::common::{DEFAULT_FIDELITY, TraceToFidelity, rx_matrix_nalgebra, rz_matrix_nalgebra}; |
There was a problem hiding this comment.
note that IPZ has been moved to common.rs since it's also used in weyl_decompositions.rs
There was a problem hiding this comment.
This is temporary just for this PR as the version in common.rs is replaced with a Matrix2 version in the next PR: #15960 That PR moves all the static IPZ, IPY, and IPX definitions to use Matrix2 instead of [[Complex64; 2]; 2] as all the usage has been moved to nalgebra in that PR.
| use qiskit_circuit::{NoBlocks, Qubit}; | ||
| use qiskit_util::alias::GateArray1Q; | ||
| use qiskit_util::complex::{C_M_ONE, C_ONE, IM, M_IM, c64}; | ||
| use qiskit_util::complex::{C_M_ONE, C_ONE, C_ZERO, IM, M_IM, c64}; |
There was a problem hiding this comment.
if we don't define IPZ in this file, then C_ZERO is not needed here.
There was a problem hiding this comment.
This is temporary just for this PR as the version in common.rs is replaced with a Matrix2 version in the next PR: #15960 That PR moves all the static IPZ, IPY, and IPX definitions to use Matrix2 instead of [[Complex64; 2]; 2] as all the usage has been moved to nalgebra in that PR.
| [c64(0., FRAC_1_SQRT_2), c64(FRAC_1_SQRT_2, 0.)], | ||
| [c64(-FRAC_1_SQRT_2, 0.), c64(0., -FRAC_1_SQRT_2)], | ||
| ]; | ||
| static IPZ: Matrix2<Complex64> = Matrix2::new(IM, C_ZERO, C_ZERO, M_IM); |
There was a problem hiding this comment.
note that IPZ has been moved to common.rs since it's also used in weyl_decompositions.rs
There was a problem hiding this comment.
This is temporary just for this PR as the version in common.rs is replaced with a Matrix2 version in the next PR: #15960 That PR moves all the static IPZ, IPY, and IPX definitions to use Matrix2 instead of [[Complex64; 2]; 2] as all the usage has been moved to nalgebra in that PR.
| static IPZ: Matrix2<Complex64> = Matrix2::new(IM, C_ZERO, C_ZERO, M_IM); | ||
|
|
||
| static HGATE: Matrix2<Complex64> = | ||
| Matrix2::new(H_GATE[0][0], H_GATE[0][1], H_GATE[1][0], H_GATE[1][1]); |
There was a problem hiding this comment.
the name HGATE may be a bit confusing (with H_GATE). Maybe call it HGATE_matrix or H_matrix ?
also, why not use the matrix method of the standard gates here?
There was a problem hiding this comment.
I can rename the variable name I just picked something that wouldn't conflict with the static being imported here. But it needs to be all capital letters as a static, rustfmt will complain otherwise.
The reason I didn't use the StandardGate::matrix() method here is that the matrix method is not defined as a const method and I can't call it from a static context. The matrix method in particular is written to return an owned Array2<Complex64> and we wouldn't be able to use that in a const function anyway because it relies on dynamic memory allocation. We might be able to make the matrix_as_static_1q and matrix_as_static_2q methods const functions, but it would require everything in them to be defined as const functions.
I wanted this to be a static so we only have a single Matrix2 that is use by reference for the Hadamard matrix when we need it. This avoid allocating a temporary 2x2 matrix in any form when we need it. The problem with the existing static is it's a [[Complex64; 2]; 2] which isn't compatible with nalgebra types for matrix multiplication or other operations, so I needed a static that was a Matrix2 type for this use case.
There was a problem hiding this comment.
maybe HGATE_MATRIX? or H_GATE_MATRIX?
| #[inline] | ||
| pub fn ndarray_to_matrix2<T: Copy>(view: ArrayView2<T>) -> Matrix2<T> { | ||
| Matrix2::new(view[[0, 0]], view[(0, 1)], view[(1, 0)], view[(1, 1)]) | ||
| } |
There was a problem hiding this comment.
in https://github.com/Qiskit/qiskit/blob/main/crates/synthesis/src/linalg/mod.rs there are several methods to convert ndarrays to and from nalgbra and faer.
perhaps it's worth to move this function there too?
Yes as I mentioned in the commit message/PR summary this is just an incremental step towards migrating to use nalgebra for fixed size small matrices in the two qubit decomposer code. I have the second PR open for the |
4 participants
Summary
This commit moves to using Matrix2 as the array type used internally for the TwoQubitBasisDecomposer. Matrix2 is a fixed size stack allocated matrix type that has several performance advantages especially for matmul because the compiler can reason about a fixed number of operations and better optimize the implementation. Similarly we avoid a lot of heap allocations. This will improve the runtime performance of the two qubit basis decomposer.
This is part of the ongoing effort to move to using nalgebra's fixed size matrix types Matrix4 and Matrix2 inside all of the two qubit decomposer paths. We will still use faer for the involved linear algebra such as eigenvalue decomposition where it is faster and more numerically stable. This doesn't get us all the way to this goal, it's just another step on the journey.
There are still places in the module that are using ndarray as the array types, this is mostly because they're used with either the Weyl decomposition or the one qubit euler decomposition. In particular there are a couple of duplicate methods either prefixed or postfixed with nalgebra to either return or convert to/from an nalgebra object which are temporary while we're in the middle of the transition. The goal is to remove these as we migrate the rest of the two qubit decomposers to be using nalgebra for the storage type.
Details and comments