Reimplement sine-cosine decomposition using faer instead of nalgebra #14967
Conversation
|
One or more of the following people are relevant to this code:
|
| if s.diagonal().iter().any(|x| x.im != 0.) { | ||
| for j in 0..n { | ||
| let z = s[(j, j)]; | ||
| let mut s: Vec<Complex64> = s.diagonal().column_vector().iter().copied().collect(); |
There was a problem hiding this comment.
This collection of diagonal elements of a matrix seems unnecessary complex, but I was not able to make it better, any ideas? More generally, I don't fully understand how to use DiagRef in faer (see https://docs.rs/faer/latest/faer/diag/type.DiagRef.html), in particular how to iterate over its elements (e.g., mat.diag().iter() does not work as there is no support for iteration). Am I missing something obvious?
There was a problem hiding this comment.
I think the obvious thing is the faer api here isn't very clear. I looked at the source: https://docs.rs/faer/0.21.9/src/faer/diag/diagref.rs.html#4-6 and diagonal().column_vector().iter() is the correct way to get an iterator over the diagonal elements from a matrix (if using the .diagonal() method). I guess you could do something like (0..n).map(|i| mat[(i, i)]) instead.
Pull Request Test Coverage Report for Build 17340333031Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
LGTM. Nice that it's working better now, but it's better to merge it after #14797 |
| let mut c: DVector<f64> = svd.singular_values.column(0).into_owned(); | ||
| let svd = u00.svd().expect("Problem with SVD decomposition"); | ||
| let l0 = svd.U().to_owned(); | ||
| let r0 = svd.V().adjoint().to_owned(); |
There was a problem hiding this comment.
adjoint here is the conjugate transpose and not the inverse?
| let mut l0 = svd.u.unwrap(); | ||
| let mut r0 = svd.v_t.unwrap(); | ||
| let mut c: DVector<f64> = svd.singular_values.column(0).into_owned(); | ||
| let svd = u00.svd().expect("Problem with SVD decomposition"); |
There was a problem hiding this comment.
do you want to add verify_svd_decomp (or a similar code in faer):
qiskit/crates/synthesis/src/linalg/mod.rs
Line 44 in 544885d
similar to what we did in:
qiskit/crates/synthesis/src/linalg/mod.rs
Line 122 in 429a9c0
| let arr2 = res.2.as_ref().into_ndarray().to_pyarray(py); | ||
| let arr3 = res.3.as_ref().into_ndarray().to_pyarray(py); | ||
|
|
||
| Ok(((arr0, arr1), res.4, (arr2, arr3)) |
There was a problem hiding this comment.
looking at this commit, it seems that instead of 0,1,2,3,4 it's better to call them l0,l1,r0,r1,thetas
Summary
After we have bumped
faerto the latest version (see #14873) and saw that it significantly improves the numerical accuracy of the SVD (see #14797), I have decided to reimplement the sine-cosine decomposition usingfaerinstead ofnalgebra, thus essentially making a full circle, as the original implementation by @mtreinish was also written usingfaer.Details and comments
This is all a bit unnecessary since we are no longer using CSD for the Quantum Shannon Decomposition (and currently anywhere else in the code), but I believe that now we have significantly lower numerical errors when using CSD, and in addition I could remove using
closest_unitaryinside the algorithm. We do have a large number of python tests checking that CSD is correct.