Reimplementing QSD using faer instead of nalgebra. by alexanderivrii · Pull Request #15874 · Qiskit/qiskit

alexanderivrii · 2026-03-25T11:28:21Z

Summary

This PR reimplements quantum Shannon decomposition to use faer instead of nalgebra as faer is significantly better for involved linear algebra calculations.

Fixes #15870.

qiskit-bot · 2026-03-25T11:28:25Z

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

@Qiskit/terra-core

mtreinish · 2026-03-25T13:43:13Z

I'm curious is there any runtime difference between this and the previous implementation. Since this is a correctness fix it doesn't really matter since we're going to do it anyway, I'm just curious.

ShellyGarion

Thnaks for fixing this bug so quickly... I do hope that with faer we won't have the numeric issues in QSD anymore...

mtreinish

I've only gotten through the linear algebra module and not the bulk of the qsd yet. But I'm about to step away so I thought I'd share my comments so far

mtreinish · 2026-03-25T13:57:11Z

+pub fn svd_decomposition_faer(
+    mat: MatRef<Complex64>,
+) -> (Mat<Complex64>, Mat<Complex64>, Mat<Complex64>) {
+    let svd = mat.svd().expect("Call to faer SVD failed");


Should we handle this and return a user facing error like we do for nalgebra. From what I can see in the faer docs this will be a convergence error which I'm not sure we'd want to result in a panic.

I have improved the error checking as part of a4a1172, however I did not succeed to wrap errors produced by faer similarly to errors coming from python or circuit data (while there is a class FaerError, see https://docs.rs/faer/latest/faer/sparse/enum.FaerError.html, it does not seem to account for errors coming from say SVD decomposition).

mtreinish · 2026-03-25T14:00:37Z

+    d: MatRef<Complex64>,
+) -> Mat<Complex64> {
+    let n = a.nrows();
+    let mut block_matrix = Mat::<Complex64>::zeros(2 * n, 2 * n);


Do you need to initialize this with zero? It feels unnecessary because you should be populating every element in the matrix based on the blocks.

The only way I can see how to avoid this (keeping the current code structure) is doing something like

let mut block_matrix = Mat::<Complex64>::with_capacity(2 * n, 2 * n); unsafe { block_matrix.set_dims(2 * n, 2 * n) };

However, I semi-expect that the compiler should be able to optimize this itself. I have kept the original code for now, but replaced the loops as per Shelly's suggestion. I am also not very worried about this specific code as for now it's only used in debug assertions.

I was thinking something along those lines. I wasn't sure if faer's Mat type had a method for creating an uninitialized matrix directly or you had to do something like that. But it would be inside an unsafe either way. The other option would be to use from_fn and have conditions on the index ranges.

I find it unlikely that the compiler will able to optimize it away because Mat::zeros() is calling Mat::from_fn which is explicitly calling the closure that returns zero for every element in the array. But it's only a small performance difference, it's something we can explore later if this becomes a bottleneck.

alexanderivrii · 2026-03-25T18:31:51Z

A quick answer on performance comparison.

Running the following simple script on my laptop (generating 10 random unitary matrix over nq qubits, running qs_decomposition, and taking the average of runtimes):

run_times = []
num_trials = 10
for nq in range(1, 11):
    total_time = 0
    for seed in range(num_trials):
        n = 2 ** nq
        mat = random_unitary(n, seed=seed).data
        time_start = time.time()
        qc = qsd.qs_decomposition(mat)
        time_end = time.time()
        total_time += time_end - time_start
    avg_time = total_time / num_trials
    run_times.append(avg_time)
print(run_times)

returns the following

main = [9.09566879272461e-05, 4.303455352783203e-05, 0.00023097991943359374, 0.0009044647216796875, 0.004224538803100586, 0.019590377807617188, 0.10117588043212891, 0.5623687744140625, 3.393336462974548, 22.304159712791442]

this_pr = [9.09566879272461e-05, 4.079341888427734e-05, 0.00023005008697509765, 0.0009874343872070313, 0.0043228864669799805, 0.018857145309448244, 0.09331314563751221, 0.47628915309906006, 2.518214392662048, 12.950119781494141]

Overall it seems like we have a performance improvement as well, especially for larger number of qubits (I believe it's due to translating from nalgebra to faer and back for the SVD).

ShellyGarion

LGTM. but I'll wait with merging to see if @mtreinish has any further comments.

mtreinish

Overall this is fairly straightforward. I left a few inline comments. The biggest thing is I don't think we need to add faer as a dependency to the transpiler crate. We should just adjust our public interface to use ndarray types, because that's (and nalgebra for 1q and 2q unitaries) actually what we use for storage in the transpiler. So it's more natural if we just move the ndarray -> faer conversion inside the qsd function (and do it without copies).

mtreinish · 2026-03-26T10:40:40Z

 /// Decomposed quantum circuit.
 pub fn quantum_shannon_decomposition(
-    mat: &DMatrix<Complex64>,
+    mat: MatRef<Complex64>,


I mentioned this in the transpiler module, but I think we should make this an ArrayView2, we only use qsd from either Python or the transpiler. In both contexts it's easier to work with an ndarray view, since rust-numpy gives us an ndarray array view and similarly we can run UnitaryGate::matrix_view() returns an ArrayView2. We can just make the call crate::linalg::ndarray_to_faer to get the MatRef from the ArrayView2 with no copies when we start using faer.

This makes sense - I love this! With this change, only the synthesis crates needs to be aware about faer. Does it still make sense to include the faer version in the topmost level Cargo.toml file (and set faer.workspace in the synthesis toml file), or is the older version of setting the faer version in synthesis toml directly preferable?

mtreinish · 2026-03-26T10:51:04Z

+    let (v, sigma, w_dg) = svd_decomposition_faer(a)?;

-    let s = &v * &sigma * &v.adjoint();
+    let s = v.as_ref() * sigma * v.adjoint();
    let u = v * w_dg;

-    (s, u)
+    Ok((s, u))


The helper makes an owned copy of the svd's output matrices. When I see this I'm wondering do we even want that helper method? You can avoid these copies if you just call faer directly just like the comment I left in the linalg file yesterday. I assumed the to_owned calls in the helper function were because something needed an owned copy, but this could work with references instead.

I tried removing this helper, but surprisingly it did not noticeably change runtimes, even for large matrices, while making the code a bit uglier. But I ended up removing the helper function zxz_decomp_svd which also copies matrices.

…adjoint

mtreinish

Thanks for all the updates and work on this. I just had one small comment inline about error messages. Right now I'm a bit worried they're a bit opaque for the internal tolerance checks or the decompositions fail. After updating that that I'm ready to approve I think.

mtreinish · 2026-04-09T11:01:13Z

-            QSDError::SchurDecompositionFailed => {
-                QiskitError::new_err("Schur decomposition failed")
-            }
+            QSDError::ErrorFromLinAlg(err) => err.into(),


I do wonder if we want a better error message here. Right now a user calls qsd, unitary synthesis, or the transpiler and will get an exception: `QiskitError: Eigen decomposition failed" that points to the qsd rust function. I'm worried this is too opaque since this will end up potentially user facing. Maybe we should change the error message to something like, "internal eigendecomposition run as part of qsd failed. This can point to a numerical tolerance issue...."

Improved error messages in 91f40b5.

mtreinish · 2026-04-09T11:10:50Z

 rand_pcg.workspace = true
 rand_distr.workspace = true
-faer = "0.24"
+faer.workspace = true


We don't really need this change anymore but it doesn't hurt either, since we've agreed at this point we'll rely on faer for all the heavy linear algebra. So there is a fairly good chance we'll use it in another crate at some point.

mtreinish

I think this is good to merge now, thanks for all your hard work on this. I'm still a bit worried about the error message text not being descriptive enough for end users. But I don't have a concrete suggestion on how to improve it right now. So if we come up with something better we can update the error message text in the future.

Reimplementing QSD using faer instead of nalgebra.

4d17115

alexanderivrii added this to the 2.5.0 milestone Mar 25, 2026

alexanderivrii requested a review from a team as a code owner March 25, 2026 11:28

alexanderivrii added the Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. label Mar 25, 2026

alexanderivrii added this to Qiskit 2.5 Mar 25, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.5 Mar 25, 2026

mtreinish self-assigned this Mar 25, 2026

ShellyGarion reviewed Mar 25, 2026

View reviewed changes

mtreinish reviewed Mar 25, 2026

View reviewed changes

alexanderivrii added 2 commits March 25, 2026 21:59

Addressing review comments

a4a1172

merge with main

58bebe3

alexanderivrii requested review from ShellyGarion and mtreinish March 25, 2026 20:15

ShellyGarion previously approved these changes Mar 26, 2026

View reviewed changes

mtreinish reviewed Mar 26, 2026

View reviewed changes

alexanderivrii added 8 commits March 26, 2026 13:59

refacting SVDResult into a struct

8e149b0

for naming consistency, changing SVDResult to return V rather than V.…

e6d8366

…adjoint

refactoring ZXZResult into a struct

771ec8a

improved diagonal matrix construction

1c5edc1

changing qs_decomposition API to work with ArrayView2

a9183f7

using ndarray_to_faer

e61e164

remove aux function zxz_decomp_svd

093ed8b

updating minimum tol to 1e-12

443cb3c

alexanderivrii dismissed ShellyGarion’s stale review via 443cb3c March 26, 2026 15:48

alexanderivrii requested a review from mtreinish March 26, 2026 15:48

mtreinish reviewed Mar 31, 2026

View reviewed changes

Comment thread crates/synthesis/src/linalg/mod.rs Outdated

Comment thread crates/synthesis/src/linalg/mod.rs Outdated

Comment thread crates/synthesis/src/linalg/mod.rs Outdated

Comment thread crates/synthesis/src/linalg/mod.rs Outdated

Comment thread crates/synthesis/src/qsd.rs Outdated

Merge branch 'main' into reimplement-qsd-using-faer

6146989

alexanderivrii added 3 commits April 6, 2026 11:54

cloned -> copied

527894c

Replacing QSDError in mod.rs by LinAlgError

7408540

remove pub from ZXZResult

f932a20

alexanderivrii requested a review from mtreinish April 6, 2026 09:21

mtreinish reviewed Apr 9, 2026

View reviewed changes

improving user-facing error messages

91f40b5

alexanderivrii requested a review from mtreinish April 9, 2026 12:21

mtreinish approved these changes Apr 9, 2026

View reviewed changes

mtreinish added this pull request to the merge queue Apr 9, 2026

Merged via the queue into Qiskit:main with commit f8bd5d0 Apr 9, 2026
26 checks passed

github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 Apr 9, 2026

ShellyGarion mentioned this pull request Apr 20, 2026

Improve tolerance in cosine-sine decomposition #14422

Closed

Conversation

alexanderivrii commented Mar 25, 2026

Summary

Uh oh!

qiskit-bot commented Mar 25, 2026

Uh oh!

mtreinish commented Mar 25, 2026

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderivrii commented Mar 25, 2026

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development