Fix CS/CSdg inverse cancellation with reversed qargs#16124
Fix CS/CSdg inverse cancellation with reversed qargs#16124TSS99 wants to merge 4 commits intoQiskit:mainfrom
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
|
Thanks for the fix. However, the original issue is somewhat unclear. |
Coverage Report for CI Build 25485017011Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.04%) to 87.614%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions716 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
Expand the docstring on the existing regression test to explain the operator-precedence bug that was fixed (a && b || c vs a && (b || c)). Add a complementary test that verifies a reversed default inverse pair (cs followed by csdg on the same qubits) is still correctly cancelled — confirming the fix didn't break the happy path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thank you for the review, @ShellyGarion! Here is a concrete Python example that reproduces the incorrect behaviour described in the issue: from qiskit import QuantumCircuit
from qiskit.transpiler.passes import InverseCancellation
qc = QuantumCircuit(2)
qc.csdg(0, 1) # CS† on qubits (0, 1)
qc.cs(1, 0) # CS on qubits (1, 0) — reversed qubit order
pass_ = InverseCancellation()
result = pass_(qc)
print(result.count_ops())
# Before fix → {} (both gates incorrectly cancelled)
# After fix → {'cs': 1, 'csdg': 1} (gates correctly preserved)Root cause recap: The condition in Because I also updated the test suite in the latest commit:
All 67 tests pass. Please let me know if you need anything else! |
|
I'm not saying there's no bug in |
|
The actual bug shows up when the qubits overlap but aren't a permutation, e.g. Updated:
@jakelishman, please let me know if there's anything off or anything I should change (please don't be frustrated with my PRs). I'm new to contributing here and I know I sent too many PRs; I'm slowing down. Any pointers on how to do this properly would really help, I want to learn. |
|
Thanks @TSS99, I would personally like to see this PR brought over the finishing line. A few comments: Based on your explanation about operator precedence and the added test qc = QuantumCircuit(3)
qc.csdg(0, 1)
qc.cs(1, 2)
new_circ = InverseCancellation()(qc)I was expecting that the current version of Let's keep the PR as a bug-fix, not adding new features like canceling symmetric gates. Note that we can backport bugfixes to previous qiskit versions (such as 2.4) but we don't do this for new features. In addition, adding new functionality risks making the pass slower. Looking at the Looking at the release note, what do you mean by "inverse gate pairs in reverse order"? Remember that release notes are targeted towards users. |
|
Thanks @ShellyGarion, @jakelishman and @alexanderivrii for the careful review. You were right that my previous reproducer was not valid. I had misunderstood how I narrowed the PR to the behavior Jake pointed out around qc = QuantumCircuit(2)
qc.csdg(0, 1)
qc.cs(1, 0)
# cancelled on main
qc = QuantumCircuit(2)
qc.cs(0, 1)
qc.csdg(1, 0)
# not cancelled on mainSince I also checked I updated the title, PR description, and release note to avoid the misleading “different qubits” wording. Locally I ran:
Thanks again for the patience and the detailed pointers. Let me know if any further changes needs to be made. |
|
I don't think this quite approaches the problem correctly. We generally could have symmetric gates in which case we should check the qubits match irrespective of the order. I've opened an alternative fix in #16153 since that seemed the more efficient solution compared to explaining the setup once more. |
Fixes #15855.
Bug
The default standard-gate fast path handled
CS/CSdgcancellation inconsistently when the gates acted on the same two qubits in opposite order. SinceCSandCSdgare symmetric in their qubit order, both of these are valid inverse cancellations, butmainonly cancelled the first one:Fix
CS/CSdgpair only, also accept the same two qargs in the opposite order.Tests
test_cs_csdg_cancel_with_reversed_qargs_in_both_ordersverifies that bothCS/CSdgoperation orders cancel when the qargs are reversed.AI/LLM disclosure