Use nalgebra::Matrix4 as output for instructions_to_matrix#15871
Merged
alexanderivrii merged 2 commits intoQiskit:mainfrom Mar 25, 2026
Merged
Use nalgebra::Matrix4 as output for instructions_to_matrix#15871alexanderivrii merged 2 commits intoQiskit:mainfrom
alexanderivrii merged 2 commits intoQiskit:mainfrom
Conversation
mtreinish
added
on hold
Collaborator
|
One or more of the following people are relevant to this code:
|
Member
Author
|
I ran a pair of quick asv benchmarks, although I don't trust these numbers since it seems like I had a fair amount of system noise and the variability on these numbers seems high. I'll do more thorough benchmarking after the parent PR merges and this is unblocked. |
Member
alexanderivrii
left a comment
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Matthew, overall this looks good to me, just two minor questions.
Comment on lines
+94
to
+98
| let OperationRef::Gate(gate) = inst.op.view() else { | ||
| return Err(QiskitError::new_err( | ||
| "Can't compute matrix of non-unitary op", | ||
| )); | ||
| }; |
Member
There was a problem hiding this comment.
Should we also consider OperationRef::Operation variant here (and if so, then also in get_matrix_from_inst)?
We should also extend this to PauliProductRotation, but this can be done for all relevant functions in a separate PR.
Member
Author
There was a problem hiding this comment.
I just mirrored what was already there. I think we do want to expand this for any unitary operation. So this will be PPR and Operations that have a unitary matrix. Ideally we'd probably cover PPR and unitary OperationRef::Operations in PackedInstruction::try_matrix_as_nalgebra_2q this check is just the fallback for whether we call out to python and use quantum_info.Operator to compute the unitary from a gate's definition.
Member
Author
There was a problem hiding this comment.
That being said I think we should fix this in a separate PR that fixes all the matrix methods
Comment on lines
+47
to
+68
| static IDENTITY_2Q: Matrix4<Complex64> = Matrix4::new( | ||
| // Row 1 | ||
| Complex64::ONE, | ||
| Complex64::ZERO, | ||
| Complex64::ZERO, | ||
| Complex64::ZERO, | ||
| // Row 2 | ||
| Complex64::ZERO, | ||
| Complex64::ONE, | ||
| Complex64::ZERO, | ||
| Complex64::ZERO, | ||
| // Row 3 | ||
| Complex64::ZERO, | ||
| Complex64::ZERO, | ||
| Complex64::ONE, | ||
| Complex64::ZERO, | ||
| // Row 4 | ||
| Complex64::ZERO, | ||
| Complex64::ZERO, | ||
| Complex64::ZERO, | ||
| Complex64::ONE, | ||
| ); |
Member
There was a problem hiding this comment.
I like this matrix: no need to think whether the elements are passed in row-major or column-major order 😄
Member
Author
There was a problem hiding this comment.
Ideally I would have been able to use Matrix4::identity() here, but it's not a const function so I couldn't use it in a static context. This was the best way I could come up with, but I also do a double take every time I work with nalgebra about row major vs col major because I've gotten it backwards too many times.
This commit switches the return type of convert_2q_block_matrix::instructions_to_matrix() to return a nalgebra Matrix4 rather than a dynamicly allocated ndarray Array2. The function explicitly returns a 4x4 Complex64 matrix. Using an nalgebra fixed size array is stack allocated and we avoid needing a dynamic allocation. This should also speedup matrix multiplication since nalgebra can leverage simd better (either directly or implicitly via the compiler) because it knows the fixed operations needed. We were already setup towards doing since Qiskit#13649 which moved to using nalgebra internally for the 1q component, but it didn't update the whole path in that PR to use an nalgebra array for everything. Ideally we'd be using nalgebra data types throughout the two qubit decomposers too. We should still use faer for the more involved linear algebra operations in the module but we should be using Matrix4 and Matrix2 for fixed sized matrices where we know the size of the matrices in that module and generate faer MatRefs using qiskit_synthesis::linalg::nalgebra_to_faer() to do linear algebra with the matrix. This PR is an incremental step towards doing that.
mtreinish
force-pushed
the
use-nalgebra-arrays-for-consolidate
branch
from
29a718d to
35f8d27
Compare
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Mar 26, 2026
In the recently merged Qiskit#15871 we updated the consolidate blocks pass to use Matrix4 in the common path of a 2q block being consolidated. This is like > 90% of what the pass does when run in the preset pass manager. However, there were uncommon cases in the pass around the handling of blocks of a single gate that are outside of the target which were not updated to use nalgebra arrays if it's a fixed size 1q or 2q gate. This commit updates these uncommon paths so that we're always returning an nalgebra matrix in the output UnitaryGate if the block being consolidated is a single qubit or two qubits.
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Mar 27, 2026
In the recently merged #15871 we updated the consolidate blocks pass to use Matrix4 in the common path of a 2q block being consolidated. This is like > 90% of what the pass does when run in the preset pass manager. However, there were uncommon cases in the pass around the handling of blocks of a single gate that are outside of the target which were not updated to use nalgebra arrays if it's a fixed size 1q or 2q gate. This commit updates these uncommon paths so that we're always returning an nalgebra matrix in the output UnitaryGate if the block being consolidated is a single qubit or two qubits.
gadial
pushed a commit
to gadial/qiskit
that referenced
this pull request
Mar 29, 2026
…t#15881) In the recently merged Qiskit#15871 we updated the consolidate blocks pass to use Matrix4 in the common path of a 2q block being consolidated. This is like > 90% of what the pass does when run in the preset pass manager. However, there were uncommon cases in the pass around the handling of blocks of a single gate that are outside of the target which were not updated to use nalgebra arrays if it's a fixed size 1q or 2q gate. This commit updates these uncommon paths so that we're always returning an nalgebra matrix in the output UnitaryGate if the block being consolidated is a single qubit or two qubits.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This commit switches the return type of
convert_2q_block_matrix::instructions_to_matrix() to return a nalgebra
Matrix4 rather than a dynamicly allocated ndarray Array2. The function
explicitly returns a 4x4 Complex64 matrix. Using an nalgebra fixed size
array is stack allocated and we avoid needing a dynamic allocation. This
should also speedup matrix multiplication since nalgebra can leverage
simd better (either directly or implicitly via the compiler) because it
knows the fixed operations needed. We were already setup towards doing
since #13649 which moved to using nalgebra internally for the 1q
component, but it didn't update the whole path in that PR to use an
nalgebra array for everything.
Ideally we'd be using nalgebra data types throughout the two qubit
decomposers too. We should still use faer for the more involved linear
algebra operations in the module but we should be using Matrix4 and
Matrix2 for fixed sized matrices where we know the size of the matrices
in that module and generate faer MatRefs using
qiskit_synthesis::linalg::nalgebra_to_faer() to do linear algebra with
the matrix. This PR is an incremental step towards doing that.
Details and comments
This PR is based on top of #15858 and will need to be rebased after that merges. In the meantime you can view the contents of just this PR by looking at the HEAD commit: 29a718dRebased now