Port cx_cz_lnn to rust #13867
Port cx_cz_lnn to rust #13867
Conversation
|
One or more of the following people are relevant to this code:
|
| cir.push(CircuitInstructions::CX(w1 as u32, w2 as u32)); | ||
| } | ||
| for i in 0..n { | ||
| match phase_schedule[[n - 1 - i, n - 1 - i]] { |
There was a problem hiding this comment.
I see now I missed the % 4 here and it wasn't caught by testing; I'll extend the tests to ensure this is caught.
There was a problem hiding this comment.
It seems this cannot have any real effect since the values in the phase schedule are always computed modulo 4 so the code is technically correct, although I'll add the % 4 since it's better to be on the safe side.
There was a problem hiding this comment.
I guess you could also use u4 for representing phases, and use wrapping_add to perform modular arithmetic. But I don't think it's worth the hassle.
Pull Request Test Coverage Report for Build 13569108809Warning: 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 |
gadial
added
performance
synthesis
Rust
|
Could you perhaps also compare the time improvement of the function |
When running on random cliffords, I'm getting a nice improvement but I suspect there's more we can do. |
|
Doing quick profiling of 1.2 vs 2.0: 1.2: 2.0: It seems the bottleneck now is handling the large number of |
There was a problem hiding this comment.
This is a very-very cool PR, that ports the existing Python code to Rust using highly advanced (or at least what I consider highly advanced) Rust features such as flat_map and scan methods for iterators. The code is quite similar to the original Python code, modular rewriting it in the Rustacean way, making it almost possible and yet a bit difficult (at least for me) to guarantee the formal equivalence between the two versions just by visual inspection. Fortunately, we have quite extensive tests, and based on that, the code is correct.
My main comments is to include a short summary of the performance improvement in the release notes and double-check that the function docstrings render well. Thanks for the great work!
crates/accelerate/src/synthesis/linear_phase/cx_cz_depth_lnn.rs
Outdated
Show resolved
Hide resolved
| /// (c.f. Fig.2, [2]) | ||
| fn _make_seq(n: usize) -> Vec<(usize, usize)> { | ||
| (0..n) | ||
| .scan((0..n).rev().collect::<Vec<usize>>(), |wire_labels, i| { |
| cir.push(CircuitInstructions::CX(w1 as u32, w2 as u32)); | ||
| } | ||
| for i in 0..n { | ||
| match phase_schedule[[n - 1 - i, n - 1 - i]] { |
There was a problem hiding this comment.
I guess you could also use u4 for representing phases, and use wrapping_add to perform modular arithmetic. But I don't think it's worth the hassle.
| --- | ||
| features_transpiler: | ||
| - | | ||
| The :meth:`.synth_cx_cz_depth_line_my` pass was ported into rust. |
There was a problem hiding this comment.
I think you are missing a line at the end of the file.
There was a problem hiding this comment.
Also, could you please add a short statement on the runtime improvement?
There was a problem hiding this comment.
Done in dc9bbd1, I hope that's enough
| /// Given | ||
| /// Width of the circuit (int n) | ||
| /// A CZ circuit, represented by the n*n phase schedule phase_schedule | ||
| /// A CX circuit, represented by box-labels (seq) and whether the box is SWAP+ (swap_plus) | ||
| /// * This circuit corresponds to the CX tranformation that tranforms a matrix to | ||
| /// a NW matrix (c.f. Prop.7.4, [1]) | ||
| /// * SWAP+ is defined in section 3.A. of [2]. | ||
| /// * As previously noted, the northwest diagonalization procedure of [1] consists | ||
| /// of exactly n layers of boxes, each being either a SWAP or a SWAP+. That is, | ||
| /// each northwest diagonalization circuit can be uniquely represented by which | ||
| /// of its n(n-1)/2 boxes are SWAP+ and which are SWAP. | ||
| /// Return a QuantumCircuit that computes the phase schedule S inside CX |
There was a problem hiding this comment.
Here and in other places: I am not completely sure whether this style of iterative indentation renders well, and in addition I think I remember from somewhere that using - is preferred over using * for lists. Bottom line: could you please double-check how these Rust doc comments look like, and if they look well, nothing needs to be done.
There was a problem hiding this comment.
And you can also use backticks (``n``) to highlight n and other variables.
There was a problem hiding this comment.
Done in dc9bbd1. I used cargo doc --document-private-items to verify the new version looks ok (you were correct and the original version looked bad).
Indeed, it seems that except of the 3 synthesis functions ( |
Summary
Ports the
synth_cx_cz_depth_line_mymethod to Rust.Closes #12231
Details and comments
This is synthesis pass for constructing circuits given by two matrices, representing CX and CZ circuits. The code generating the instruction list was ported to Rust in a straightforward manner; construction of the circuit itself is done in Python.
A quick benchmarking was performed by using
random_invertible_binary_matrix(n, seed=seed)of the CX matrix andnp.triu(np.random.randint(0, 2, (n, n)), k=1)for the CZ matrix, and timing the application ofsynth_cx_cz_depth_line_myon the result, showing consistent improvement for the rust version.