Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port synth_cnot_phase_aam to Rust #12937

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

MozammilQ
Copy link
Contributor

@MozammilQ MozammilQ commented Aug 10, 2024

Summary

synth-cnot-aam-performance

Details and comments

fixes #12230

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

coveralls commented Aug 10, 2024

Pull Request Test Coverage Report for Build 11874552253

Details

  • 269 of 274 (98.18%) changed or added relevant lines in 4 files are covered.
  • 10 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.02%) to 88.964%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/synthesis/linear_phase/cnot_phase_synth.rs 245 250 98.0%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 2 92.23%
crates/qasm2/src/parse.rs 6 97.15%
Totals Coverage Status
Change from base Build 11842392021: 0.02%
Covered Lines: 79579
Relevant Lines: 89451

💛 - Coveralls

@alexanderivrii
Copy link
Contributor

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
Copy link
Contributor Author

MozammilQ commented Aug 11, 2024 via email

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

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

Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 28 to 32
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

Copy link
Contributor

@alexanderivrii alexanderivrii Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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.

@MozammilQ MozammilQ marked this pull request as ready for review August 30, 2024 06:02
@MozammilQ MozammilQ requested review from ShellyGarion and a team as code owners August 30, 2024 06:02
@qiskit-bot
Copy link
Collaborator

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

@MozammilQ MozammilQ changed the title [WIP]Port synth_cnot_phase_aam to Rust Port synth_cnot_phase_aam to Rust Aug 30, 2024
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@MozammilQ MozammilQ Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason for u8? 🙂

Copy link
Contributor Author

@MozammilQ MozammilQ Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its now bool

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((
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@MozammilQ MozammilQ Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is abstracted into a function now.

Comment on lines 40 to 43
let mut rust_angles: Vec<String> = angles
.iter()
.filter_map(|data| data.extract::<String>().ok())
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

}

let state_bool = state.mapv(|x| x != 0);
let mut instrs = _synth_cnot_count_full_pmh(state_bool, section_size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be mutable?

Copy link
Contributor Author

@MozammilQ MozammilQ Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the logic!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

.into_iter()
.rev()
.collect();
let mut instrs = synth_pmh(state_bool, section_size).rev().collect();
Copy link
Contributor

@Cryoris Cryoris Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,...);

Copy link
Contributor Author

@MozammilQ MozammilQ Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


let rust_angles = angles
.iter()
.filter_map(|data| data.extract::<String>().ok())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

@MozammilQ MozammilQ marked this pull request as draft November 15, 2024 13:48
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MozammilQ for your work on this. After taking a look at the paper and at our python code, this seems a very challenging task. Personally, I find the algorithm in the paper quite hard to understand, and (just from inspection) our Python code is somewhat messy and probably buggy. I hope this PR will evolve into something that does not just port the Python code line-by-line, but settles on a good API, adds proper unit testing, and reimplements the paper's algorithm using the best suitable Rust data structures.

First things first, I think we should decide on how this function should be used. Personally, I would like to see it's used both for Clifford+T synthesis and for general synthesis, and currently I don't see a clean approach to support both. I would like to hear your thoughts on this, as well as @ShellyGarion and @Cryoris.

Second, from what I could judge (I need to double-check this though) is that we have very few tests for this function. So maybe as a separate PR, let's add more tests to this function (hopefully exploring different corner-case scenarios), and fixing the python bugs (if any).

i wasn't able to fully understand the Rust code. Two immediate observations: you seem to be doing quite a bit of unnecessary cloning, and there are too many variables whose purpose I don't fully understand. An additional thing to consider: we seem to be constantly removing things from the middle of a vector? Can we make this more efficient using possibly different data structures?

Comment on lines +162 to +163
let mat: Array2<bool> = arrayview.to_owned();
let num_qubits = mat.nrows();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you are copying the matrix twice: first, when you call mat = arrayview.to_owned(), and second, when you call let mut mat = mat in synth_pmh.

Comment on lines +196 to +197
upper_cnots
.into_iter()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Comment on lines +31 to +34
@ddt.data(
(["s", "t", "z", "s", "t", "t"],),
# Angles applied on PhaseGate are 'angles%numpy.pi',
# So, to get PhaseGate(numpy.pi) we subtract a tiny value from pi.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code also supports adding some of the angles as strings (e.g. "s") and some of the angles as floating-point numbers (e.g., pi/4), could you add such a mixed test?

Comment on lines +4 to +5
Ported :func:`~.synth_cnot_phase_aam` to rust. The algorithm computes minimal-CNOT
circuit for a given phase-polynomial. The newly ported rust code has a speedup of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this is a heuristic algorithm and is not guaranteed to produce circuits with the minimum number of CX-gates.

Suggested change
Ported :func:`~.synth_cnot_phase_aam` to rust. The algorithm computes minimal-CNOT
circuit for a given phase-polynomial. The newly ported rust code has a speedup of
Ported :func:`~.synth_cnot_phase_aam` to Rust, which is a heuristic algorithm
for synthesizing small parity networks. The newly ported Rust code has a speedup of

Comment on lines +90 to +93
cnots_array = np.asarray(cnots).astype(np.uint8)
angles = [angle if isinstance(angle, str) else f"{angle}" for angle in angles]
_circuit_data = synth_cnot_phase_aam_xlated(cnots_array, angles, section_size)
return QuantumCircuit._from_circuit_data(_circuit_data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand this correctly. A floating-point number (e.g. 1.2) is translated here to a string (e.g. "1.2") and later (in the rust code) is translated back to a floating-point number. I don't particularly like this, but I don't see how we can improve this.

Hmm, we probably want the algorithm to be usable both for Clifford+T synthesis (and so we should have a way to pass a list of very specific angles corresponding to S, T, Sdg, Tdg, and Z gates ) and in general (and so we should also have a way to pass general angles corresponding to Phase gates). While we are at it, do we want to handle parametric angles as well? The algorithm itself works with completely arbitrary angles. Personally I am strongly in favor of supporting arbitrary angles, and this can be done in the Rust code similarly to the following:

fn inject_rotations(
py: Python,
gates: &[CliffordGate],
paulis: &PauliSet,
angles: &[Param],
preserve_order: bool,
) -> (Vec<QiskitGate>, Param) {
.

However, I don't see how to support both arbitrary angles and also very specific angles needed for Clifford+T.
@MozammilQ, @Cryoris, @ShellyGarion, what are your thoughts on all of this?

Comment on lines +51 to +55
angles_in_pi => (
StandardGate::PhaseGate,
smallvec![Param::Float((angles_in_pi.parse::<f64>().ok()?) % PI)],
smallvec![Qubit(qubit_idx as u32)],
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but see my questions elsewhere on whether we want to support general parameters (I think we do), and how we can do that and also support special gates required for Clifford+T synthesis (currently I don't see a good solution for this).

Comment on lines +88 to +89
let state = Array2::<u8>::eye(num_qubits);
let mut state = state.mapv(|x| x != 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first creates an identity matrix with u8 entries and then replaces it by an identity matrix with bool entries. See the following code for an alternative implementation:

let identity_matrix: Array2<bool> = Array2::from_shape_fn((n, n), |(i, j)| i == j);

Comment on lines +75 to +76
let s = cnots.as_array().to_owned();
let s = s.mapv(|x| x != 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would let s = cnots.as_array().map(|x| x != 0); work as well?

Another comment. Here in the following we need better names for the variables (or/and.a comment in code explaining the purpose of each variable). Personally I already find the algorithm in the paper quite hard to understand, our Python code even harder to understand (but fortunately it's close enough to the pseudo-code in the paper), and the new Rust code yet even harder to understand.

Comment on lines +97 to +110
let mut _s = s;
let mut _i = (0..num_qubits).collect::<Vec<usize>>();
let mut _ep = num_qubits;

// variables keeping the state of iteration
let mut keep_iterating: bool = true;
let mut cx_gate_done: bool = false;
let mut phase_loop_on: bool = false;
let mut phase_done: bool = false;
let mut cx_phase_done: bool = false;
let mut qubit_idx: usize = 0;
let mut index: usize = 0;
let mut pmh_init_done: bool = false;
let mut synth_pmh_iter: Option<Box<dyn Iterator<Item = Instruction>>> = None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I am lost here. What are all these variables?

Comment on lines +266 to +267
let cnots0 = cnots0.reversed_axes().to_owned();
let cnots1 = cnots1.reversed_axes().to_owned();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere in this function we seem to be doing a lot of cloning. Can we try to improve this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo Rust This PR or issue is related to Rust code in the repository synthesis
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Port synth_cnot_phase_aam to Rust
6 participants