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

Additional group theory functions for error correction #351

Open
wants to merge 74 commits into
base: master
Choose a base branch
from

Conversation

IsaacP1234
Copy link
Contributor

Adding logical_operator_canonicalize, commutavise, and embed functions, as well as SubsystemCodeTableau datastructure and minor performance improvements for groupify and normalizer.

@Fe-r-oz
Copy link
Contributor

Fe-r-oz commented Sep 4, 2024

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 QC.jl, then make a new branch, say newgrouptab. Then, you can fork newgrouptab, and do commits and push PR. It seems here you have forked the grouptableaux branch of Kenneth whose first commit is part of PR 293 (grouptableaux). Then you made loc-comm-embed by making a new branch based on grouptableaux and the pushing your commits to loc-comm-embed . But the first branch (grouptableaux) does not coincide with the master that has grouptableaux.jl and it's needs to be updated, and hence all the commits have reappeared here.

@IsaacP1234
Copy link
Contributor Author

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

@Krastanov
Copy link
Member

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.

@IsaacP1234
Copy link
Contributor Author

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.

@KDGoodenough
Copy link
Contributor

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.

@Krastanov
Copy link
Member

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 MixedDestabilizer and just return a tableaux with zero rows:

julia> s = MixedDestabilizer(S"XX ZZ") # A full rank tableau
𝒟ℯ𝓈𝓉𝒶𝒷
+ Z_
+ _X
𝒮𝓉𝒶𝒷
+ XX
+ ZZ

julia> logicalxview(s)


julia> logicalxview(s) |> typeof
Stabilizer{QuantumClifford.Tableau{SubArray{UInt8, 1, Vector{UInt8}, Tuple{UnitRange{Int64}}, true}, SubArray{UInt64, 2, Matrix{UInt64}, Tuple{Base.Slice{Base.OneTo{Int64}}, UnitRange{Int64}}, true}}}

julia> length(logicalxview(s))
0

@IsaacP1234
Copy link
Contributor Author

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.

Copy link
Member

@Krastanov Krastanov left a 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).

@@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

@KDGoodenough , @IsaacP1234

  • Any preferences between commutavise and commutivise? 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 already embed methods which embed a small unitary operator in a larger space by padding it with identities.

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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

Comment on lines 5 to 8
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.
Copy link
Member

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.

represented.
"""
mutable struct SubsystemCodeTableau <: AbstractStabilizer
tab::Tableau
Copy link
Member

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.

Comment on lines 64 to 66
return SubsystemCodeTableau(tab, index, length(s), m, (index-1)/2)

end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return SubsystemCodeTableau(tab, index, length(s), m, (index-1)/2)
end
return SubsystemCodeTableau(tab, index, length(s), m, (index-1)/2)
end


end

Base.length(t::SubsystemCodeTableau) = length(t.tab)
Copy link
Member

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.

Comment on lines 277 to 279
com, d1= QuantumClifford.commutavise(t)
norm = QuantumClifford.normalizer(com.tab)
state, d2 = QuantumClifford.commutavise(norm)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)

"""
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`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
but can be included by setting `phases = true`.
but can be included by setting `phases=true`.

end
end

return Tableau(normalizer)
return QuantumClifford.Tableau(norm)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return QuantumClifford.Tableau(norm)
return Tableau(norm)

@@ -16,34 +16,75 @@
s_test = copy(s)
Copy link
Member

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)?

Copy link
Contributor Author

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.

Copy link
Member

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.

src/grouptableaux.jl Outdated Show resolved Hide resolved
Co-authored-by: Stefan Krastanov <github.acc@krastanov.org>
@KDGoodenough
Copy link
Contributor

@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

@KDGoodenough
Copy link
Contributor

KDGoodenough commented Sep 10, 2024

And a reference to this paper and Fig. 1: https://arxiv.org/pdf/2406.02427 for the delete, contractor, and embed function please!

@IsaacP1234
Copy link
Contributor Author

@KDGoodenough I have rewritten the documentation for canonicalize noncomm, commutify, and matroid_parent to be more clear, but is there anything about the functions or the theory behind them that I should include in the documentation?

@IsaacP1234
Copy link
Contributor Author

I think I have completed all the requested changes, but I am not aware of more uses of commutify aside from matroid parent to add to its documentation. @KDGoodenough are there any I should add?

Additionally, should I add the newly-referenced papers to references.md?

@KDGoodenough
Copy link
Contributor

@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.

@IsaacP1234
Copy link
Contributor Author

Changelog:

  • New error correction tools:
    • canonicalize_noncomm function to find a generating set with minimal anticommutivity
    • SubsystemCodeTableau data structure to represent the output of canonicalize_noncomm
    • commutify function to find a commutative version of a non-commutative set of Paulis with
      minimal changes
    • matroid_parent function to, for set of Paulis that doesn't represent a state, find a version
      that does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants