-
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
add Quantum Shannon Decomposition #7907
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:
|
Pull Request Test Coverage Report for Build 2223540529
💛 - 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.
Overall this LGTM, I'll have to read the paper again to check the algorithm against this. But other than that the only other thing I'm wondering is around our plugin structure now. Specifically, I'm wondering if we should just make isometry a separate plugin now move it out of the default plugin. Or if we want to make this a standalone plugin with min_qubits=3
, but I'm guess the performance of this method is good enough that it just makes sense to leave it in the default.
nqubits = int(np.log2(dim)) | ||
if opt_a2: | ||
raise NotImplementedError("Optimization A.2 is not currently implemented.") | ||
if np.allclose(np.identity(dim), mat): |
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.
Do you think there is value in having an option to tweak the tolerances here (and anywhere we're doing float comparisons)?
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.
Maybe, but I'm not sure how to do that sensibly at the moment.
if dim == 2: | ||
if decomposer_1q is None: | ||
decomposer_1q = one_qubit_decompose.OneQubitEulerDecomposer() | ||
circ = decomposer_1q(mat) | ||
elif dim == 4: | ||
if decomposer_2q is None: | ||
decomposer_2q = two_qubit_decompose.TwoQubitBasisDecomposer(CXGate()) | ||
circ = decomposer_2q(mat) | ||
else: |
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.
I'm wondering if it makes more sense here to just raise if we expect people to use other decomposition techniques for 1 and 2 qubit matricies. For example, if this were a separate unitary synthesis plugin we'd just set min qubits to 3 and it would fallback to the default plugin for smaller matrices.
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.
Actually this decomposition method will go down to single qubit unitaries but it's just that the known 3-cnot decomposition will yield lower cnot counts. If we let it go down to single qubits, this would correspond to the "QSD, l=1" column of the table above. I wouldn't raise, however; since you'd raise every time you tried to do this decomposition due to the recursion. I could see, however allowing it to use this the QSD all the way down to 1q but I wasn't sure how best to implement that or if anyone would actually prefer the higher CX cost.
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.
Oh, the other thing I just noticed this could use a release note. Probably a feature
note about the new function and a upgrade note about the change in the behavior of the unitary synthesis pass. (this is where having a dedicated isometry synthesis plugin would be useful because we could document calling transpile(unitary_synthesis_method='isometry")
as a way to preserve the previous decompositions for >2q unitaries).
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.
Basically, it looks good. I learned a lot from this PR.
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.
This looks good to me, thanks for implementing it. There are some potential duplicates of some inner bits and pieces, mainly around uniformly-controlled (multiplexed) gates with those existing under qiskit/extensions/quantum_initializer/
(which isometry is built on). It would be good to unify these.
Co-authored-by: Ali Javadi-Abhari <[email protected]>
Co-authored-by: Ali Javadi-Abhari <[email protected]>
@ajavadia The duplication was an attempt to make the code more efficient as suggested by Matthew (#7907 (comment)). Another alternative would be to let the code in UCPauliRotGate take keyword options to replace CX with CZ and remove the final CZ but that seemed somewhat specific to this decomposition method. |
Summary
Currently, general unitary decomposition yields CX gate counts which are about double one of the best currently known algorithms, the Quantum Shannon Decomposition, which is itself within a factor of two from the theoretical lower bound. This PR adds this decomposition as described in arXiv:quant-ph/0406176.
Details and comments
In this implementation recursive decomposition proceeds down to two qubit operators which it hands off to the
TwoQubitBasisDecomposer
.Only, one of the two optimizations mentioned in the paper is currently implemented. When option
a1_opt
is set, the default, the CX gate count is9/16 * 4^(n) - 3/2 * 2^n - 1/3 * 4^(n-1) - 1
The table shows a comparison of CX gates counts before and after this PR along with the methods described in the paper,