-
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
Multicontrol Rz fix #9574
Multicontrol Rz fix #9574
Conversation
…e functionality added
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: |
@javabster @jakelishman @1ucian0 - Thanks for your help during the qiskit office hours. It turns out the code still fails some unit tests even without the mcx functionality. |
@alexanderivrii, @Cryoris (and @ShellyGarion, I think?): please could you two (three if I'm right about Shelly too) have a look at this? Alexis came to the office hours earlier today and we talked about what he'd been working on, and in principle it all looks like something we're very interested in to me. Sasha: for you, my questions are mostly about how you think it's best to organise the code in the short-term with other synthesis efforts going on. I suspect that it's mostly ok to mirror the current style, since our hope is to more completely overhaul MCX/MCU/etc synthesis later. Julien and Shelly: I think Julien you did the original (or most recent) implementations of the MC synthesis routines, and Shelly you're definitely more up-to-date on the literature and other methods than I am. |
Thanks @jakelishman - to reproduce the McX code run the following:
This outputs the following (counts number of CNOT gates):
i.e. the diagonal approach should be used up to 5 controls and then the new decomposition after 6 controls |
|
||
|
||
QuantumCircuit.mcrx = mcrx | ||
QuantumCircuit.mcry = mcry | ||
QuantumCircuit.mcrz = mcrz | ||
|
||
|
||
class ControlRotationGate(ControlledGate): |
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.
Hi @AlexisRalli, I think that ucry, ucrx and ucrz in this folder already implements Theorem 8 of https://arxiv.org/pdf/quant-ph/0406176.pdf.
if n_c == 1: # cu | ||
_apply_cu(self, 0, 0, lam, control_qubits[0], target_qubit, use_basis_gates=use_basis_gates) | ||
if n_c <= 6: | ||
ncrz = ControlRotationGate(lam, n_c, axis="z") |
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.
You can probably use 'QuantumCircuit.ucrz' with several id gates and one rz. Or maybe adapt ControlRotationGate to use ucrz. Nice observation that with a small number of qubits the multiplexer is a good approach.
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 think that mcrz should be in another PR, because the mcrz issue.
use_basis_gates=use_basis_gates, | ||
) | ||
if n_c <= 6: | ||
ncrx = ControlRotationGate(theta, n_c, axis="x") |
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.
Is it possible to use ucrx
here?
rygate = ( | ||
numpy.cos(theta / 2) * numpy.eye(2) - 1j * numpy.sin(theta / 2) * YGate().__array__() | ||
) | ||
ncry = MCU2Gate(rygate, n_c, label=f"Ry({theta:0.3f})") |
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.
Is it possible to use ucry here?
from qiskit.extensions.quantum_initializer.squ import SingleQubitUnitary | ||
|
||
|
||
class MCU2Gate(ControlledGate): |
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.
My suggestion is to create a different PR with the MCU2Gate because this class is for "any multicontrol version of any single qubit unitary matrix (gate)" and this PR is about multicontrolled rotation gates.
Hi @AlexisRalli. I'm not on the qiskit team, but I left some suggestions. The main one is that this PR could be separeted in two. One for the MCU2Gate class and another for multicontrolled rotations with few control bits. |
can we close this PR as #9836 has been merged? |
Summary
I've currently written code to decompose multi control rotation gates properly (see issue 9202 ):
QuantumCircuit.mcrz
,QuantumCircuit.mcrx
,QuantumCircuit.mcry
The update includes a new file in generalized_gates that for any given single qubit unitary matrix will generate a multicontrol quantum circuit of it using O(n^2) two qubit gates.
Details and comments
Currently there are problems with the unit tests. I think this could stem from the following:
i.e. there is a relative phase in the gate decomposition that should NOT be there. I think the unit tests take this into account, when they should not. However, by correcting the codebase these tests then fail.
Note the updated code for
QuantumCircuit.mcrz
builds the correct quantum circuits:How should I proceed?
Thanks for the help!
(side note) The
MCU2Gate
class allows arbitrary control single qubit gates to be built as follows: