Port synth_clifford_bm to Rust.
#12714
Merged
Conversation
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
…rices by significantly cheaper in-place prepend operations
|
One or more of the following people are relevant to this code:
|
alexanderivrii
commented
Jul 3, 2024
Comment on lines
+149
to
+151
| for qubit0 in 0..cliff.num_qubits { | ||
| for qubit1 in qubit0 + 1..cliff.num_qubits { | ||
| for (n0, n1) in iproduct!(0..3, 0..3) { |
There was a problem hiding this comment.
Feedback wanted! I very much like the iproduct! macro as it allows to write concisely nested for loops. Would it be possible to put for loops for qubit0 and qubit1 inside the same macro? Note that qubit1 iterates from qubit0+1.
There was a problem hiding this comment.
With iproduct probably not as qubit0 and qubit1 are not independent... unless you'd want to do something like iproduct!(0..n, 0..n, 0..3, 0..3).filter(|(q0, q1, n0, n1)| q1 > q0) -- but it seems to me being explicit like you have it is benevolent for people reading the code in the future 😉
alexanderivrii
commented
Jul 3, 2024
Comment on lines
79
to
84
| let xs = clifford.tableau.slice(s![q1, 0..6]).to_owned(); | ||
| let zs = clifford.tableau.slice(s![q1 + 3, 0..6]).to_owned(); | ||
| let ys = xs.clone() ^ zs.clone(); | ||
| let is_loc_x = (xs.clone() & mask.clone()) == xs; | ||
| let is_loc_z = (zs.clone() & mask.clone()) == zs; | ||
| let is_loc_y = (ys.clone() & mask.clone()) == ys; |
There was a problem hiding this comment.
Feedback wanted!. This is almost a 1-1 rewrite of the Python code. I had to use to_owned and clone to make this compile. This is probably not a huge deal since all the arrays have only 6 elements, but I would like to get rid of this extra copying if possible. I also do not understand why let ys = xs ^ zx; does not compile (it seems that it needs to move both xs or ys). Of course, checking that x & mask == x is equivalent to checking that x has no nonzero-elements on non-masked qubits, leading to an alternative implementation, but the current form is probably a bit clearer.
There was a problem hiding this comment.
I think you can get away with passing most of these by reference, even your views of xs and zs. Try this:
Suggested change
| let xs = clifford.tableau.slice(s![q1, 0..6]).to_owned(); | |
| let zs = clifford.tableau.slice(s![q1 + 3, 0..6]).to_owned(); | |
| let ys = xs.clone() ^ zs.clone(); | |
| let is_loc_x = (xs.clone() & mask.clone()) == xs; | |
| let is_loc_z = (zs.clone() & mask.clone()) == zs; | |
| let is_loc_y = (ys.clone() & mask.clone()) == ys; | |
| let xs = clifford.tableau.slice(s![q1, 0..6]); | |
| let zs = clifford.tableau.slice(s![q1 + 3, 0..6]); | |
| let ys = &xs ^ &zs; | |
| let is_loc_x = (&xs & &mask) == xs; | |
| let is_loc_z = (&zs & &mask) == zs; | |
| let is_loc_y = (&ys & &mask) == ys; |
There was a problem hiding this comment.
Thank you, @raynelfss! This solves the problem. Done in a89ed9e.
Pull Request Test Coverage Report for Build 9773370551Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9775748915Details
💛 - Coveralls |
ShellyGarion
reviewed
Jul 8, 2024
Pull Request Test Coverage Report for Build 10004179783Warning: 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 |
Cryoris
approved these changes
Jul 19, 2024
| /// for a 3-qubit Clifford. | ||
| fn cx_cost3(clifford: &Clifford) -> usize { | ||
| // create information transfer matrices r1, r2 | ||
| let mut r1 = arr2(&[[0, 0, 0], [0, 0, 0], [0, 0, 0]]); |
There was a problem hiding this comment.
You could also use the designated Array::zero function here if you want to type less 0s, as
Suggested change
| let mut r1 = arr2(&[[0, 0, 0], [0, 0, 0], [0, 0, 0]]); | |
| let mut r1 = Array2::<u32>::zeros((3, 3)); |
🙂
Comment on lines
+149
to
+151
| for qubit0 in 0..cliff.num_qubits { | ||
| for qubit1 in qubit0 + 1..cliff.num_qubits { | ||
| for (n0, n1) in iproduct!(0..3, 0..3) { |
There was a problem hiding this comment.
With iproduct probably not as qubit0 and qubit1 are not independent... unless you'd want to do something like iproduct!(0..n, 0..n, 0..3, 0..3).filter(|(q0, q1, n0, n1)| q1 > q0) -- but it seems to me being explicit like you have it is benevolent for people reading the code in the future 😉
ShellyGarion
approved these changes
Jul 21, 2024
ElePT
pushed a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jul 24, 2024
* initial commit * release notes * fixing synthesis plugin options * finalize merge conflicts * fixing default option values for qft plugins' * additional tests for qft plugins * starting to experiment * porting code * messy code porting * printing statements to enable debugging * fixes * fixing phase * removing some of the printing statements * fixing inaccuracy for cost computation * Moving some of the functionality to SymplecticMatrix class * reducing the number of warnings * formatting * replacing expensive adjoint and compose operations for symplectic matrices by significantly cheaper in-place prepend operations * resolving merge conflicts * cleanup * code cleanup * cleanup * cleanup * cleanup * cleanup * cleanup * cleanup * cleanup * using fast lookup * cleanup * clippy * including params in gate_seq to avoid mapping * removing unnecessary inner function * cleanup * renaming * changes on the python side * reno * adding error handling * improved error handling * removing redundant Ok(Some(...)) * using random_clifford in tests * reorganizing clifford code * fixes * formatting * improved error handling * do not panic * formatting * Applying refactoring suggestions d/utils.rs from code review * release notes update * starting to port code * continuing to port code * porting + fixing * polishing * changes on the python side; modifying tests to include missing testcases and to use random_clifford instead of random_clifford_circuit since the former provides better randomness guarantees * release notes (last but not least) * Correct reported speedups * applying suggestion from code review * resolving even more merge conflicts
Procatv
pushed a commit
to Procatv/qiskit-terra-catherines
that referenced
this pull request
Aug 1, 2024
* initial commit * release notes * fixing synthesis plugin options * finalize merge conflicts * fixing default option values for qft plugins' * additional tests for qft plugins * starting to experiment * porting code * messy code porting * printing statements to enable debugging * fixes * fixing phase * removing some of the printing statements * fixing inaccuracy for cost computation * Moving some of the functionality to SymplecticMatrix class * reducing the number of warnings * formatting * replacing expensive adjoint and compose operations for symplectic matrices by significantly cheaper in-place prepend operations * resolving merge conflicts * cleanup * code cleanup * cleanup * cleanup * cleanup * cleanup * cleanup * cleanup * cleanup * using fast lookup * cleanup * clippy * including params in gate_seq to avoid mapping * removing unnecessary inner function * cleanup * renaming * changes on the python side * reno * adding error handling * improved error handling * removing redundant Ok(Some(...)) * using random_clifford in tests * reorganizing clifford code * fixes * formatting * improved error handling * do not panic * formatting * Applying refactoring suggestions d/utils.rs from code review * release notes update * starting to port code * continuing to port code * porting + fixing * polishing * changes on the python side; modifying tests to include missing testcases and to use random_clifford instead of random_clifford_circuit since the former provides better randomness guarantees * release notes (last but not least) * Correct reported speedups * applying suggestion from code review * resolving even more merge conflicts
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
Closes #12234.
This ports the function
synth_clifford_bmto Rust. Recall that this function optimally synthesizesCliffordoperators on 1, 2 or 3 qubits with respect to the number of CX-gates.Here are the performance comparisons on my laptop between Python and Rust versions for 1000 random
n-qubit Cliffords, withn=1, 2, 3.1000 random 1-qubit cliffords:
Python version: 0.05
Rust version: 0.02
Improvement: 2.5x
1000 random 2-qubit cliffords:
Python version: 14.68
Rust version: 0.03
Improvement: 400x.
1000 random 3-qubit cliffords:
Python version: 30.95
Rust version: 0.38
Improvement: 80x
Details and comments
The circuits produced by the Python and Rust versions should be identical (up to register naming).
This is an almost line-by-line port of the Python file. The only minor difference is that the function
reduce_costreturns the list of inverted gates, so that the gates in the final circuit only needs to be reversed and not both reversed and inverted.There are a few places where I think the Rust code could be improved, but I did not find a good solution. I will highlight this as separate comments and ask for feedback/suggestions.