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

Dev/decoherence #22

Closed
wants to merge 19 commits into from
Closed

Conversation

jasonhan3
Copy link

  • Created an abstract noise operation class AbstractNoiseBellOp
  • Implemented PauliZOp for phase damping errors and MixedStateOp for amplitude damping errors

@Krastanov Krastanov self-requested a review July 21, 2024 12:58
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 76.74419% with 10 lines in your changes missing coverage. Please review.

Project coverage is 86.54%. Comparing base (cf6575d) to head (69be8ca).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/BPGates.jl 76.74% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   87.93%   86.54%   -1.39%     
==========================================
  Files           2        2              
  Lines         232      275      +43     
==========================================
+ Hits          204      238      +34     
- Misses         28       37       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

This looks pretty good! Most of my questions are on naming conventions. In a second round of review we will probably need to discuss testing a bit more (I have not read through it in detail yet).

src/BPGates.jl Outdated
@@ -260,6 +265,7 @@ const double_perm_tuple = (

"""The permutations realized by [`BellDoublePermutation`](@ref) as Clifford operations."""
const double_perm_qc = ( # TODO switch to symbolic gates
# Q: are these the stabilizer states for each qubit?
Copy link
Member

Choose a reason for hiding this comment

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

These are gates defined as Clifford tableaux from QuantumClifford.jl

src/BPGates.jl Outdated
Comment on lines 328 to 329
# does this apply both coincidence AND anti-coincidence measurements?
# i assume that it's hard coded into measure_tuple
Copy link
Member

Choose a reason for hiding this comment

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

Yes, all three types of measurements that result in preserving the A state are included here.

src/BPGates.jl Outdated
@@ -422,6 +433,7 @@ const cnot_perm = (1, 2, 11, 12, 6, 5, 16, 15, 9, 10, 3, 4, 14, 13, 8, 7)

function QuantumClifford.apply!(state::BellState, g::CNOTPerm) # TODO abstract away the permutation application as it is used by other gates too
phase = state.phases
# why can you do inbounds here?
Copy link
Member

Choose a reason for hiding this comment

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

the constructor has checked that the indices are within bounds

src/BPGates.jl Outdated
@@ -379,6 +388,7 @@ struct CNOTPerm <: BellOp
idx2::Int
function CNOTPerm(s1,s2,i1,i2)
(1 <= s1 <= 6 && 1 <= s2 <= 6) || throw(ArgumentError("The permutation index needs to be between 1 and 6."))
# no other checks on the BP indices? maybe that's the job of the other component
Copy link
Member

Choose a reason for hiding this comment

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

do you have other checks in mind?

Copy link
Author

Choose a reason for hiding this comment

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

Just that i1 and i2 (representing the indices of the Bell states to be manipulated) aren't checked to be inbounds, but actually I think this is a good design choice because you are decoupling the gate application from the specific Bell state you are applying it to.

src/BPGates.jl Outdated
@@ -409,6 +419,7 @@ const h = tHadamard
const p = tPhase
const hp = h*p
const ph = p*h
# TODO: What is this????
Copy link
Member

Choose a reason for hiding this comment

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

the 6 gates that are "good permutation gates", i.e. the ones from good_perm_tuple, i.e. GoodSingleQubitPerm, but represented as Clifford tableaux, for more general-purpose simulations. Used in the toQCcircuit conversion utility.

src/BPGates.jl Outdated
@@ -507,6 +522,7 @@ end
function QuantumClifford.apply!(state::BellState, g::PauliNoiseOp)
i = g.idx
# TODO repetition with ...NoisyReset and PauliNoise...
# ^ ?
Copy link
Member

Choose a reason for hiding this comment

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

this todo is a reminder that there is some code repetition with things implemented in QuantumClifford

"""
PauliZOp(idx, pz) causes qubit at idx `idx` to have a Z flip with probabilty `pz`.
"""
struct PauliZOp <: AbstractNoiseBellOp
Copy link
Member

Choose a reason for hiding this comment

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

I would not call this an Op as this implies it is a deterministic gate, not a noise process that happens only probabilistically.

Copy link
Author

Choose a reason for hiding this comment

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

I see. But isn't "PauliNoiseOp" in the original BPGates.jl also a probabilistic application of an X, Y, or Z gate? So I think it's in agreement with that convention?

Technically this struct is just a special case of the PauliNoiseOp but px = 0, py = 0, and pz = pz, but I wanted to create my own for testing purposes (in case PauliNoiseOp was broken for whatever reason; the original BPGates.jl does not have explicit tests for the PauliNoiseOp struct

Copy link
Author

Choose a reason for hiding this comment

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

Would calling this "PhaseDampingOp" make more sense?

Comment on lines +577 to +581
"""
MixedStateOp(idx, lambda) causes Bell state at idx `idx` to change to another
Bell state determined by `mixed_state_tuple` and `lambda`.
"""
struct MixedStateOp <: AbstractNoiseBellOp
Copy link
Member

Choose a reason for hiding this comment

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

Why is this called MixedStateOp? Semantically I am not sure I am following the naming convention.

Copy link
Author

Choose a reason for hiding this comment

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

Mainly because in the original BPGates.jl, even probabilistic applications are suffixed by "Op".

Maybe it makes more sense to say that this is specifically a noise operation, so calling this "AmplitudeDampingTwirlingOp" makes more sense?

@@ -6,6 +6,7 @@ DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae"
Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4"
JET = "c3a54625-cd67-489e-a8e7-0a5a0ff4e31b"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
Logging = "56ddb016-857b-54e1-b83d-db4d58db5568"
Copy link
Member

Choose a reason for hiding this comment

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

do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

It makes printing out error messages cleaner; it's not strictly necessary. I can remove it if you'd like to minimize the extra dependences. However it is a part of the standard library so I feel like the chances of it breaking are smaller.

@jasonhan3
Copy link
Author

From the previous time this PR was reviewed, I made a couple more updates:

  1. Removed the comments as questions that you answered in your replies above (thank you!)
  2. Added tests for PauliZOp and MixedStateOp for comparison to exact density matrix computation
  3. Refactored MixedStateOp
  4. Added AsymmDepOp (is mainly experimental; is not tested and most likely can be removed)

@Krastanov
Copy link
Member

@jasonhan3 thanks for setting this up! A simpler alternative implementation of most of these capabilities was merged in #23 so closing this one

@Krastanov Krastanov closed this Nov 5, 2024
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