Feature/convert more controlled gates #50
Conversation
| "qiskit ControlledGate with " | ||
| + "base gate {} not implemented".format(i.base_gate) | ||
| ) | ||
| if type(i.base_gate) in set(_known_qiskit_gate.keys()): |
There was a problem hiding this comment.
| if type(i.base_gate) in set(_known_qiskit_gate.keys()): | |
| if type(i.base_gate) in _known_qiskit_gate: |
| @@ -500,8 +518,11 @@ def append_tk_command_to_qiskit( | |||
| qargs = [qregmap[q.reg_name][q.index[0]] for q in args] | |||
| if optype == OpType.CnX: | |||
| return qcirc.mcx(qargs[:-1], qargs[-1]) | |||
|
|
|||
| # special case | |||
| num_controls = len(qargs) - 1 | |||
There was a problem hiding this comment.
I think assuming that the "target" gate will always be single-qubit is a bug waiting to happen: better to set this in the individual cases below.
There was a problem hiding this comment.
Okay, do you mean like this?
There was a problem hiding this comment.
In fact I could just do away with the num_controls variable and just do
if optype == OpType.CnY:
return qcirc.append(qiskit_gates.YGate().control(len(qargs) - 1), qargs)
There was a problem hiding this comment.
Yes, that would be better I think.
| sub_circ = Circuit(n_base_qubits) | ||
| # use base gate name for the CircBox (shows in renderer) | ||
| sub_circ.name = instr.base_gate.name.capitalize() | ||
| sub_circ.add_gate(base_tket_gate, params, list(range(n_base_qubits))) |
There was a problem hiding this comment.
Do you think that adding the base_tket_gate to the list given by list(range(n_base_qubits) is correct here? I guess I have implicitly assumed that the base gate acts on the base qubits symmetrically.
i.e. sub_circ.add_gate(base_tket_gate, params, [0, 1])) is equivalent to sub_circ.add_gate(base_tket_gate, params, [1, 0 ]))
There was a problem hiding this comment.
Yes I think it is correct. The order of qubits is handled below.
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
Redo of #49 avoiding annoying auto-formatting issues.
I'm adding support for general multi-controlled gates in the qiskit converters.
There is a case where the
CircBoxunderlying theQControlBoxhas multiple qubits (e.g. when the base gate is anRZZGate) which I'm not quite sure I got right. I wouldn't mind discussing this further.(edit: resolved)
Requested a review from Alec as well as the
QControlBoxwas his suggestion.Changes
These used to be supported directly in
tk_to_qiskitbut after Conversion rebase #10 these were decomposed by the rebase prior to conversion. Adding the CnRy gate to the target gateset (_protected_tket_gates) should take care of this.Add CnY and CnZ gates to converters #11. I previously thought this would be annoying to add - turns out its easy.
Add support for general controlled gates in converters #28. I've added the additional detail of naming the
Circuitin theQControlBoxto reflect the gate that is implemented (see image) instead of using the default "Circuit" name. Let me know if you think this is a good idea. Alternatively I could just call it "U" or something.Also some suggestions...
if type(i) == ControlledGatein this codebase. Shouldn't we be usingif isinstance(i, ControlledGate)instead? Seems pylint recommends this too.qiskit_convert.py:299:17: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)_tk_gate_setseems to be unused... I assume we can remove this? ( edit: its used in another file)