-
Notifications
You must be signed in to change notification settings - Fork 45
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
adding all missing types of two qubit SWAP gates and sSQRTZZ/sInvSQRTZZ gate #336
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #336 +/- ##
==========================================
+ Coverage 82.44% 82.45% +0.01%
==========================================
Files 62 62
Lines 4186 4190 +4
==========================================
+ Hits 3451 3455 +4
Misses 735 735 ☔ View full report in Codecov by Sentry. |
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.
This looks great! Same comments as in #333
Added EDIT: (skipped to keep track of referencing to related open issues (if any)) This PR introduces the following gates w.r.t to #45: ✓ |
…SQRTZZ and sInvSQRTZZ gates
b52f506
to
fe3887d
Compare
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.
This would be a pretty nice addition, thank you! I have a few questions on where some of the definitions are coming from.
@@ -88,6 +88,19 @@ | |||
@test CliffordOperator(inv(sXCZ(n₁, n₂)), n₁) == inv(CliffordOperator(sCNOT(n₂, n₁), n₁)) | |||
@test CliffordOperator(inv(sZCrY(n₁, n₂)), n₁) == inv(CliffordOperator(sZCrY(n₁, n₂), n₁)) | |||
@test CliffordOperator(inv(sInvZCrY(n₁, n₂)), n₁) == inv(CliffordOperator(sInvZCrY(n₁, n₂), n₁)) | |||
@test CliffordOperator(inv(sCXSWAP(n₁, n₂)), n₁) == inv(CliffordOperator(sInvSWAPCX(n₁, n₂), n₁)) |
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.
Where does this come from?
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.
CXSWAP
: https://github.com/quantumlib/Stim/blob/main/doc/gates.md#CXSWAP
SWAPCX:
https://github.com/quantumlib/Stim/blob/main/doc/gates.md#SWAPCX
I used the inv(CliffordOperator(...))
to find SWAPCX
inverse. I saw that InvSWAPCX
is the same as CXSWAP
. So, I added a test for this relationship as well.
@testset "New two qubit gates consistency check" begin | ||
# see https://github.com/quantumlib/Stim/blob/main/doc/gates.md | ||
@test CliffordOperator(sSWAPCX) == C"IX XX ZZ ZI" | ||
@test CliffordOperator(sInvSWAPCX) == C"XX XI IZ ZZ" |
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.
This does not seem to be in the linked list of gates. Where is it from?
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.
SWAPCX:
https://github.com/quantumlib/Stim/blob/main/doc/gates.md#SWAPCX
I used the inv(CliffordOperator(...))
to find it's inverse.
@test CliffordOperator(sSQRTZZ) == C"YZ ZY ZI IZ" | ||
@test CliffordOperator(sInvSQRTZZ) == C"-YZ -ZY ZI IZ" |
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.
Where are these two coming from?
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.
LinearAlgebra.inv(op::sSQRTZZ) = sInvSQRTZZ(op.q1, op.q2) | ||
LinearAlgebra.inv(op::sInvSQRTZZ) = sSQRTZZ(op.q1, op.q2) |
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.
repeated question: where are these two from
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.
Please see: #336 (comment)
Co-authored-by: Stefan Krastanov <github.acc@krastanov.org>
Hopefully, my comments were satisfactory. All the gate definitions are from stim gates page. I think PR is ready for review, Thank you! |
This PR aims to add new types of
SWAP
gates.Reference: https://github.com/quantumlib/Stim/blob/main/doc/gates.md
Exact notation as stim uses has been used. There were many 2 qubit gates, so I decided to go small steps at a time.
Thank you for reviweing the PR.
f you want to submit an unfinished piece of work in order to get comments and discuss, please mark the pull request as a draft and ping the repository maintainer.
Please address only one topic or issue per pull request! Many small PRs are much easier to review and merge than one large PR.
Before merging, all changes and new functionality should be marked in the CHANGELOG file, but feel free to just leave your CHANGELOG notes in the PR description, to avoid merge conflicts with other requests modifying that file. The maintainer will add these CHANGELOG notes for you if you do so.
Before considering your pull request ready for review and merging make sure that all of the following are completed (please keep the clecklist as part of your PR):
If possible, keep your git history not too wild (rebase and squash commits, keep commits small and semantically separated) so that review is easier.