Switch Two qubit decompose to nalgebra#15017
Closed
gadial wants to merge 10 commits intoQiskit:mainfrom
Closed
Conversation
Collaborator
|
One or more of the following people are relevant to this code:
|
…ise in our code due to column vs row order)
Pull Request Test Coverage Report for Build 18377399152Details
💛 - Coveralls |
Contributor
Author
|
Current benchmarking results for QSD decomposition show a minor improvement, which is good when trying to set a baseline but far from enough at this stage: |
This commit removes the number of copies being made around nalgebra conversions. In places we use ndarray, faer, or fixed sized arrays and/or slices the conversion between the types was typically done via copying. While in some places the copying is desireable, such as going from a dynamic container type like an owned nalgebra Array2 or faer Mat to a fixed size type like Matrix2 or Matrix4. In other places, especially going from nalgebra -> ndarray or faer, we should be able to do the conversion without any copying. This commit makes these changes in the following ways: - going from nalgebra to ndarray we create an ArrayView2 pointing to the underlying Matrix storage. - going from nalgebra to faer this calls the ArrayView2 generator and then uses faer-ext to create a MatRef from the ArrayView2 - The `static [[Complex64; 2]]` and static `[[Complex64; 4]]` usage is replaced with direct `static Matrix2<Complex64>` and `static Matrix4<Complex64>`. This enables directly using the statics in combination with dynamically created matrices The conversion from faer and ndarray to owned matrix types As follow ons to this commit we should look at places where we are using faer (or to a lesser extent ndarray) and see if wrapping the return from faer functions in a dynamic MatrixView instead of an owned Matrix4 or Matrix2 makes sense. For example if the result is only used in computation with nalgebra types and never stored, then we don't need an owned Matrix object and doing faer -> MatrixView with no copies is potentially faster (see Qiskit#15059 for a conversion function doing that).
Member
|
Thanks for getting this started. I know that this is still a draft, but I started to review it. But I couldn't help myself and pushed some small changes that I think will help move this toward a better end state. This commit tries to reduce some of the copy overhead: 67e3e2a and shows some ideas for working between the three array/matrix/linear algebra libraries being used in the module now. I tried to keep the changes minimal here to not step on your toes while you're finishing this. If you don't like the commit feel free to revert it and continue working without my interjections. I'm running some benchmarks now. But I don't think we'll get most of the advantages of this migration until a follow up to this PR that leverages nalgebra for the |
Member
|
|
Member
|
From my local benchmark run it's showing a nice transpile speedup: |
In the previous commit the internal logic for the conversion from faer Mat to an owned Matrix4 was updated to avoid the overhead of checking the matrix shape on each execution by using a debug_assert. The function is only used internally with our generated matrices which should always be a 4x4 based on how it's called so the shape check is needless overhead. However, when the logic was moved from an if check with an error returned to a debug assert the condition wasn't updated accordingly. This caused debug builds to fail the assertion when the matrix was 4x4. This commit fixes this oversight and fixes the conversion function in debug mode.
Member
|
Is this PR still looking to merge, or did it get overtaken by other linear-algebra performance PRs? |
Member
|
Yeah, we can close this. It's been superseded by #15928 and #15960. The follow-on work I outlined in #15017 (comment) still needs to be done to take full advantage of the changes, but that will come after #15960 merges. |
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
7 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
Changing the two qubit decompose (and related files) to work with
Matrix4andMatrix2instead of arbitrary sized matrices.Details and comments
Fix #13665