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

fixed XPowGate matrix description #5946

Merged
merged 3 commits into from Nov 11, 2022
Merged

Conversation

michaelontiveros
Copy link
Contributor

Let t = 1.
Then the XPowGate matrix should be a phase factor times the Pauli X-matrix,
which means that the first coefficient should equal 0.
As written, the first coefficient is equal to a phase factor times \cos(\pi t),
which does not equal 0.

Let t = 1.
Then the XPowGate matrix should be a phase factor times the Pauli X-matrix,
which means that the first coefficient should equal 0.
As written, the first coefficient is equal to a phase factor times \cos(\pi t), 
which does not equal 0.
@google-cla
Copy link

google-cla bot commented Nov 9, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@CirqBot CirqBot added the Size: XS <10 lines changed label Nov 9, 2022
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

This is a good catch! Thank you! I have a suggestion to improve this further, please see below.

e^{i \pi t /2} \cos(\pi t) & -i e^{i \pi t /2} \sin(\pi t) \\
-i e^{i \pi t /2} \sin(\pi t) & e^{i \pi t /2} \cos(\pi t)
e^{i \pi t /2} \cos(\pi t /2) & -i e^{i \pi t /2} \sin(\pi t /2) \\
-i e^{i \pi t /2} \sin(\pi t /2) & e^{i \pi t /2} \cos(\pi t /2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The e^{i \pi t /2} factor is on every matrix element, so for readability we should pull it out and combine with e^{i \pi s t}. Could you please do this while you're at it? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gladly
I moved the e^{i \pi t /2} factors to the front, as you suggested :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you!

@viathor viathor added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 11, 2022
@viathor viathor merged commit c35ccff into quantumlib:master Nov 11, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Tells CirqBot to sync and merge this PR. (If it's running.) Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants