-
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
Additional group theory functions for error correction #351
base: master
Are you sure you want to change the base?
Conversation
Hi, Isaac, The merge conflicts are occuring because the commits from the top to 39af6f0 are actually from PR #293 and these commits are being pushed again. If you submit the PR of commits starting from (add LOC function)109104e to the end, there would be no merge conflict. You should request Kenneth to fork the latest master branch of |
Thank you for your explanation and solution Feroz. @KDGoodenough when you get chance, could you fork the latest version of QuantumClifford and create a branch that I can fork as described by Feroz? I can also fork the repository myself if that would make more sense @Krastanov |
This is a bit roundabout and unnecessary. Isaac, if you do not want to bother with git, just make a new branch from the latest master branch and copy over your code, make sure it is your latest implementation and submit a new PR. If you prefer to do it the "proper way", make a new branch from the latest master branch and cherry pick ("cherrypick" is the name of a git command) the commits from this branch that you care about and submit a new PR. Another "proper way" would be to go back to a clean commit of yours and use an interactive rebase. Your git history is a bit messy so this would probably be a bit difficult. There is no need for Kenneth to make new forks of anything. If you do want to follow Feroz's suggested approach, all the branches and commits referred to in his post are public so you can do all the steps yourself. But that is just one out of many ways you can do this. Lastly, you can also simply fix the merge conflicts here without doing cherrypicks or rebases or anything like that. The merge conflicts seem reasonably small, so this solution might be easiest. |
Thank you, I resolved the conflicts here on Github. I will write a changelog when review is complete. Additionally, for the SubsystemCodeTableau datastructure, what should (de)stabilizerview and logicalx/zveiw return when their respective category is empty? For example, when the result of logical_operator_canaonicalize contains no operators that commute with all other operators, what should (de)stabilizerview return? Currently, I just have them return an n-qubit PauliOperator. |
For logical_operator_canonicalize, note there always exist one operator that commutes with everything, no matter the input --- the identity operator. For the other cases ((de)stabilizerview and logicalx/zview), one can make a similar argument, I believe. |
For the stabilizer part of the tableau (the part that commutes with the logical views), are you returning a generating set or all elements of the group? If it is a generating set, I would suggest keeping the same conventions as in
|
I think the tests are failing for reasons unrelated to my code. The Julia-1/Ubuntu tests fail when testing noisy circuits, which don't seem to overlap with my code, and the jet tests are classified as 'broken', but it doesn't seem like it involves my code based on the output. the Buildkite fails on the downstream tests on a test labeled "piracy", which also seems separate from my code. If any of these errors are related, let me know and I will fix them. If not, the code is ready for review. |
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.
I did a quick first round of review. I have a couple of minor comments on the code, but most of my comments are on documentation. The implementation looks great, and it seems it will be pretty useful for Kenneth's work. Some work still to do on making sure that other people can use this by reading the documentation (maybe Kenneth can help with some of the docs).
src/QuantumClifford.jl
Outdated
@@ -76,6 +76,7 @@ export | |||
graphstate, graphstate!, graph_gatesequence, graph_gate, | |||
# Group theory tools | |||
groupify, minimal_generating_set, pauligroup, normalizer, centralizer, contractor, delete_columns, | |||
logical_operator_canonicalize, commutavise, embed, SubsystemCodeTableau, |
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.
- Any preferences between
commutavise
andcommutivise
? Is this term used elsewhere by anyone else? Where does the name come frome? - Any other suggestions for
logical_operator_canonicalize
? The name is a bit long and I am not quite sure whether it conveys what is happening well. embed
as a name is already used for other purposes so this also would need to be renamed. There is alreadyembed
methods which embed a small unitary operator in a larger space by padding it with identities.
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.
let's use the following name for logical_operator_canoninicalize
to keep in similar to the other canonicalize methods we have:
canonicalize_noncomm
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.
let's, for now, change commutavise to commutify
to keep it in sync with simplify or lambdify or sympify
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.
let's, for now, rename embed
to matroid_parent
src/grouptableaux.jl
Outdated
A tableau representation of the logical operator canonical form of a set of Paulis. | ||
The index of the first operator that commutes with all others is tracked so that the | ||
stabilizer, destabilizer, logical X, and logical Z aspects of the tableau can be | ||
represented. |
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.
I do not think logical operator canonical form
is a known thing in the field. Could we document this a bit more, with definitions and explanations for why this is interesting and useful, maybe a few doctest examples?
Check with Kenneth on this, maybe ask him to help with the documentation for it.
src/grouptableaux.jl
Outdated
represented. | ||
""" | ||
mutable struct SubsystemCodeTableau <: AbstractStabilizer | ||
tab::Tableau |
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.
Parameterize the tableau type (same as MixedDestabilizer for instance), to ensure type information is not lost here.
src/grouptableaux.jl
Outdated
return SubsystemCodeTableau(tab, index, length(s), m, (index-1)/2) | ||
|
||
end |
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.
return SubsystemCodeTableau(tab, index, length(s), m, (index-1)/2) | |
end | |
return SubsystemCodeTableau(tab, index, length(s), m, (index-1)/2) | |
end |
src/grouptableaux.jl
Outdated
|
||
end | ||
|
||
Base.length(t::SubsystemCodeTableau) = length(t.tab) |
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.
Is this used somewhere? Given the complicated internal structure, I am reluctant to define a "homogenous" length function as I am not sure what it would mean semantically. If it is not used, let's delete it.
src/grouptableaux.jl
Outdated
com, d1= QuantumClifford.commutavise(t) | ||
norm = QuantumClifford.normalizer(com.tab) | ||
state, d2 = QuantumClifford.commutavise(norm) |
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.
com, d1= QuantumClifford.commutavise(t) | |
norm = QuantumClifford.normalizer(com.tab) | |
state, d2 = QuantumClifford.commutavise(norm) | |
com, d1= commutavise(t) | |
norm = normalizer(com.tab) | |
state, d2 = commutavise(norm) |
src/grouptableaux.jl
Outdated
""" | ||
Return the full Pauli group of a given length. Phases are ignored by default, | ||
but can be included by setting `phases=true`. | ||
but can be included by setting `phases = true`. |
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.
but can be included by setting `phases = true`. | |
but can be included by setting `phases=true`. |
src/grouptableaux.jl
Outdated
end | ||
end | ||
|
||
return Tableau(normalizer) | ||
return QuantumClifford.Tableau(norm) |
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.
return QuantumClifford.Tableau(norm) | |
return Tableau(norm) |
@@ -16,34 +16,75 @@ | |||
s_test = copy(s) |
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.
@KDGoodenough , could you check that this file makes sense (the test file)?
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.
T_str
is already public, and Tableau
is not needed to be public for this code. I can still make it public if you want.
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.
oh, no need to make Tableau public then, but in various examples switch to @T_str
instead of the more complicated construction of Tableau that you are using.
Co-authored-by: Stefan Krastanov <github.acc@krastanov.org>
@IsaacP1234, could you add a reference of this paper to what we now call logical_operator_canonicalize? https://scholar.google.com/scholar?q=https://arxiv.org/pdf/1302.3428&hl=en |
And a reference to this paper and Fig. 1: https://arxiv.org/pdf/2406.02427 for the delete, contractor, and embed function please! |
@KDGoodenough I have rewritten the documentation for |
…mClifford.jl into loc-comm-embed fixed merge conflict with origin
I think I have completed all the requested changes, but I am not aware of more uses of Additionally, should I add the newly-referenced papers to references.md? |
@IsaacP1234 I think it's good like this! There's not too much work (yet!) that uses this commutify business. My guess would be that yes, it would be good to add these references to the .md file as well, but Stefan would know better. |
Changelog:
|
Adding logical_operator_canonicalize, commutavise, and embed functions, as well as SubsystemCodeTableau datastructure and minor performance improvements for groupify and normalizer.