-
Notifications
You must be signed in to change notification settings - Fork 46
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
Addressing some TODOs in non-clifford functionality along with resolving #309 #313
Conversation
Iterative Decoder Attempted Packages Used: 1. QuantumClifford 2. LDPCDecoders 3. Distances
#how to use # Create two different instances of the quantum Hamming code #code1 = QHamming(5) # r = 5 #code2 = QHamming(7) # r = 7 # Get the parity check matrices for each instance #H1 = parity_checks(code1) #H2 = parity_checks(code2)
Package to be used: Linear Algebra # Example usage #n_i = [2, 3] # Valid example #k_i = [1, 2] #d_i = [1, 1] #r_i = [1, 1] #code = HypergraphProduct(n_i, k_i, d_i, r_i) # Access and use functionalities: #println("Code block size (n):") #println(code_n(code)) #println("X parity-check matrix:") #println(parity_checks_x(code)) #println("Z parity-check matrix:") #println(parity_checks_z(code))
Create hypergraphproductcode.jl
Thanks! Here are a few more details on what I meant by "single commit = single semantic change" and how it relates to interactive rebase. Do not hesitate to ask if something is unclear! This workflow can be a bit annoying at first, but it really helps with organizing your work longterm. I can look at the changes that you have made by clicking on your single new commit: 8a0f778 I see three changes in it. They are small so it kinda makes sense for them to be in a single commit, but let's "role play" through a review: step 1
I review and say "hey could you change something about the second TODO" step 2you make that change and we have
Then we notice that there is something about the first todo and you fix that too and we end up with step 3
finally, there is something else that we notice about the second TODO and you fix that and we are left with step 4
So now if I want to review just the changes to the second TODO, I need to read through commits 1,2, and 4, which makes my work pretty inconvenient. Even worse, it is much more probable that I will miss something this way. I will not be able to think separately about each TODO, I will have to review them all together, risking missing something because I need to divine which change is related to which TODO. Of course, this particular example is incredibly contrived, the changes are small, etc, so it is actually pretty easy for me to read all three TODOs together. You really do not need to change anything here. I am just using it as an example. In this example, it would have been better if your commits looked like:
Again, this is just a contrived example, no need to actually do it here. Anyway, this would already be a (hypothetical) improvement. I can now select commits 2, 4, and 6 and look at the changes just to the second TODO. Importantly, at this point you should also run an interactive rebase
And the commits should not only be reordered during the rebase, some of them should be squashed:
Now you would have only 3 neatly separated commits, each dedicated to a separate semantic change. |
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.
Here are a few comments specifically on the code, unrelated on the previous comments on workflow.
Currently I believe the previous versions of these lines were better.
src/nonclifford.jl
Outdated
end | ||
|
||
function _dictvaltype(dict) | ||
return eltype(dict).parameters[2] # TODO there must be a cleaner way to do this |
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.
This is going backwards. The []
is syntactic sugar for getindex
. It does not make sense to go to the less readable getindex
.
The original reason for this todo is because it is ugly and dangerous to rely on non-public APIs, specifically the .parameters
access.
Looking around the Julia documentation, I see that there is valtype
which is probably the correct way to fix this:
julia> valtype(Dict(:a=>1))
Int64
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.
Yeah, the following based on valtype works
function _dictvaltype(dict)
return valtype(dict)
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.
julia> d = DefaultDict(0.0im, (falses(1),falses(1))=>1.0+0.0im)
DefaultDict{Tuple{BitVector, BitVector}, ComplexF64, ComplexF64} with 1 entry:
([0], [0]) => 1.0+0.0im
julia> valtype(d)
ComplexF64 (alias for Complex{Float64})
julia> eltype(values(d))
ComplexF64 (alias for Complex{Float64})
julia> return eltype(d).parameters[2]
ComplexF64 (alias for Complex{Float64})
src/nonclifford.jl
Outdated
for (k,v) in newdict # TODO is it safe to modify a dict while iterating over it? | ||
if abs(v) < 1e-14 # TODO parameterize this pruning parameter | ||
delete!(newdict, k) | ||
keys_to_delete = Tuple[] | ||
for (k, v) in newdict | ||
if abs(v) < 1e-14 | ||
push!(keys_to_delete, k) | ||
end | ||
end | ||
for k in keys_to_delete | ||
delete!(newdict, k) | ||
end | ||
state.destabweights = newdict | ||
state |
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 actually necessary? Is it actually dangerous to modify the dictionary keys while iterating over it?
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.
Nope. It's not dangerous.
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.
Passed the verdit too soon.
Another guy provide comprehensive reasons of why it's dangerous and suggested that filter!
is safe. 'why not use filter!(.., result_dict),
which handles the edge case of rehashing correctly for you' and 'deleting like you're doing is definitely dangerous'
@@ -179,25 +178,29 @@ function apply!(state::GeneralizedStabilizer, gate::PauliChannel) | |||
dtype = _dictvaltype(dict) | |||
tzero = zero(dtype) | |||
tone = one(dtype) | |||
newdict = typeof(dict)(tzero) # TODO jeez, this is ugly |
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.
This is worse. Previously the type of the new dictionary was neatly dependent on the type of the input dictionary, ensuring generality. Now you have hardcoded the dictionary type (and you are not guaranteed that the rest of the code would actually work with that type, e.g. if the user is not using Float64).
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.
Agreed.
I asked about whether it is dangerous to modify the dictionary keys while iterating over it. The answer is that it's not!
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.
Another guy provide comprehensive reasons (forwarded them) of why it's dangerous and suggested that filter!
is safe. 'why not use filter!(.., result_dict)
, which handles the edge case of rehashing correctly for you' and 'deleting like you're doing is definitely dangerous'
I checked this code passed the test
function apply!(state::GeneralizedStabilizer, gate::PauliChannel)
dict = state.destabweights
stab = state.stab
dtype = _dictvaltype(dict)
tzero = zero(dtype)
tone = one(dtype)
newdict = typeof(dict)(tzero)
for ((dᵢ,dⱼ), χ) in dict # the state
for ((Pₗ,Pᵣ), w) in zip(gate.paulis, gate.weights) # the channel
phaseₗ, dₗ, dₗˢᵗᵃᵇ = rowdecompose(Pₗ, stab)
phaseᵣ, dᵣ, dᵣˢᵗᵃᵇ = rowdecompose(Pᵣ, stab)
c = (dot(dₗˢᵗᵃᵇ, dᵢ) + dot(dᵣˢᵗᵃᵇ, dⱼ)) * 2
dᵢ′ = dₗ .⊻ dᵢ
dⱼ′ = dᵣ .⊻ dⱼ
χ′ = χ * w * (-tone)^c * (im)^(-phaseₗ + phaseᵣ + 4)
# Optionally insert into newdict conditionally
if abs(χ′) >= 1e-14 # TODO parameterize this pruning parameter
if haskey(newdict, (dᵢ′, dⱼ′))
newdict[(dᵢ′, dⱼ′)] += χ′
else
newdict[(dᵢ′, dⱼ′)] = χ′
end
end
end
end
# Filter newdict based on abs value
filter!(x -> abs(x[2]) >= 1e-14, newdict)
state.destabweights = newdict
state
end
Kindly please click on the code review suggestions: TODO2, TODO3[0a1c58f] to have a look at the attempt to TODO2, and TODO3 whenever convenient. I sought some advice on this question: "Is this actually necessary? Is it actually dangerous to modify the dictionary keys while iterating over it?" After discussions, it turned out that this is dangerous and So, I have used the Hope this is plausible. |
Kindly please check out a faster approach: 10.6ns to 3.5ns.
|
I used interactive rebase today by following your email (thanks for comprehensive email) that caused a lot of emails today as I was going through rebase. My apologies for that. I have also reworded the commits to they are easy to follow. Tested the package locally as well. |
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.
Hi, Feroz! Please squash the commits you have made (or clean them up so that one commit is responsible for one task).
For instances, your first commit makes changes to multiple different things that are then undone in other commits, making it difficult to understand what is actually being done:
Given that the changes here are fairly small, it is probably doable to just squash your commits. If this was a bigger set of changes, it would be best to be careful in separating the commits by topic.
Closing in favor of #316 When I tried to squash the commits, the head got diverged and somehow it ended up pushing the commits on my master branch. I tried to reset hard. My apologies for this inconvenience! Thank you very much again for your comprehensive comments and feedback.
|
This PR aims to address the TODOs that are provided in the non-clifford functionality and further resolve 309.
Sorry for inconvenience, @Krastanov, I will have to learn about interactive rebase stuff. I wanted to have a chat that I address some of the comments that you had in the form of TODOs because I was checking out the PR 259 and becoming aware of the cool work on the non-clifford. But I was not sure what to do, either ask you as a question, or comments it in the PR, so I decided to have the improvements as a comment. But, as you suggested, better to have PR.
I am going through your implementation and saw some TODOs, so I decided that implementing them will make me more familiar with the functionality to understand the changes that needs to be introduced to resolve Issue 309
Before appling improvement to TODOs:
After addressing the TODOs, please let me know are these improvements plausible.
I reran the this test to confirm that there was no error: