Port synth_cnot_phase_aam to Rust #12937
Conversation
qiskit-bot
added
the
Community PR
Pull Request Test Coverage Report for Build 11874552253Details
💛 - Coveralls |
|
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 |
|
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
added
synthesis
Rust
|
I see that you have some lint issues, and would like to suggest to use |
| 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 | ||
|
|
There was a problem hiding this comment.
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. :)
|
@ShellyGarion , @HuangJunye , @alexanderivrii , |
|
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:
|
MozammilQ
changed the title
| } | ||
|
|
||
| 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> { |
There was a problem hiding this comment.
Some comments on this:
- It's likely better to keep this returning an iterator instead of collecting into a vector, this way the
CircuitDataconstructor 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_sizebe ausizedirectly. We were only usingi64as 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?
There was a problem hiding this comment.
1> Done!
2> Done! (Since, its internal function its indeed logical to have section_size as usize directly.)
3> Done!
| #[pyo3(signature = (cnots, angles, section_size=2))] | ||
| pub fn synth_cnot_phase_aam( | ||
| py: Python, | ||
| cnots: PyReadonlyArray2<u8>, |
There was a problem hiding this comment.
Is there a specific reason for u8? 🙂
| 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(( |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
The logic is abstracted into a function now.
| let mut rust_angles: Vec<String> = angles | ||
| .iter() | ||
| .filter_map(|data| data.extract::<String>().ok()) | ||
| .collect(); |
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| let state_bool = state.mapv(|x| x != 0); | ||
| let mut instrs = _synth_cnot_count_full_pmh(state_bool, section_size) |
There was a problem hiding this comment.
Does this need to be mutable?
There was a problem hiding this comment.
removed the logic!
| .iter() | ||
| .filter_map(|data| data.extract::<String>().ok()) | ||
| .collect(); | ||
| let mut state = Array2::<u8>::eye(num_qubits); |
There was a problem hiding this comment.
Could this be made into a matrix of bools right away? It seems like it is treated as one.
| .into_iter() | ||
| .rev() | ||
| .collect(); | ||
| let mut instrs = synth_pmh(state_bool, section_size).rev().collect(); |
There was a problem hiding this comment.
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,...);
|
|
||
| let rust_angles = angles | ||
| .iter() | ||
| .filter_map(|data| data.extract::<String>().ok()) |
There was a problem hiding this comment.
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! :)
Summary
Details and comments
fixes #12230