Skip to content
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 decomposition of C3SXGate #7230

Merged
merged 4 commits into from
Nov 9, 2021

Conversation

jakelishman
Copy link
Member

Summary

The phases used in the decomposition were inverted, so that the gate was
actually equivalent to SdgXGate().control(3) rather than the intended
SXGate().control(3). Note that both gates are valid square roots of
C3XGate(), which is likely how this went undetected, but the names and
value of ControlledGate.base_gate did not match.

The decomposition of C4XGate had a similar negative-phase error, but
the prelude cancelled with the use of the inverse C3SXGate, resulting
in a correct gate in the end. This commit updates C4XGate to work
with the new C3SXGate.

Details and comments

Fix #7209.

The phases used in the decomposition were inverted, so that the gate was
actually equivalent to `SdgXGate().control(3)` rather than the intended
`SXGate().control(3)`.  Note that both gates are valid square roots of
`C3XGate()`, which is likely how this went undetected, but the names and
value of `ControlledGate.base_gate` did not match.

The decomposition of `C4XGate` had a similar negative-phase error, but
the prelude cancelled with the use of the inverse `C3SXGate`, resulting
in a correct gate in the end.  This commit updates `C4XGate` to work
with the new `C3SXGate`.
@jakelishman jakelishman requested a review from a team as a code owner November 5, 2021 11:47
@jakelishman jakelishman added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Nov 5, 2021
@coveralls
Copy link

coveralls commented Nov 5, 2021

Pull Request Test Coverage Report for Build 1439617041

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.484%

Totals Coverage Status
Change from base Build 1439569847: 0.0%
Covered Lines: 49616
Relevant Lines: 60152

💛 - Coveralls

@Cryoris
Copy link
Contributor

Cryoris commented Nov 8, 2021

Should we add a test for this? E.g. with

Operator(SXGate().control(3)).equiv(C3SXGate())

🙂

Otherwise LGTM 👍🏻

@jakelishman
Copy link
Member Author

Whoops, not sure why I didn't write those before, thanks. Done in 1afc46a.

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix!

@mergify mergify bot merged commit 49110f9 into Qiskit:main Nov 9, 2021
@jakelishman jakelishman deleted the fix/c3sxgate-definition branch November 9, 2021 14:28
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C3SXGate inconsistent with SXGate
4 participants