Port linear_depth_lnn to rust #13654
Conversation
gadial
requested review from
alexanderivrii,
ShellyGarion and
a team
as code owners
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 12747661672Details
💛 - Coveralls |
ShellyGarion
added
synthesis
Rust
There was a problem hiding this comment.
Thanks @gadial for your contribution! I only have some minor comments and questions.
| } | ||
|
|
||
| /// Get the instructions for a lower triangular basis change of a matrix mat. | ||
| /// See the proof of Proposition 7.3 in [1]. |
There was a problem hiding this comment.
there are extra spaces in the docstring
| } | ||
|
|
||
| /// Transform a north-west triangular matrix to identity in depth 3*n by Proposition 7.4 of [1] | ||
| fn _north_west_to_identity(n: usize, mut mat: ArrayViewMut2<bool>) -> InstructionList { |
| Some(result) | ||
| } | ||
| /// Perform ROW operation on a matrix mat | ||
| fn _row_op(mat: ArrayViewMut2<bool>, ctrl: usize, trgt: usize) { |
There was a problem hiding this comment.
we already have the function row_op here:
if you need a non pyo3 function, please also add it to the util file.
There was a problem hiding this comment.
I think my main question here is - should I use pyo3 function? It seems like adding overhead where it's not needed (but sure, I'll move to the utils file).
There was a problem hiding this comment.
you don't have to use the pyo3 function, but please add your new function to the util file, and see that the naming conventions are consistent (there are many functions there that have both rust and pyfunctions)
| } | ||
|
|
||
| /// Perform COL operation on a matrix mat (in the inverse direction) | ||
| fn _col_op(mat: ArrayViewMut2<bool>, ctrl: usize, trgt: usize) { |
There was a problem hiding this comment.
we already have the function col_op here:
if you need a non pyo3 function, please also add it to the util file.
| /// `arXiv:quant-ph/0701194 <https://arxiv.org/abs/quant-ph/0701194>`_. | ||
|
|
||
| type InstructionList = Vec<(usize, usize)>; | ||
| fn _row_sum(row_1: ArrayView1<bool>, row_2: ArrayView1<bool>) -> Option<Array1<bool>> { |
There was a problem hiding this comment.
I think this function should go into the util file:
https://github.com/Qiskit/qiskit/blob/main/crates/accelerate/src/synthesis/linear/utils.rs
could you write a docstring explaining what this function does?
There was a problem hiding this comment.
A small question: In the case that the two rows have different lengths, would it make sense to return an Error rather than None (that is, returning a Result rather than Option)?
| let mut mat = mat.to_owned(); | ||
| let mut mat_t = mat.to_owned(); | ||
| let mut mat_inv_t = mat_inv.to_owned(); | ||
|
|
There was a problem hiding this comment.
I agree, we aggressively call copy in the Python code, but it might be a good time to revisit if this excessive copying is really required.
There was a problem hiding this comment.
I have also thought about it. On the surface of it we need all those copies, but then again - maybe not.
mat_inv, in this stage, is a clone of the original matrix passed to the algorithm (it's cloned in line 305, in optimize_cx_circ_depth_5n_line). We pass it to _matrix_to_north_west and don't use it again in _north_west_to_identity. We also don't use it inside _matrix_to_north_west other than as the input to _get_lower_triangular so it is possible to have _get_lower_triangular altering it instead of creating a copy (this is less "clean", code-wise, so I didn't rush to do it).
As to the the clonings in 65 and 66 - I don't see a way around it. mat is changed in _matrix_to_north_west and used again later so it must be cloned prior to the changes done in _get_lower_triangular. Inside _get_lower_triangular itself, we modify mat to generate the instruction list, and then need to perform the row operations on mat_t in order to obtain the label array; this cannot be done on the original mat or the one after the changes.
| // Use the instructions in U, which contains only gates of the form cx(a,b) a>b | ||
| // to transform the matrix to a permuted lower-triangular matrix. | ||
| // The original Matrix is unchanged. | ||
| for i in (0..n).rev() { |
There was a problem hiding this comment.
can we replace some for loops by iterators? (I don't know how, just asking...)
There was a problem hiding this comment.
We definitely can, but I think this is a good tradeoff between code clarity and code "rustiness". 😄
There was a problem hiding this comment.
it's worth to do it for better performance, for the clarity one can add a comment.
There was a problem hiding this comment.
I actually think the iterator-based version can be easier to understand. compare
(0..i)
.rev()
.filter(|k| mat[[k, first_j]])
.for_each(|k| {
_row_op_update_instructions(&mut cx_instructions_rows, mat.view_mut(), i, k);
});With
for k in (0..i).rev() {
if mat[[k, first_j]] {
_row_op_update_instructions(&mut cx_instructions_rows, mat.view_mut(), i, k)
}While the latter is more concise, it's less explicit in describing what it actually does.
| /// For each row in mat_t, save the column index of the last "1" | ||
| fn _get_label_arr(n: usize, mat_t: ArrayView2<bool>) -> Vec<usize> { | ||
| let mut label_arr: Vec<usize> = Vec::new(); | ||
| for i in 0..n { |
There was a problem hiding this comment.
can we replace some for loops by iterators? (I don't know how, just asking...)
There was a problem hiding this comment.
just for fun, you could probably do something like this (I am not sure this compiles or returns the correct result):
fn _get_label_arr(n: usize, mat_t: ArrayView2<bool>) -> Vec<usize> {
mat
.rows()
.into_iter()
.map(|i|i.iter().rposition(|&r| r == true).unwrap())
.collect()
}
|
I also think we can add some release notes saying that there was a performance improvement in synth_cnot_depth_line_kms since it's implementation was ported to rust |
| /// Optimize the synthesis of an n-qubit circuit contains only CX gates for | ||
| /// linear nearest neighbor (LNN) connectivity. | ||
| /// The depth of the circuit is bounded by 5*n, while the gate count is approximately 2.5*n^2 | ||
| /// | ||
| /// References: | ||
| /// [1]: Kutin, S., Moulton, D. P., Smithline, L. (2007). | ||
| /// Computation at a Distance. | ||
| /// `arXiv:quant-ph/0701194 <https://arxiv.org/abs/quant-ph/0701194>`_. |
There was a problem hiding this comment.
I think you mean this as a regular comment, and not as a docstring comment for InstructionList. If so, it should preceded by //, not by ///. In addition, it may make sense to move the reference to the paper to the docstring of optimize_cx_circ_depth_5n_line.
| /// `arXiv:quant-ph/0701194 <https://arxiv.org/abs/quant-ph/0701194>`_. | ||
|
|
||
| type InstructionList = Vec<(usize, usize)>; | ||
| fn _row_sum(row_1: ArrayView1<bool>, row_2: ArrayView1<bool>) -> Option<Array1<bool>> { |
There was a problem hiding this comment.
A small question: In the case that the two rows have different lengths, would it make sense to return an Error rather than None (that is, returning a Result rather than Option)?
| let mut mat = mat.to_owned(); | ||
| let mut mat_t = mat.to_owned(); | ||
| let mut mat_inv_t = mat_inv.to_owned(); | ||
|
|
There was a problem hiding this comment.
I agree, we aggressively call copy in the Python code, but it might be a good time to revisit if this excessive copying is really required.
| // Use the instructions in U, which contains only gates of the form cx(a,b) a>b | ||
| // to transform the matrix to a permuted lower-triangular matrix. | ||
| // The original Matrix is unchanged. | ||
| for i in (0..n).rev() { |
There was a problem hiding this comment.
We definitely can, but I think this is a good tradeoff between code clarity and code "rustiness". 😄
| /// For each row in mat_t, save the column index of the last "1" | ||
| fn _get_label_arr(n: usize, mat_t: ArrayView2<bool>) -> Vec<usize> { | ||
| let mut label_arr: Vec<usize> = Vec::new(); | ||
| for i in 0..n { |
There was a problem hiding this comment.
just for fun, you could probably do something like this (I am not sure this compiles or returns the correct result):
fn _get_label_arr(n: usize, mat_t: ArrayView2<bool>) -> Vec<usize> {
mat
.rows()
.into_iter()
.map(|i|i.iter().rposition(|&r| r == true).unwrap())
.collect()
}| #[pyfunction] | ||
| #[pyo3(signature = (mat))] | ||
| pub fn optimize_cx_circ_depth_5n_line( | ||
| _py: Python, | ||
| mat: PyReadonlyArray2<bool>, | ||
| ) -> PyResult<(InstructionList, InstructionList)> { |
There was a problem hiding this comment.
For the rust function to be entirely self-contained and for consistency with other synthesis functions, it would be nice to return PyResult<CircuitData>.
| @@ -33,7 +33,7 @@ | |||
|
|
|||
| from qiskit.circuit import QuantumCircuit | |||
| from qiskit.synthesis.linear.linear_matrix_utils import calc_inverse_matrix | |||
| from qiskit.synthesis.linear.linear_depth_lnn import _optimize_cx_circ_depth_5n_line | |||
| from qiskit._accelerate.synthesis.linear import optimize_cx_circ_depth_5n_line | |||
There was a problem hiding this comment.
is this change needed, i.e. shouldn't the relevant function be importable from qiskit.synthesis.linear.linear_depth_lnn?
There was a problem hiding this comment.
This is not the "main" function in the sense that it is not the one that generates the circuit, but the helper method that generates the instruction lists (as lists of pairs), so I'm not sure if we should wrap it as well.
| cx_inst = optimize_cx_circ_depth_5n_line(mat) | ||
| qc = QuantumCircuit(num_qubits) |
There was a problem hiding this comment.
it would be nice to have all the code to construct the quantum circuit be entirely in rust.
| cx_instructions_rows_m2nw, cx_instructions_rows_nw2id = _optimize_cx_circ_depth_5n_line(mat_x) | ||
| cx_instructions_rows_m2nw, cx_instructions_rows_nw2id = optimize_cx_circ_depth_5n_line(mat_x) |
There was a problem hiding this comment.
Ah, I see the problem: we actually want to have a method that returns both lists. So maybe we can expose two different functions from the Rust crate (the one that returns a pair of lists and the one that returns the full circuit), what do you think?
Summary
Ports the
optimize_cx_circ_depth_5n_linemethod to Rust.Closes #12228
Details and comments
This is synthesis pass for constructing general linear reversible circuits given by an invertible boolean matrix. 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)and timing the application ofsynth_cnot_depth_line_kmson the result, showing consistent improvement for the rust version.