-
Notifications
You must be signed in to change notification settings - Fork 50
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
embed for single-qubit and two-qubit symbolic Clifford Operators #415
Conversation
ec1dc07
to
88c9a8d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #415 +/- ##
=======================================
Coverage 82.94% 82.95%
=======================================
Files 70 70
Lines 4557 4559 +2
=======================================
+ Hits 3780 3782 +2
Misses 777 777 ☔ View full report in Codecov by Sentry. |
ae51ac6
to
4186fb1
Compare
8e18dd4
to
af7a20a
Compare
af7a20a
to
c940f2f
Compare
The PR is ready for review. Thank you! |
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.
Thanks for looking into this. Do we need an embed for these though? They already work fine on arbitrary number of qubits, i.e. they are already implicitly embedded. You can run apply!(stab, sCNOT(1,2))
on any size stab
(if anything, an embed here would make thinks slower). You can not do that with apply!(stab, tCNOT)
so an embed is needed.
I guess adding this functionality makes sense just for completeness, but there should be a warning in the docstring that this is a bad way to do it.
Also, it seems there is a single docstring for one of the methods that includes what is supposed to be a docstring for the other method. This should be two separate docstrings (if they are method dosctrings).
Lastly, the single-qubit embed should still permit one-tuples as argument types for i
to be consistent with the rest of the embed
methods
also, new functionality like this needs tests |
Thank you for your comments.
Actually, I considered closing this PR since the embedding is already handled implicitly. While it’s worth exploring alternative approaches while looking into #151, the final outcome here doesn’t offer much added value. Instead of adding code that SLOW DOWN performance || is a BAD way to do things || might introduce redundant checks, we can simply update the documentation to clarify that the embedding is implicitly supported. This also seems like an inefficient use of your valuable review time. So, let's just close it :) |
The PR #413 revealed to us that we don't have
embed
for single-qubit dense/symbolic Clifford Operator or two-qubit dense/symbolic Clifford Operator. Thus, this PR attemps to defineembed
for single-qubit Clifford Operator, thereby addressing a chunk of #151Edit:
I think is is straightforward as we have already defined implicit
embed
asCliffordOperator(sCNOT(1, 2), 4)
for instance!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):