-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Better decomposition for multi-controlled 1-qubit gates #13801
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13237981288Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - 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.
Thanks @ShellyGarion. The results look really good. I have left two comments inline. And you need release notes.
qiskit/circuit/add_control.py
Outdated
@@ -250,20 +250,40 @@ def apply_basic_controlled_gate(circuit, gate, controls, target): | |||
elif theta == 0 and phi == 0: | |||
circuit.mcp(lamb, controls, target) | |||
else: | |||
circuit.mcp(lamb, controls, target) | |||
circuit.mcrz(lamb, controls, target, use_basis_gates=True) |
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.
If I recall correctly, setting use_basis_gates=True
recursively calls transpile
. It's not a problem, but I am wondering whether this can be avoided.
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.
done in f0c7e31.
I also updated other places.
elif gate.name == "sxdg": | ||
circuit.h(target) | ||
circuit.mcp(3 * pi / 2, controls, target) | ||
circuit.h(target) | ||
|
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.
Are these 4 new elif
statements covered by existing tests (you can check the coverage report for that)?
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.
Since test_controlled_gate.py
already contains (too) many tests, I don't want to add extra unnecessary tests.
I've looked at the coverage reports and checked that the new cases are covered.
Moreover, I added some printouts to check that controlled h / y / sx / sxdg are checked for 2 and 3 controls.
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.
Thanks Shelly. Just a few minor suggestions for the release notes, otherwise it's good to go.
releasenotes/notes/improve-decomposition-controlled-one-qubit-unitaries-3ae333a106274b79.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/improve-decomposition-controlled-one-qubit-unitaries-3ae333a106274b79.yaml
Outdated
Show resolved
Hide resolved
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.
LGTM!
I was wondering, I have been using the same formulation for MCY (I think I may have even mentioned it before in Slack), but I get 705 for 9 controlled gate instead of 432. Could I please see the depth table for MCX? I have a feeling that's why mine is different. Although, I just upgraded my qiskit to latest version, and it gives the exact same depth as I do. from qiskit import QuantumCircuit, transpile
qc = QuantumCircuit(10)
qc.mcx(list(range(9)), 9)
qc = transpile(qc, basis_gates=['cx', 'u3'])
qc.depth()
>>> 703 which becomes 705 when sandwiched between sdg and s. OHH. Nevermind. I'm stupid, you're just counting the number of CX. |
By the way, could you kindly try this for MCH? I think this one gives better results. circuit.s(target)
circuit.h(target)
circuit.t(target)
circuit.mcx(controls, target)
circuit.tdg(target)
circuit.h(target)
circuit.sdg(target) |
I still feel we could do a bit better on MCU3. |
@ACE07-Sev - thank you very much for your comments!
for MCX the results are:
|
My pleasure. I am honored to have been of some use. I am actively working on it. The way I am going about it is by working on MCX a bit more (I think I mentioned it on Slack). If I get any improvement I'll let you know and will open a new Issue for it and reference this one there. |
For my MCX I got
I guess I have missed a small PR that has improved MCX compilation slightly? |
Summary
close #13514
Reduce the CX count of the multi-controlled 1-qubit unitary gates for the gates: H, SX, SXdg and U,
based on the efficient synthesis of MCRX, MCRY, MCRZ, MCX and MCPhase gates.
Details and comments
<style> </style>