-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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_cz_depth_line_mr
to Rust
#12949
Port synth_cz_depth_line_mr
to Rust
#12949
Conversation
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
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:
|
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
…ti/qiskit into port-synth-cz-depth-line-mr
Pull Request Test Coverage Report for Build 10465567689Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlofti - thank you very much for your contribution to Qiskit!
I have a few preliminary comments, about the code structure and performance.
First, could you please transfer your code to a new directory called:
crates/accelerate/src/synthesis/linear_phase
(as CZ circuits are not linear).
Note that there is another algorithm to synthesize linear circuits for LNN (see also #12228)
In addition, could you perhaps compare the performance of the new rust and original python code for CZ circuits of different sizes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @jlofti! Could you maybe run a small benchmark comparing the runtime of the old Python code and the new Rust implementation? It can be something a simple as taking a random binary matrix (rather large, maybe 1000x1000?) and running both codes from the Python interface 🙂
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Hi @ShellyGarion @Cryoris , Thanks for all the feedback! Made all the changes, hopefully everything will come back good in CI/CD. My machine was having trouble but for a binary matrix of 350x350 I get the results: Rust: 308s Please let me know if everything looks good or if more changes should be made! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jlofti for handling our comments.
-
Usually, the rust code should be much faster than python. Could you perhaps check why this is not the case here?
-
Could you please update the current reverse permutation code to use rust code?
-
Could you please add release notes stating that the CZ synthesis code and reverse permutation code for LNN were ported to rust (you can see examples in previous PRs) ?
Hi @jlofti, when you compile the rust code, do you compile it in the release mode?
|
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Thats exactly what happened lol. Redid the benchmarking and Rust takes about 10s. So seeing around 30x speedup. Also added release notes and changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is in a great shape. I had a minor nitpick about promoting comments to docstrings and some suggestions for the release notes, but other than that it looks ready. The 30x improvement in performance is nice :).
releasenotes/notes/port-synth-cz-depth-line-mr-to-rust-1376d5a41948112a.yaml
Outdated
Show resolved
Hide resolved
…41948112a.yaml Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Signed-off-by: jlofti <65878716+jlofti@users.noreply.github.com>
Thanks! Promoted the comments to docstrings and added the ones you requested 👍 |
Co-authored-by: Julien Gacon <gaconju@gmail.com>
@ShellyGarion @Cryoris @alexanderivrii Hi all, was looking for guidance on how to best deduce whats happening with the mac python 3.12 unit tests failing now? from the commit |
@jlofti - it seems that the failing tests are not related to your PR. Qiskit team in handling it. |
Ok cool thanks! Just wanted to double check. Took care of all comments so standing by |
The CI failures should be fixed after #12979 will be merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for addressing our comments.
Summary
This PR is aims to advance #12212 by porting #12229 specifically.
Details and comments
This PR ports the
synth_cz_depth_line_mr
function to Rust. Notes:lnn.rs
file similar to howpmh.rs
is currently implemented.CircuitData
.synth_cz_depth_line_mr
lnn::_append_phase_gate
_append_cx_stage1/2
in thelnn.rs
file, but can break them out if we want them in seperate file more similar to PythonPlease let me know any changes to make and I'll take care of it
Thanks!
fixes #12229