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

noncliff: define _proj subroutine for projectrand! #355

Open
wants to merge 18 commits into
base: nonclif
Choose a base branch
from

Conversation

Fe-r-oz
Copy link
Contributor

@Fe-r-oz Fe-r-oz commented Sep 8, 2024

PR implements project! for noncliff. Basic tests are added as well.

  • The code is properly formatted and commented.
  • Substantial new functionality is documented within the docs.
  • All new functionality is tested.
  • All of the automated tests on github pass.

@Fe-r-oz Fe-r-oz marked this pull request as ready for review September 27, 2024 14:56
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Sep 27, 2024

I think the PR is ready for review. I hope it's not awkward. Thank you!

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.

Thanks for starting this, it is incredibly helpful to have someone tackle it.

I left a few comments in, but I think there is a bigger thing that is currently unclear to me: in _proj₊ you access only the base tableau out of which all terms in the weighted sum are generated, but you do not access any of those terms themselves, nor are their relative weights taken into account. And it seems that you are returning a pure state, not a GeneralizedStabilizer. But one single measurement is not enough to turn a mixed state into a pure state, so there must be an issue with the current code.

src/nonclifford.jl Outdated Show resolved Hide resolved
src/nonclifford.jl Outdated Show resolved Hide resolved
test/test_nonclifford_quantumoptics.jl Outdated Show resolved Hide resolved
@@ -50,3 +50,18 @@ qo_tgate.data[2,2] = exp(im*pi/4)
end
end
end

@testset "project!" begin
Copy link
Member

Choose a reason for hiding this comment

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

These tests do not really check the projection operation results. They just run some projection operation and make sure that errors are not raised, but do not do verification.

We also should try something that is not exclusively single-qubit.

Something like this maybe:

for n in 1:4
    for repetition in 1:5
        s = random_stabilizer(n)
        p = random_pauli(n)
        gs = GeneralizedStabilizer(s)
        for i in 1:rand(1:3)
            apply!(gs, appropriately_padded_pcT) # this might need an `embed` or `tensor`
        end
        qo_state = Operator(gs)
        projectrand!(gs)
        qo_state_after_proj = Operator(gs)
        # finally, compute the projected result with QuantumOptics and compare
        qo_pauli = Operator(p)
        qo_proj1 = (Identity - qo_pauli)/2
        qo_proj2 = (Identity - qo_pauli)/2
        result1 = qo_proj1*qo_state*qo_proj1'
        result2 = qo_proj2*qo_state*qo_proj2'
        @test qo_state_after_proj ≈ result2 || qo_state_after_proj ≈ result1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for this! That is really appreciated.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Oct 1, 2024

Thanks for your pertinent observation. Indeed, something was wrong as I was only considering the tableau, not the catering X. What I overlooked was that in the base paper, Ted tells us to update the GeneralizedStabilizer state τ with the reduced χ-matrix namely χ' after performing the measurement (expect). That I did not...

This is why Ted says that 'trace Tr [τ′] = Tr [χ′] is the probability of measuring 0' when talking about the case of measuring 0. My bad...

Now, we have deterministic project! and random projectrand!.

@Fe-r-oz Fe-r-oz force-pushed the project branch 3 times, most recently from 18a3b7b to b367530 Compare October 1, 2024 16:21
@Fe-r-oz Fe-r-oz marked this pull request as ready for review October 1, 2024 17:30
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Oct 1, 2024

The PR is ready for review, Thank you!

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.

Thanks for continuing with this. It seems there are a few mistakes in the tests. Could you look into them?

Comment on lines 227 to 241
function _proj₊(sm::GeneralizedStabilizer, p::PauliOperator)
newstab, res = projectrand!(sm.stab, p)
χ′ = expect(p, sm)
n = nqubits(newstab)
newsm = GeneralizedStabilizer(newstab, DefaultDict(0.0im, (falses(n),falses(n))=>χ′))
return newsm, res
end

function _proj₋(sm::GeneralizedStabilizer, p::PauliOperator)
newstab, res = projectrand!(sm.stab, -p)
χ′ = expect(p, sm)
n = nqubits(newstab)
newsm = GeneralizedStabilizer(newstab, DefaultDict(0.0im, (falses(n),falses(n))=>χ′))
return newsm, res
end
Copy link
Member

Choose a reason for hiding this comment

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

there is a lot of code repetition here. It seems it would be simpler if on line 224 we just change the sign of p -- then we do not need separate proj+ and proj- functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with the latest commit a6c5fed

Comment on lines 236 to 237
newstab, res = projectrand!(sm.stab, -p)
χ′ = expect(p, sm)
Copy link
Member

Choose a reason for hiding this comment

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

It seems you are modifying an internal detail of sm before you are calling expect on it. Is sm still a valid object after this operation?

newstab, res = projectrand!(sm.stab, -p)
χ′ = expect(p, sm)
n = nqubits(newstab)
newsm = GeneralizedStabilizer(newstab, DefaultDict(0.0im, (falses(n),falses(n))=>χ′))
Copy link
Member

Choose a reason for hiding this comment

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

For this to be a valid object we need χ′==1, right? Is that always the case (e.g. you can use this to check for bugs)? If this is always the case (i.e. no bugs detected), then we can presumably just skip the computation of χ′.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

χ′ is the updated density matrix which is not necessarily be 1 as this is observed when we apply a non-clifford op to sm, and them find expectation value of a given pauli operator using expect as done in the doctest. In that particular case, χ′ = 0.7071067811865475 as seen in doctest whose probability is ~0.85.

test/test_nonclifford_quantumoptics.jl Outdated Show resolved Hide resolved
test/test_nonclifford_quantumoptics.jl Outdated Show resolved Hide resolved
test/test_nonclifford_quantumoptics.jl Outdated Show resolved Hide resolved
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Oct 25, 2024

Thank you for your feedback.

I've removed projectrand! from this PR, as I don’t have clarity on whether χ' is needed in projectrand!, based on your comment here: discussion link.

This PR focuses solely on implementing project!, which corresponds to Algorithm 2 in Ted's paper. Regarding project!, Ted states that we should update the GeneralizedStabilizer state τ with the reduced χ-matrix, specifically χ', after performing the measurement (expectation). That is why we take the trace (i.e., $\text{Tr} [\tau'] = \text{Tr} [χ']$) to calculate the probability of obtaining 0 or non-zero. I hope this sounds reasonable.

I’ll address projectrand! in a separate PR after giving it more thought in light of your feedback.

I attempted to use the suggested test (shown below) from this comment: discussion link, but I encountered an error when testing with Stabilizer/MixedDestabilizer. Consequently, I added a similar test in the test section that passes for both GeneralizedStabilizer and Stabilizer/MixedDestabilizer.

While the test may be basic, I would appreciate your feedback on its conceptual correctness. I hope it doesn't fall into the trap of being tautologically true. The tests pass for gs when changed to checks = [(random_stabilizer(1), random_pauli(1))] as well. It appears that using a repetition loop outside or employing more than one qubit results in errors, even when utilizing the Stabilizer instead of GeneralizedStabilizer

Thank you again for your guidance, and I look forward to your thoughts.

# gives error for Stabilizer/MixedDestabilizer
julia> @testset "project! for Stabilizer/MixedStabilizer" begin
           for n in 1:4
               for repetition in 1:5
                   s = random_stabilizer(n) # or md = MixedDestabilizer(random_stabilizer(n))
                   p = random_pauli(n)
                   apply!(s, p)
                   qo_state = Operator(s)
                   project!(s, p)[1]
                   qo_state_after_proj = Operator(s)
                   qo_pauli = Operator(p)
                   qo_proj1 = (identityoperator(qo_pauli) - qo_pauli)/2
                   qo_proj2 = (identityoperator(qo_pauli) + qo_pauli)/2
                   result1  = qo_proj1*qo_state*qo_proj1'
                   result2  = qo_proj2*qo_state*qo_proj2'
                   @test qo_state_after_proj ≈ result2 || qo_state_after_proj ≈ result1
               end
           end
       end

@Fe-r-oz Fe-r-oz marked this pull request as ready for review October 25, 2024 14:07
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.

Thanks for working on this. There are some issues on semantics in terms of what project! and projectrand! are supposed to mean and the tests seem to be missing a lot of cases.

@@ -50,3 +50,20 @@ qo_tgate.data[2,2] = exp(im*pi/4)
end
end
end

@testset "project!" begin
checks = [(S"X",P"X"),(S"Y",P"Y"),(S"Z",P"Z"),(S"-X",P"-X"),(S"-Y",P"-Y"),(S"-Z",P"-Z")]
Copy link
Member

Choose a reason for hiding this comment

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

This tests only very specific simple states. Right now I do not believe this is actually correct for more complicated states like:

  • multi-qubit states
  • non-stabilizer states

Using random_stabilizer and applying a few non-Clifford gates at random qubits would make this much more believable. It can be in a separate testset to help readability, so no need to necessarily modify what is already here.

Copy link
Member

Choose a reason for hiding this comment

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

and multiqubit pauli operators

qo_proj2 = (identityoperator(qo_pauli) + qo_pauli)/2
result1 = qo_proj1*qo_state*qo_proj1'
result2 = qo_proj2*qo_state*qo_proj2'
@test qo_state_after_proj result2 || qo_state_after_proj result1
Copy link
Member

Choose a reason for hiding this comment

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

it would also be nice to check that one of result1 or result2 is the zero matrix given that you are working stabilizer states only here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been now implemented with projectrand!

Copy link
Contributor Author

@Fe-r-oz Fe-r-oz Nov 8, 2024

Choose a reason for hiding this comment

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

The condition where result1 or result2 is zero is probabilistic, particularly when the test is repeated multiple times. This holds true even for Stabilizer and MixedDestabilizer projections, particularly when using multi-qubit random_stabilizer states and multi-qubit random_pauli operators

Comment on lines 225 to 228
Base.copy(sm::GeneralizedStabilizer) = GeneralizedStabilizer(copy(sm.stab),copy(sm.destabweights))

Base.:(==)(sm₁::GeneralizedStabilizer, sm₂::GeneralizedStabilizer) = sm₁.stab==sm₂.stab && sm₁.destabweights==sm₂.destabweights

Copy link
Member

Choose a reason for hiding this comment

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

these would be nice to add (maybe closer to where struct GeneralizedStabilizer is defined), so if this PR gets protracted feel free to add them in a separate PR (which probably will get merged faster). Just make sure to add some consistency tests for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, implemented in #419

0.707107+0.0im | + _ | + _
```
"""
function project!(sm::GeneralizedStabilizer, p::PauliOperator)
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 I understand what this project! function is meant to mean.

  • It is not a simulation of a (potentially nondeterministic) projective measurement (that would be projectrand! and it would involve some randomness in the output)
  • It is not what project! usually means in this library (a deterministic operation that tells you whether your measurement operator commutes with your stabilizers, and depending on whether it commutes it performs a variety of extra steps to tell you what the state would be after projection). I actually do not believe one can even define project! consistenly for GeneralizedStabilizer. After all, what would be the meaning of anticom and res in that case.

In short: I think this needs projectrand! and not project!. I think earlier in a related PR there was already a discussion of that semantic issue. Maybe we should even define project!(::GeneralizedStabilizer, ::Pauli) = error(...) with an explanation why projectrand! needs to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I’ve consolidated _proj+ and proj- into a single _proj, and added a detailed explanation in #416 with definition of project!(::GeneralizedStabilizer, ::Pauli) = error(...).

@Fe-r-oz Fe-r-oz changed the title noncliff: implement project! noncliff: implement projectrand! Nov 3, 2024
@Fe-r-oz Fe-r-oz changed the title noncliff: implement projectrand! noncliff: define _proj subroutine for projectrand! Nov 8, 2024
@Fe-r-oz Fe-r-oz force-pushed the project branch 3 times, most recently from 810fc91 to 9e2ef48 Compare November 8, 2024 13:05
@Fe-r-oz Fe-r-oz marked this pull request as ready for review November 8, 2024 22:28
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Nov 8, 2024

Thank you for your valuable feedback!

This PR builds upon the scaffolding introduced in #420, addresses #418, and includes a test for GeneralizedStabilizer from #424. It demonstrates multi-qubit projection using random_stabilizer states and the random_pauli operator for GeneralizedStabilizer, which also holds for Stabilizer and MixedDestabilizer. The remaining task is to add tests for non-stabilizer case, which pass probabilistically but require additional work. Overall, this PR demonstrates that GeneralizedStabilizer encompasses all the capabilities outlined in the Gottesman-Knill theorem.

Edit:

The MixedDestabilizercase from #424 has been added as a reference to demonstrate consistent multi-qubit projection results for stabilizer simulation when compared with GeneralizedStabilizer

graph TD

subgraph NittyGritty [ ]
        direction TB
        D--> D1[define_proj]
    end

subgraph Tests [ ]
    direction TB
     E[Consistency Tests] --> E1[random_stabilizer, *done* <br>random_pauli, *done* <br> random_clifford, *done* <br> non-stabilizer **remaining**]
 end

    subgraph ScaffoldingBranch [ ]
        direction TB
        B[Scaffolding] --> B1[completed]
    end
    subgraph ErrorBranch [Unrelated Errors, <br> Issue #418]
        direction TB
        C[Unrelated Errors, <br> Issue #418] --> C1[Resolve projection inconsistencies for multi-qubit Stabilizer/MixedDestabilizer]
    end
    A[projectrand!] --> C
    A[projectrand!] --> B
  A[projectrand!] --> E
    A --> D[Nitty Gritty]
   
Loading

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Nov 9, 2024

This PR is ready for review, Thank you!

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.

2 participants