-
Notifications
You must be signed in to change notification settings - Fork 34
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
Create ChoiState type and conversions to/from SuperOperator #115
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #115 +/- ##
==========================================
+ Coverage 93.00% 93.06% +0.05%
==========================================
Files 25 26 +1
Lines 3104 3257 +153
==========================================
+ Hits 2887 3031 +144
- Misses 217 226 +9 ☔ View full report in Codecov by Sentry. |
@akirakyle , I think here it would be a great place to use the multimethod style of julia. With the current setup you are proposing, the following will be a bit difficult: applying the choi superoperator to some operator. My initial suggestion would be to create
Then you can easily have the same function (e.g. multiplication or Do not hesitate to voice dissent if this seems misguided. Let me know what you think. |
@Krastanov I think it might make sense to keep this in draft status for awhile while I get some experience using the choi representation in my own code to see what feels most natural. I think you might be right in that the safest method would to introduce a |
@Krastanov I am finally getting back to working on this and I noticed that the I will be needing the Kraus representation of superoperators as well so I think I will go ahead and implement a |
I'm also noticing that the constructors for |
7e848f4
to
81a9159
Compare
Hi, Akira! Pardon the slow response, I did not notice that was converted away from draft status. I will try to review this in the next few days. |
Hi, Akira! I guess a week turned into a month, sorry about that. I did some very minor touchup, but feel free to disregard the last commit (by doing a force push here). Otherwise, do not forget to do a pull first to incorporate it. Given other things you have mentioned in this thread, does the following todo list sound reasonable as prerequisite for merging (of course, there is no expectation that you have to do this -- it is already appreciated that you have donated some of your time to start this):
I will add this to the PR description and convert it to a draft for now (that helps me with organizing my own PR-review work). Do not hesitate to convert this back to non-draft and request a review. |
That looks like a solid todo list to me and represents what I would like to see with regards to getting all the common superoperator representations implemented with a reasonable interface. I've been consumed by another research project which is why I haven't been spending time on this lately but I do intended to come back to this at some point, hopefully soon. |
45e5850
to
c12a150
Compare
@Krastanov I've finally been coming back to this PR as I've picked up the research project that relies on it again! I've implemented the The conversion from PS: I am currently in Boston until this Sunday, then stonehill until next friday, so if you are around and would like to meet up in person to discuss this or anything else julia/quantum related, let me know when works! |
2ab7770
to
1154c2a
Compare
@Krastanov I think this should finally be ready for review! I've split off implementing Kraus operators into a separate PR #177 to make it easier for both myself and for review. |
Thanks! I will try to look into it later this week. Please ping me if I have not done so by next week |
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 looks great, thank you so much for the contribution! Apologies for the slow review. I have a few very minor suggestions, please check them out.
If we keep this non-public for a while, I am happy to merge this ASAP.
src/superoperators.jl
Outdated
new(basis_l, basis_r, data) | ||
end | ||
end | ||
ChoiState{BL,BR}(b1::BL, b2::BR, data::T) where {BL,BR,T} = ChoiState{BL,BR,T}(b1, b2, data) |
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.
just to make sure everything is really fine, could you add a test for this constructor 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.
Ah this is left over from my copy-paste of the SuperOperator
constructors. I don't actually use this so I just went ahead and deleted it. It can easily be added back if it's needed at any point in the future.
@@ -228,4 +247,20 @@ N = exp(log(2) * sparse(L)) # 50% loss channel | |||
@test (0.5 - real(tr(ρ^2))) < 1e-10 # one photon state becomes maximally mixed | |||
@test tracedistance(ρ, normalize(dm(fockstate(b, 0)) + dm(fockstate(b, 1)))) < 1e-10 | |||
|
|||
# Testing 0-2-4 binomial code encoder |
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 a really neat test!
src/QuantumOpticsBase.jl
Outdated
@@ -36,8 +36,8 @@ export Basis, GenericBasis, CompositeBasis, basis, | |||
current_time, time_shift, time_stretch, time_restrict, static_operator, | |||
#superoperators | |||
SuperOperator, DenseSuperOperator, DenseSuperOpType, | |||
SparseSuperOperator, SparseSuperOpType, spre, spost, sprepost, liouvillian, | |||
identitysuperoperator, | |||
SparseSuperOperator, SparseSuperOpType, ChoiState, |
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 keep it unexported until more of the related functionality is completed and it has been tested a bit more. I want to avoid a breaking release.
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.
Sounds good to me, especially since #177 may rework things a bit!
end | ||
|
||
ChoiState(op::SuperOperator) = ChoiState(_super_choi(op.basis_l, op.basis_r, op.data)...) | ||
SuperOperator(op::ChoiState) = SuperOperator(_super_choi(op.basis_l, op.basis_r, op.data)...) |
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.
does it make sense to also add a Ket(::ChoiState)
constructor, so that we can actually extract it as a 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.
I don't think it makes sense since in general the ChoiState is mixed iff the corresponding channel is non-unitary, so it is akin to asking for a Ket(::Operator) method which has multiple ways in which it could make sense, e.g. closest pure state under a given operator norms.
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.
Ah, noted. Then maybe a dm(::ChoiState)
would make sense, but no need to block the PR on 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.
I have something like that added as part of #177. The tricky part is ensuring the input and ancilla bases make sense and can be appropriately manipulated!
@Krastanov I just addressed your comments! P.S. If you get the chance to give some feedback on my proposal in qojulia/QuantumInterface.jl#26 that would help me to organize my work on this going forward! |
This is ready to merge, but it seems the release of 1.11 in the meantime has broken a bunch of allocation tests. @amilsted , is this on your radar, it seems to be related to the time-dependent operators. I will make a separate issue for that. This PR is not at fault. @akirakyle , could you please mark the those tests as broken? Afterwards we can go ahead and merge this. If you do not have the bandwidth, that is fine, one of us should get around to it in the next week anyway. |
@Krastanov I marked the failing test as broken and so now all tests are passing on 1.11 |
Unlike qutip, which uses
type = super, superrep = choi
to mark aqobj
as a superoperator in the choi representation, I personally think it makes more sense to just reuse the normalOperator
type to represent a choi matrix since it must be a valid density operator after all. Also it often makes sense to compute things like the Von Neumann entropy of the choi state. To convert back to the superoperator representation, one needs to assume some convention for which of the two Hilbert spaces is the auxiliary/reference/environment system and which represents the channel's output system. I've chosen the convention which seems standard to have the first system be the reference and the second be the output system.I still need to add docs and tests but I thought I'd open this now as a draft so that there can be some discussion of these choices. Also this might be a good time to follow through on the TODO to upstream
_permutedims
and maybe figure out how to make it operate onReshapedSparseArray
?Rough TODO list: