Implement multi-controlled U1 as gate#3883
Conversation
this gate has been implemented in Aqua before, see https://github.com/Qiskit/qiskit-aqua/blame/769ca8d/qiskit/aqua/circuits/gates/multi_control_u1_gate.py The contributors have been added as co-authors. Co-authored-by: Albert Frisch <alfr@de.ibm.com> Co-authored-by: Shaohan Hu <shaohan.hu@ibm.com> Co-authored-by: Manoel Marques <manoel@us.ibm.com>
Cryoris
requested review from
1ucian0,
ajavadia,
ewinston,
kdk and
mtreinish
as code owners
Cryoris
changed the title
- implement review changes by @AJAVADI - Some tests started failing which should not have been affected by the changes in this PR. Looking at the code, these tests should fail since they require a ancilla qubits, which are None. I'm not sure why these tests have been skipped before.
- add ctrl state to matrix computation of MCU1 - revert to not specifying noancilla mode in mcry/mct - handle test cases for MCU1 in the same fashion as MSGate/Barrier
the same logic is also used for the MCU3 gate, therefore it's exported to another function in u3.py
multi_controlled_rotation_gates will be removed in a fututre PR
Cryoris
changed the title
ajavadia
left a comment
ajavadia
left a comment
There was a problem hiding this comment.
I think this is good, just concerned about lumping CU1 and MCU1.
Also can you show what a CU1 and CCU1 gate will look like in the drawer?
They should be symmetric (i.e. changing controls/target doesn't change the gate).
| """The controlled-u1 gate.""" | ||
|
|
||
| def __init__(self, theta): | ||
| def __init__(self, theta, num_ctrl_qubits=1): |
There was a problem hiding this comment.
hm, a CU1Gate should really just mean 1 control. I think a MCU1 is a more appropriate name for more controls (similar to mcrx, mcry, etc. that we have).
Separating them gives you the flexibility to write algorithms for synthesis of multi-control gates (which are non-trivial), whereas for CU1 it's already a 2-qubit gate and is easier to decompose (requires no ancilla, routing, etc.).
I know that ultimately one is the generalization of the other, but it's more about separating out common cases. By this logic we could lump together X, CX, CCX, and MCT (which I think should be called MCX btw). Each has the same base gate, with 0, 1, 2, 3+ controls respectively. But we don't do that since X, CX and CCX are extensively studied and are common.
There was a problem hiding this comment.
Yes, agreed, also if we find different ways to synthesize the MCU1 it makes sense to separate this logic from CU1 and have different types derived from MCU1.
@ewinston Do you agree to change this?
| q_0: ────■──── | ||
| │ | ||
| . | ||
| │ | ||
| q_(n-1): ────■──── | ||
| ┌───┴───┐ | ||
| q_n: ┤ U1(λ) ├ | ||
| └───────┘ |
There was a problem hiding this comment.
Is this how this actually gets rendered currently? It should be something like:
q_0: ────■────
│
.
│ λ
q_(n-1): ────■────
.
│
q_(n): ────■────
Because in this gate changing control and target will not make a difference. I know the drawer is currently correct with one control, but maybe not for more controls. If not, then can you open an issue? (It should b e fixed for all 3 types of drawer: latex, text, mpl)
kdk
added
the
Changelog: Changed
* port mcu1 from function to gate this gate has been implemented in Aqua before, see https://github.com/Qiskit/qiskit-aqua/blame/769ca8d/qiskit/aqua/circuits/gates/multi_control_u1_gate.py The contributors have been added as co-authors. Co-authored-by: Albert Frisch <alfr@de.ibm.com> Co-authored-by: Shaohan Hu <shaohan.hu@ibm.com> Co-authored-by: Manoel Marques <manoel@us.ibm.com> * update init, remove old implementation * fix tests * fix cyclic import by full path * make `control` use MCU1 * mcu1 already importet via u1.py * implement review changes and fix tests - implement review changes by @AJAVADI - Some tests started failing which should not have been affected by the changes in this PR. Looking at the code, these tests should fail since they require a ancilla qubits, which are None. I'm not sure why these tests have been skipped before. * apply changes of the review - add ctrl state to matrix computation of MCU1 - revert to not specifying noancilla mode in mcry/mct - handle test cases for MCU1 in the same fashion as MSGate/Barrier * move MCU1 into CU1 * try to fix docstring error? * move gray code logic to external function the same logic is also used for the MCU3 gate, therefore it's exported to another function in u3.py * circumvent special case distinction for CU1Gate * keep mode 'noancilla' * move generate gray code to u3 multi_controlled_rotation_gates will be removed in a fututre PR * move determination of num free params to utils * revert to MCU1 as own class Co-authored-by: Albert Frisch <alfr@de.ibm.com> Co-authored-by: Shaohan Hu <shaohan.hu@ibm.com> Co-authored-by: Manoel Marques <manoel@us.ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
4 participants
Summary
The multi-controlled U1 gate is currently implemented as function only, not as class. This PR integrates the functionality in a class.
Details and comments
The tests for
mcu1are already being migrated in #3714, therefore this PR doesn't include any.Open questions:
CU1(theta, num_control_qubits=1)?_apply_mcu1function had a global phase argument, which was not accessible via theQuantumCircuit.mcu1method. With the global phase PR being a work in progress I kept the functionality in the class to easily add it later on. Is that fine?Edit: This is actually explained in Nielsen & Chuang.