-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix: Allow circuits containing CnZ gates to run on AerBackend
#369
Conversation
Fixed the bug for #368
I change the gate name with `mcz` for CnZ and edit test functions. https://github.com/CQCL/pytket-qiskit/blob/49816251a059db704a40a50e61e00742fe85c361/pytket/extensions/qiskit/qiskit_convert.py#L706
@@ -702,7 +702,9 @@ def append_tk_command_to_qiskit( | |||
if optype == OpType.CnY: | |||
return qcirc.append(qiskit_gates.YGate().control(len(qargs) - 1), qargs) | |||
if optype == OpType.CnZ: | |||
return qcirc.append(qiskit_gates.ZGate().control(len(qargs) - 1), qargs) | |||
new_gate = qiskit_gates.ZGate().control(len(qargs) - 1) | |||
new_gate.name = "mcz" |
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.
Can you freely choose this name? Could we do cnz
?
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.
Unfortunately 'cnz' doesn't work because AerSimulater accepts the name 'mcz'.
In general, AerSimulater accepts the gete list on blow
#368 (comment)
Maybe the changelog should be updated as well for this fix? https://github.com/CQCL/pytket-qiskit/blob/main/docs/changelog.rst |
docs/changelog.rst
Outdated
@@ -13,6 +13,7 @@ Unreleased | |||
* Add conversion of controlled unitary gates from qiskit to tket. | |||
* Initialize `TketAutoPass` with a `BackendV2`. | |||
* Update `TketBackend` to derive from `BackendV2`. | |||
* Fix to allow `AerBackend` to work with multi-controlled Y and Z gates. |
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 don't think the handling of multi-controlled-Y gates has been changed in this PR. I think its just multi-controlled-Z gates that are given a different name.
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.
Yes you're right.
tests/backend_test.py
Outdated
compilation_pass = b.default_compilation_pass(i) | ||
compilation_pass.apply(circ) | ||
unitary_after = circ.get_unitary() | ||
assert compare_unitaries(unitary_before, unitary_after) |
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 unitary comparision needed in this test? I guess we just want to verify that the circuit containing the CnZ is runable on AerBackend
. Not sure what the purpose of checking that the unitary is the same before/after compilation is for. Guess it does no harm.
I don't think the handling of the other multi-controlled gates has changed at all
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.
True, it would be more useful to have a test that the circuit ran correctly, to confirm that the conversion was correct. I'll change the test.
AerBackend
AerBackend
AerBackend
Changed the PR title for clarity |
Description
Replace CnY and CnZ in qiskit gates
519a512?diff=split&w=1
Make a test function
9412856?diff=split&w=0
Related issues
#368
Checklist