Do not unroll gates in basis in `add_control` #13475

Cryoris · 2024-11-21T21:44:07Z

Summary

The add_control mechanism currently does some redundant unrolling and re-wrapping of gates that are already in the basis of gates we know how to control. This has knock-on effects and causes #13473, by translating an RZ operation (already supported) to a Phase gate.

This PR avoids this unrolling. As discussed with @alexanderivrii, in the longer term we should consider rewriting this logic entirely and have a controlled gate plugin, which also allows users to supply their own control mechanism.

Details and comments

There's a failing test (likely since the decomposition now uses the RX-path instead of wrongly the RX->R->U3->U path before), but the unitary is still correct and there are only dangling U(0,0,0) gates.
Need to add a test for The multi-controlled gate by RZGate().control() is less optimized. #13473

qiskit-bot · 2024-11-21T21:50:25Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core

coveralls · 2024-11-21T22:18:03Z

Pull Request Test Coverage Report for Build 12179929323

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

48 of 50 (96.0%) changed or added relevant lines in 1 file are covered.
26 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-0.02%) to 88.924%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/add_control.py	48	50	96.0%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/two_qubit_decompose.rs	1	92.13%
qiskit/circuit/add_control.py	1	96.49%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	5	92.48%
crates/qasm2/src/parse.rs	18	96.69%

Totals
Change from base Build 12131777933:	-0.02%
Covered Lines:	79347
Relevant Lines:	89230

💛 - Coveralls

qiskit/circuit/add_control.py

test/python/circuit/test_controlled_gate.py

ShellyGarion · 2024-12-05T10:51:14Z

qiskit/circuit/add_control.py

-        if operation.definition is not None and operation.definition.global_phase:
-            global_phase += operation.definition.global_phase
+
+    basis = ["p", "u", "x", "z", "rx", "ry", "rz", "cx"]


Any reason for this fixed list of gates? what about other gates like:
y / s / t / sx / cz / cp ?

for example, it seems that mcy is not as efficiently decomposed as mcx or mcz, see #13514

This was the original list in the code, this PR ensures we don't unroll unnecessarily but doesn't change what we unroll. For a bugfix PR I'd keep it this way, but we should add the cases above for main!

ShellyGarion · 2024-12-05T11:01:58Z

qiskit/circuit/add_control.py

+                    target,
+                    use_basis_gates=True,
+                )
+                circuit.mcp(phi, controls, target)


why do you use here mcp-mcry-mcp and not mcrz-mcry-mcrz (which should have less CX gates) ?

I didn't touch any of the synthesis logic in this PR, just how we unroll 🙂

We can change this in a PR to main, but MCP-MCRY-MCP is not the same as MCRZ-MCRY-MCRZ due to the phase difference, so wouldn't that implement a different unitary?

ShellyGarion

LGTM, thanks for the fix

* Do not unroll gates in basis * add tests * review comments * Add reno (cherry picked from commit 582070d)

Cryoris · 2024-12-05T14:58:33Z

@Mergifyio backport stable/1.4

mergify · 2024-12-05T14:59:01Z

backport stable/1.4

✅ Backports have been created

#13529 Do not unroll gates in basis in add_control (backport #13475) has been created for branch stable/1.4

* Do not unroll gates in basis * add tests * review comments * Add reno (cherry picked from commit 582070d)

* Do not unroll gates in basis * add tests * review comments * Add reno (cherry picked from commit 582070d) Co-authored-by: Julien Gacon <[email protected]>

Cryoris added 2 commits November 21, 2024 16:39

Do not unroll gates in basis

b38db38

add tests

6ecc101

Cryoris marked this pull request as ready for review November 21, 2024 21:50

Cryoris requested a review from a team as a code owner November 21, 2024 21:50

Cryoris linked an issue Nov 22, 2024 that may be closed by this pull request

The multi-controlled gate by RZGate().control() is less optimized. #13473

Closed

ElePT reviewed Nov 22, 2024

View reviewed changes

qiskit/circuit/add_control.py Outdated Show resolved Hide resolved

test/python/circuit/test_controlled_gate.py Outdated Show resolved Hide resolved

test/python/circuit/test_controlled_gate.py Outdated Show resolved Hide resolved

ShellyGarion reviewed Dec 2, 2024

View reviewed changes

test/python/circuit/test_controlled_gate.py Outdated Show resolved Hide resolved

test/python/circuit/test_controlled_gate.py Show resolved Hide resolved

test/python/circuit/test_controlled_gate.py Outdated Show resolved Hide resolved

mtreinish added this to the 1.3.1 milestone Dec 4, 2024

Cryoris added 2 commits December 5, 2024 09:49

Merge branch 'main' into fix-mcrz

638b367

review comments

ffbb955

ShellyGarion added the Changelog: None Do not include in changelog label Dec 5, 2024

ShellyGarion reviewed Dec 5, 2024

View reviewed changes

ShellyGarion added Changelog: Bugfix Include in the "Fixed" section of the changelog and removed Changelog: None Do not include in changelog labels Dec 5, 2024

Add reno

d96dae2

ShellyGarion approved these changes Dec 5, 2024

View reviewed changes

Cryoris added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Dec 5, 2024

Cryoris enabled auto-merge December 5, 2024 12:50

Cryoris added this pull request to the merge queue Dec 5, 2024

Merged via the queue into Qiskit:main with commit 582070d Dec 5, 2024
18 checks passed

mergify bot pushed a commit that referenced this pull request Dec 5, 2024

Do not unroll gates in basis in add_control (#13475)

795a2f3

* Do not unroll gates in basis * add tests * review comments * Add reno (cherry picked from commit 582070d)

mergify bot mentioned this pull request Dec 5, 2024

Do not unroll gates in basis in add_control (backport #13475) #13528

Merged

2 tasks

Cryoris deleted the fix-mcrz branch December 5, 2024 14:58

mergify bot pushed a commit that referenced this pull request Dec 5, 2024

Do not unroll gates in basis in add_control (#13475)

fa81b54

* Do not unroll gates in basis * add tests * review comments * Add reno (cherry picked from commit 582070d)

mergify bot mentioned this pull request Dec 5, 2024

Do not unroll gates in basis in add_control (backport #13475) #13529

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not unroll gates in basis in `add_control` #13475

Do not unroll gates in basis in `add_control` #13475

Cryoris commented Nov 21, 2024 •

edited

Loading

qiskit-bot commented Nov 21, 2024

coveralls commented Nov 21, 2024 •

edited

Loading

ShellyGarion Dec 5, 2024 •

edited

Loading

ShellyGarion Dec 5, 2024

Cryoris Dec 5, 2024

ShellyGarion Dec 5, 2024

Cryoris Dec 5, 2024 •

edited

Loading

ShellyGarion left a comment

Cryoris commented Dec 5, 2024

mergify bot commented Dec 5, 2024 •

edited

Loading

Do not unroll gates in basis in add_control #13475

Do not unroll gates in basis in add_control #13475

Conversation

Cryoris commented Nov 21, 2024 • edited Loading

Summary

Details and comments

qiskit-bot commented Nov 21, 2024

coveralls commented Nov 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12179929323

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ShellyGarion Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

ShellyGarion Dec 5, 2024

Choose a reason for hiding this comment

Cryoris Dec 5, 2024

Choose a reason for hiding this comment

ShellyGarion Dec 5, 2024

Choose a reason for hiding this comment

Cryoris Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

ShellyGarion left a comment

Choose a reason for hiding this comment

Cryoris commented Dec 5, 2024

mergify bot commented Dec 5, 2024 • edited Loading

✅ Backports have been created

Do not unroll gates in basis in `add_control` #13475

Do not unroll gates in basis in `add_control` #13475

Cryoris commented Nov 21, 2024 •

edited

Loading

coveralls commented Nov 21, 2024 •

edited

Loading

ShellyGarion Dec 5, 2024 •

edited

Loading

Cryoris Dec 5, 2024 •

edited

Loading

mergify bot commented Dec 5, 2024 •

edited

Loading