-
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
Update Optimize1qGatesDecomposition to be Target aware #8917
Update Optimize1qGatesDecomposition to be Target aware #8917
Conversation
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 the following people are requested to review this:
|
7917f7e
to
c72f4c6
Compare
Pull Request Test Coverage Report for Build 3492084298
💛 - Coveralls |
I'm assigned as reviewer, but I'm not familiar with the transpiler pass. @itoko -san may be more appropriate. |
c72f4c6
to
d6ad95e
Compare
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.
Overall this LGTM, thanks for doing this. I especially like that we're factoring the error rates into how we selection a decomposition now. I have a couple of small inline questions but besides those minor details I think this should be good to merge.
qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py
Outdated
Show resolved
Hide resolved
c377053
to
7e2f461
Compare
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 updates, I think this mostly LGTM now, I just caught one potential performance issue inline (the same one in both passes). Once that's fixed I think I'll be good to approve and merge this.
qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py
Outdated
Show resolved
Hide resolved
qiskit/transpiler/passes/optimization/optimize_1q_commutation.py
Outdated
Show resolved
Hide resolved
4322296
to
f26ccdf
Compare
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 the quick update.
This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by Qiskit#8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after Qiskit#9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related Qiskit#8774
This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by Qiskit#8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after Qiskit#9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related Qiskit#8774
* Oxidize parameter calculation for OneQubitEulerDecomposer This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by #8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after #9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related #8774 * Eliminate python function for staticmethod definition This commit removes one layer of function calls for the OneQubitEulerDecomposer's staticmethods for calculating parameters. Previously, there was a python function which called an inner rust function, this eliminates one layer and attaches the rust function directly as a static method to the class definition.
* make Optimize1qGatesDecomposition target aware * choose decomopsition based on fidelity * fix test that assumed cost of u3 vs. u1 to now rely on target * always replace identity with empty * black * remove a bunch of unnecessary basis translations in tests * tests for the target path * add to preset passmanagers * fixup Optimize1qGatesSimpleCommutation to work with new Optimize1qGatesDecomposition * dont access duration and error unconditionally * black * more docs * lint * simplify test * work around not being able to filter global gates by qubit * review comments * add a test for the case of no errors specified in target: pick shortest decomp. * performance for qubit index lookup * black Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Oxidize parameter calculation for OneQubitEulerDecomposer This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by Qiskit#8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after Qiskit#9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related Qiskit#8774 * Eliminate python function for staticmethod definition This commit removes one layer of function calls for the OneQubitEulerDecomposer's staticmethods for calculating parameters. Previously, there was a python function which called an inner rust function, this eliminates one layer and attaches the rust function directly as a static method to the class definition.
Summary
Updating the Optimize1qGatesDecomposition pass to take into account Target if available. This means that it chooses the best decomposition adaptively per qubit, and tries to minimize error.
For example, it would convert a RX-RZ-RX sequence to a RZ-RX-RZ sequence over the same basis, if RZ is cheaper. Previously the cost heuristic was something simple like the length of the sequence.