Port synth_cnot_phase_aam to Rust #12937

MozammilQ · 2024-08-10T00:35:12Z

Summary

Details and comments

coveralls · 2024-08-10T01:01:04Z

Pull Request Test Coverage Report for Build 11683949353

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

320 of 327 (97.86%) changed or added relevant lines in 4 files are covered.
446 unchanged lines in 21 files lost coverage.
Overall coverage increased (+0.08%) to 88.808%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs	299	306	97.71%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/two_qubit_decompose.rs	1	92.09%
crates/accelerate/src/unitary_synthesis.rs	1	92.2%
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py	1	94.54%
qiskit/circuit/library/pauli_evolution.py	2	96.08%
qiskit/circuit/library/data_preparation/pauli_feature_map.py	2	91.8%
qiskit/primitives/backend_sampler_v2.py	2	98.53%
qiskit/synthesis/evolution/suzuki_trotter.py	3	92.5%
crates/qasm2/src/lex.rs	3	92.73%
qiskit/transpiler/passmanager_config.py	3	96.1%
qiskit/synthesis/evolution/product_formula.py	4	94.52%

Totals
Change from base Build 11617024653:	0.08%
Covered Lines:	77012
Relevant Lines:	86717

💛 - Coveralls

alexanderivrii · 2024-08-11T13:27:39Z

Thanks for working on this! I have not yet looked closely at the PR (and it's marked as work-in-progress), but I would like to suggest to set up a proper directory structure from the beginning. You have added a new linear_phase directory (which is great), and now you want to add mod.rs to that (and not modify files in linear directory). Please feel free to ask any additional questions.

MozammilQ · 2024-08-11T14:07:34Z

Thanks Noted !!!

…

On Sun, 11 Aug, 2024, 6:58 pm Alexander Ivrii, ***@***.***> wrote: Thanks for working on this! I have not yet looked closely at the PR (and it's marked as work-in-progress), but I would like to suggest to set up a proper directory structure from the beginning. You have added a new linear_phase directory (which is great), and now you want to add mod.rs to that (and not modify files in linear directory). Please feel free to ask any additional questions. — Reply to this email directly, view it on GitHub <#12937 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANBTUUIEN2PKYVENG4DGVHDZQ5RGDAVCNFSM6AAAAABMJKPVUSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBSG43DCNJUGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ShellyGarion · 2024-08-21T07:05:43Z

I see that you have some lint issues, and would like to suggest to use cargo clippy.
In addition, could you compare the performance of your rust code with the previous python code?
you should then compile it in the release mode: python setup.py build_rust --inplace --release

MozammilQ · 2024-08-21T07:09:48Z

Thanks for both the information, I think both probably are not mentioned in the contributing.md for qiskit

…

On Wed, 21 Aug, 2024, 12:36 pm Shelly Garion, ***@***.***> wrote: I see that you have some lint issues, and would like to suggest to use cargo clippy. In addition, could you compare the performance of your rust code with the previous python code? you should then compile it in the release mode: python setup.py build_rust --inplace --release — Reply to this email directly, view it on GitHub <#12937 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANBTUUM66T4Z5NQCE4AFHFDZSQ353AVCNFSM6AAAAABMJKPVUSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBRGI4TCOBRGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

alexanderivrii

This looks like a nice progress. I have briefly experimenting with your code: please see a more detailed comment above.

alexanderivrii · 2024-08-27T10:55:49Z

qiskit/synthesis/linear_phase/cnot_phase_synth.py

+def synth_cnot_pahse_aam_xlated(cnots: list[list[int]], angles: list[str], section_size:int 2) -> QuantumCircuit
+    _circuit_data = synth_cnot_phase_aam_xlated(cnots, angles, section_size)
+    qc_from_rust = QuantumCircuit._from_circuit_data(_circuit_data)
+    return qc_from_rust
+


This function has a small typo, you want section_size: int = 2.

More importantly, there are still some problems with passing cnots and angles from the Python code to the Rust code. I am also learning Rust/Pyo3 and am not sure if the following is optimal, but one possible way to fix these is to modify the Python function to

def synth_cnot_phase_aam_xlated(cnots: list[list[int]], angles: list[str], section_size: int = 2) -> QuantumCircuit: cnots = np.asarray(cnots, dtype=np.uint8) _circuit_data = _synth_cnot_phase_aam_xlated(cnots, angles, section_size) qc_from_rust = QuantumCircuit._from_circuit_data(_circuit_data) return qc_from_rust

and the Rust function to:

#[pyfunction] #[pyo3(signature = (cnots, angles, section_size=2))] pub fn synth_cnot_phase_aam( py: Python, cnots: PyReadonlyArray2<u8>, angles: &Bound<PyList>, section_size: Option<i64>, ) -> PyResult<CircuitData> { ... let mut rust_angles: Vec::<String> = Vec::new(); for angle in angles.iter() { let rust_string: String = angle.extract()?; rust_angles.push(rust_string); } ...

(you could write the above loop more compactly, but let's not worry about this right now).

Now running the first example in test_cnot_phase_synthesis.py makes the Rust code throw a panic exception, so you will need to debug what's going on.

One pitfall that I noticed when experimenting with the Python version of synth_cnot_phase_aam is that it clears the list of angles passed to it as input, so pay attention to this. :)

MozammilQ · 2024-08-30T06:02:23Z

@ShellyGarion ,
I want to publicly apologize for the delay in getting this PR on track.
Qiskit Challenge and QGSS happened to be extra interesting this time.
and, dealing with 'cuQuantum' is no less interesting either.

@HuangJunye ,
The credit goes to Junye for any good that came out of this PR, because he gave me his precious time and motivated me to learn rust in QAMP.
Thanks Junye Huang for all your time. :)

@alexanderivrii ,
Thanks for your previous comments and guidance, I have tried to maintain a balance between performance, simplicity and code-readability, please let me know if something needs to change.

qiskit-bot · 2024-08-30T06:02:30Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@kevinhartman
@mtreinish

Cryoris

I didn't finish all of it yet, but here's already some comments 🙂

Cryoris · 2024-08-30T14:58:37Z

crates/accelerate/src/synthesis/linear/pmh.rs

+}
+
+type Instructions = (StandardGate, SmallVec<[Param; 3]>, SmallVec<[Qubit; 2]>);
+pub fn _synth_cnot_count_full_pmh(mat: Array2<bool>, sec_size: Option<i64>) -> Vec<Instructions> {


Some comments on this:

It's likely better to keep this returning an iterator instead of collecting into a vector, this way the CircuitData constructor can simply iterate over the inputs. If you do need a vector you could collect where you call this function 🙂

Since this is a Rust internal function, it might be better to have section_size be a usize directly. We were only using i64 as input from Python, but logically, this should be an unsigned integer (also is there a reason for renaming? 🙂)

We might want to promote this as public function, so I would think we don't want a leading underscore in the name here. To avoid the naming conflict you could maybe rename it to synth_pmh?

1> Apart from the name changes and typo fix, I am reverting changes made in "156da41", there is a very tiny bit of performance cost, and the return type looked complex, so I decided to revert, please let me know if you want those changes anyway :)

2> I decided to keep it as Option<i64> because we have to convert from Options<i64> to Option<usize> in both occurrences in pmh.rs and cnot_phase_synth.rs as soon as we get data from python, and then send it to synth_pmh, furthermore its being converted to usize anyway in line: 185. Also no reason behind renaming it

3> Done!

Cryoris · 2024-08-30T15:04:54Z

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

+#[pyo3(signature = (cnots, angles, section_size=2))]
+pub fn synth_cnot_phase_aam(
+    py: Python,
+    cnots: PyReadonlyArray2<u8>,


Is there a specific reason for u8? 🙂

in cnot_phase_synth.rs we have _s.row(_j).sum() as usize == _s.row(_j).len(). and the trait num_traits::Zero is not implemented for bool. So, we need cnots to be some number, since it is only 0 or 1, I chose u8 to save on memory because 1 is the max value it can get.

Cryoris · 2024-08-30T15:05:54Z

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

+            let icnot = s_cpy.column(index).to_vec();
+            if icnot == state.row(qubit_idx).to_vec() {
+                match rust_angles.remove(index) {
+                    gate if gate == "t" => instructions.push((


Could this (and below) be written more concisely by matching the gate name to the StandardGate and then pushing the gate afterwards (to avoid repeating the same instruction creation)?

I chose not to change it, below code is slowing things down, from x309 to x290.

I tried doing it this way

let _data = match rust_angles.remove(index) { gate if gate == "t" => (StandardGate::TGate, smallvec![]), gate if gate == "tdg" => (StandardGate::TdgGate, smallvec![]), gate if gate == "s" => (StandardGate::SGate, smallvec![]), gate if gate == "sdg" => (StandardGate::SdgGate, smallvec![]), gate if gate == "z" => (StandardGate::ZGate, smallvec![]), angles_in_pi => ( StandardGate::PhaseGate, smallvec![Param::Float( (angles_in_pi.parse::<f64>()?) % PI )], ), }; instructions.push((_data.0, _data.1, smallvec![Qubit(_ep as u32)]));

Cryoris · 2024-08-30T15:17:26Z

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

+    let mut rust_angles: Vec<String> = angles
+        .iter()
+        .filter_map(|data| data.extract::<String>().ok())
+        .collect();


I know the Python code does the same, but this seems highly volatile and we should change this: the input type should be properly differentiated in either floats or strings, but not a mix. You can leave this for now @MozammilQ but maybe we can find a better solution @alexanderivrii and @ShellyGarion 🙂

Indeed, it's quite strange that the API of angles can accept strings e.g. s, t, sdg, tdg or z or float.
in the test we see that the angles are only strings, e.g. s, t, sdg, tdg or z. It would be good to add some tests of angles which are float to check that the code works well for any angle.

@ShellyGarion ,
I had already taken care of this by angles = [angle if isinstance(angle, str) else f"{angle}" for angle in angles] in cnot_phase_synth.py. This converts all occurrences of float in the list of angles to String which rust subsequently accepts.

Anyways, I have added a test. Take a note of the change in circuit. I have just changed the circuit in the docstring to the actual circuit I am getting here locally, but not have changed the circuit in the code, just to make sure the code works for the circuit generated from the older python implementation of synth_cnot_phase_aam, there by proving the circuit generated by rust is merely "equivalent" to the circuit generated by python version and not different.

Nevertheless, I am surprised because there is no random steps in the rust or the python implementation of the algorithm, so both the rust and python implementation should produce exact same circuit, but this is not the case.
Maybe, the difference is because when I append the output of synth_pmh I just do .rev() but not the actual .inverse() in python space as it has been done originally in python implementation.
I decided to do only .rev() thinking inverse of a cx is itself. So, just reversing the strings of cxs in the circuit should be equivalent to doing .inverse() on QuantumCircuit in python space. Please, guide me if you feel I am lost :)

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

Cryoris · 2024-08-30T15:24:21Z

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

+    }
+
+    let state_bool = state.mapv(|x| x != 0);
+    let mut instrs = _synth_cnot_count_full_pmh(state_bool, section_size)


Does this need to be mutable?

290 | instructions.append(&mut instrs); ^^^cannot borrow as mutable
because, append moves all data from instrs to instructions and empties instrs.

Cryoris · 2024-08-30T15:27:11Z

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

+        .iter()
+        .filter_map(|data| data.extract::<String>().ok())
+        .collect();
+    let mut state = Array2::<u8>::eye(num_qubits);


Could this be made into a matrix of bools right away? It seems like it is treated as one.

if icnot == state.row(_ep).to_vec(), so state has to be u8.

@Cryoris ,
just want to let you know that I value your comments and not agreeing to some is not questioning you rather its having an opportunity to learn, please guide me if you feel I am lost :)
I am still learning rust.

Also, I am measuring performance by taking average of 6 runs of code in the picture in the summary of this PR.
I am taking average to compensate for time cpu could spend doing something else.

Cryoris · 2024-09-02T08:55:53Z

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

-        .into_iter()
-        .rev()
-        .collect();
+    let mut instrs = synth_pmh(state_bool, section_size).rev().collect();


Is it possible to write the instructions as iterator, instead of creating it as vector and pushing each element? That would allow us to avoid the collections and have CircuitData directly consume the iterator (and likely be a bit faster 🙂)

let insts = instructions.chain(synth_pmh(...).rev()); CircuitData::from_standard_gates(..., insts,...);

I have implemented Iterator for all the loops in the logic, the performance seems to be same, but the code has become very complex. and, the program crashes for inputs when num_qubits * depth > 14900. (This is stack-overflow, it worked when I increased the stack to 40MB by doing ulimit -s 40960 )
at the end I have no option other than to collect these iterator into a vector, because I have to get the residual state out of the iterator to feed into synth_pmh which has to be chained into final instructor iterator.
Still, using vec.push(...) doesn't seem bad, please have a look at it and guide me what to change next :)

MozammilQ · 2024-11-06T06:16:25Z

crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs

+
+    let rust_angles = angles
+        .iter()
+        .filter_map(|data| data.extract::<String>().ok())


doing

.filter_map(|data| data.extract::<String>().or_else(|_| data.extract::<f64>().map(|f| f.to_string())).ok())

or, doing

angles = [angle if isinstance(angle, str) else f"{angle}" for angle in angles]

in qiskit/synthesis/linear_phase/cnot_phase_synth.py

takes the same amount of time to execute, so, I decided to keep this check in python space only! :)

initial commit

8da594c

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 10, 2024

added some code

193058b

ShellyGarion added synthesis Rust This PR or issue is related to Rust code in the repository labels Aug 15, 2024

MozammilQ and others added 5 commits August 22, 2024 14:50

refactor code

2ae9379

Merge branch 'main' into port-synth_cnot_phase_aam-to-rust

d7f41b7

done some cargo-clippy linting

60379b2

added code

671b634

added some more code

ae91425

alexanderivrii reviewed Aug 27, 2024

View reviewed changes

MozammilQ added 13 commits August 27, 2024 17:22

refactor code

64ce56b

refactor code

fe01589

refactor code

1ecab6d

refactor code

a217522

refactor code

4a36e1e

refactor code

0b681f9

refactor code

d2142f6

refactor code

8982c2c

removed associated python code

032b8c2

rust lint

583ca17

added release note; added docstring to rust algo

d1394f4

rust lint

6956e0f

refactor code

0091c98

MozammilQ marked this pull request as ready for review August 30, 2024 06:02

MozammilQ requested review from ShellyGarion and a team as code owners August 30, 2024 06:02

MozammilQ changed the title ~~[WIP]Port synth_cnot_phase_aam to Rust~~ Port synth_cnot_phase_aam to Rust Aug 30, 2024

Merge branch 'main' into port-synth_cnot_phase_aam-to-rust

92bf2aa

Cryoris reviewed Aug 30, 2024

View reviewed changes

MozammilQ and others added 3 commits August 31, 2024 19:20

Merge branch 'main' into port-synth_cnot_phase_aam-to-rust

6b2d31e

applied suggestion partially

156da41

reverting suggestions

b236d62

Cryoris reviewed Sep 2, 2024

View reviewed changes

MozammilQ and others added 2 commits September 4, 2024 07:51

added a test

d28da2a

Merge branch 'Qiskit:main' into port-synth_cnot_phase_aam-to-rust

4c32291

MozammilQ requested a review from alexanderivrii October 29, 2024 23:14

MozammilQ and others added 7 commits October 30, 2024 04:45

Merge branch 'main' into port-synth_cnot_phase_aam-to-rust

1c7ad1e

Merge branch 'Qiskit:main' into port-synth_cnot_phase_aam-to-rust

68917a0

replace vector with iterator

5973f85

refactor code; lint

353e5ff

refactor code

6262d16

refactor code

41a8906

refactor code

0cd01ce

MozammilQ commented Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port synth_cnot_phase_aam to Rust #12937

Port synth_cnot_phase_aam to Rust #12937

MozammilQ commented Aug 10, 2024 •

edited

Loading

coveralls commented Aug 10, 2024 •

edited

Loading

alexanderivrii commented Aug 11, 2024

MozammilQ commented Aug 11, 2024 via email

ShellyGarion commented Aug 21, 2024

MozammilQ commented Aug 21, 2024 via email

alexanderivrii left a comment

alexanderivrii Aug 27, 2024 •

edited

Loading

MozammilQ commented Aug 30, 2024

qiskit-bot commented Aug 30, 2024

Cryoris left a comment

Cryoris Aug 30, 2024

MozammilQ Sep 1, 2024 •

edited

Loading

Cryoris Aug 30, 2024

MozammilQ Sep 1, 2024

Cryoris Aug 30, 2024

MozammilQ Sep 1, 2024 •

edited

Loading

Cryoris Aug 30, 2024

ShellyGarion Sep 3, 2024

MozammilQ Sep 4, 2024

Cryoris Aug 30, 2024

MozammilQ Sep 1, 2024

Cryoris Aug 30, 2024

MozammilQ Sep 1, 2024

MozammilQ Sep 1, 2024

MozammilQ Sep 2, 2024

Cryoris Sep 2, 2024 •

edited

Loading

MozammilQ Sep 4, 2024 •

edited

Loading

MozammilQ Nov 6, 2024

Port synth_cnot_phase_aam to Rust #12937

Are you sure you want to change the base?

Port synth_cnot_phase_aam to Rust #12937

Conversation

MozammilQ commented Aug 10, 2024 • edited Loading

Summary

Details and comments

coveralls commented Aug 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11683949353

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

alexanderivrii commented Aug 11, 2024

MozammilQ commented Aug 11, 2024 via email

ShellyGarion commented Aug 21, 2024

MozammilQ commented Aug 21, 2024 via email

alexanderivrii left a comment

Choose a reason for hiding this comment

alexanderivrii Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

MozammilQ commented Aug 30, 2024

qiskit-bot commented Aug 30, 2024

Cryoris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MozammilQ Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MozammilQ Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cryoris Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

MozammilQ Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MozammilQ commented Aug 10, 2024 •

edited

Loading

coveralls commented Aug 10, 2024 •

edited

Loading

alexanderivrii Aug 27, 2024 •

edited

Loading

MozammilQ Sep 1, 2024 •

edited

Loading

MozammilQ Sep 1, 2024 •

edited

Loading

Cryoris Sep 2, 2024 •

edited

Loading

MozammilQ Sep 4, 2024 •

edited

Loading