Port synth_clifford_greedy to Rust
#12601
Merged
+801
−308
Conversation
This file contains 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
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 9564063418Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9567144979Details
💛 - Coveralls |
…rices by significantly cheaper in-place prepend operations
Pull Request Test Coverage Report for Build 9577770854Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9597167430Warning: 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 |
Pull Request Test Coverage Report for Build 9641284102Warning: 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 |
Pull Request Test Coverage Report for Build 9722822853Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9725700405Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9725842348Details
💛 - Coveralls |
|
This is ready for review (@ShellyGarion, @Cryoris). All suggestions to make the code even faster or more "rusty" are highly welcome. |
There was a problem hiding this comment.
The code looks great! My main comment is that we should think a little bit on the API, since it will be used for other Clifford synthesis methods as well as to port the Clifford class to rust later (#12367)
| struct Clifford { | ||
| /// Number of qubits. | ||
| num_qubits: usize, | ||
| /// Matrix with dimensions (2 * num_qubits) x (2 * num_qubits + 1). |
There was a problem hiding this comment.
what's the reason to have the phases as the last line of the matrix and not as a separate vector (unlike the current Clifford class)? will it be useful later?
There was a problem hiding this comment.
Hmm, this is an interesting suggestion, which has its pros and cons. The current form is consistent with how we store a Clifford tableau in Python, but we can change it of course. It is also easier to perform row operations directly on the (2n) x (2n+1) matrix.
releasenotes/notes/oxidize-synth-clifford-greedy-0739e9688bc4eedd.yaml
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 9741012360Details
💛 - Coveralls |
ShellyGarion
reviewed
Jul 1, 2024
Pull Request Test Coverage Report for Build 9741620005Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9743540137Warning: 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 |
ShellyGarion
approved these changes
Jul 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Addresses #12235.
This PR ports the method
synth_clifford_greedyfor synthesizingCliffordoperators to Rust. In the process the algorithm was additionally improved by (1) ignoring all (expensive!) clifford phase computations till the very end, thus working on the "symplectic matrix" part of the tableau, (2) completely avoiding (expensive!) cliffordevolveand cliffordadjointoperations by pre-composing rather than post-composing the current Clifford with elementary Cliffords (corresponding to CX, SWAP, S, and H gates), (3) almost completely avoiding copying clifford objects, (4) fixing an inefficiency in cost computations.Runtime improvements:
On my laptop, the old python code takes about 400 seconds to synthesize 50 random 50-qubit Cliffords, and the new code takes 0.25 seconds to do the same. So it's at least 1000x improvement in performance.
Quality improvements:
After fixing an inaccuracy in cost computations, the number of SWAPs introduced by the new algorithm is smaller than before; the totals for the 50 random cliffords from before are: