-
Notifications
You must be signed in to change notification settings - Fork 11
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
trace, partial trace, and superoperators #55
Conversation
The failed tests seem to come from the recent bumped compat for TermInterface #52, although most recent docs (https://juliasymbolics.github.io/TermInterface.jl/dev/) say that implementing
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 70.46% 74.23% +3.77%
==========================================
Files 17 18 +1
Lines 650 753 +103
==========================================
+ Hits 458 559 +101
- Misses 192 194 +2 ☔ View full report in Codecov by Sentry. |
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.
great start! There are a few things I want us to discuss before considering for merging
src/QSymbolicsBase/predefined.jl
Outdated
function ptrace(x::STensorOperator, s) | ||
terms = arguments(x) | ||
sys_op = terms[s] | ||
new_terms = deleteat!(copy(terms), s) | ||
isone(length(new_terms)) ? tr(sys_op)*first(new_terms) : tr(sys_op)*STensorOperator(new_terms) | ||
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.
This is actually a bit worse than this. You can have @op A SpinBasis(1//2)^2
, i.e. the symbolic operator is already over a composite basis. The number of subsystem would be the number of subsystems in basis
, which might be different from the number of terms in the tensor product.
Also, what if flattening has not happened. E.g ptrace(A⊗(B⊗C+D⊗E), 2)
?
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, these are both fair comments. Will make changes.
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 found another small bug:
julia> @op A; @op B; @op C;
julia> tp = A⊗B⊗C
((A⊗B)⊗C)
julia> QuantumSymbolics.arguments(tp)
2-element Vector{Any}:
(A⊗B)
C
We would want tp
to show (A⊗B⊗C)
and QuantumSymbolics.arguments(tp)
to be a 3-element vector of the symbolic operators.
Weird. I wonder why this doesn't happen for multiplication of operators:
julia> mp = A*B*C
ABC
julia> QuantumSymbolics.arguments(mp)
3-element Vector{Any}:
A
B
C
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.
Also, what if flattening has not happened. E.g
ptrace(A⊗(B⊗C+D⊗E), 2)
?
A question somewhat related to this: should we have a function in the package that expands expressions? For instance, if a user for some reason wanted to convert A⊗(B⊗C+D⊗E)
into A⊗B⊗C+A⊗D⊗E
, shouldn't they have a way to do that? Maybe we could call it qexpand
, using the q
prefix the same we do for qsimplify
.
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.
yup, let's have a qexpand
for now. Maybe at some point we will just hook into the standard expand
and at that point it can be just an alias without making a breaking release
concerning the difference between mul and tensor, I think it is because prefactorscalings
were created for tensor, and it has been kinda hackishly applied to mul. We probably need to clean prefactorscalings
to work on both. I will make a separate issue for that.
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.
Wait, no that is overly complicated. Why not just have scalar=true
always. Is there a situation that benefits from scalar=false
?
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.
yes about a new file
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.
It's my understanding that prefactorscalings
was originally created for tensor product properties. For instance, |k1⟩⊗(A*|k2⟩)
would give A|k1⟩|k2⟩
.
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.
Regardless, I will clean up the scaling and flattening functions.
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.
prefactorscaling
was always meant only for scalars. The operator identity you are giving is wrong because A is defined to act on the Hilbert space of k2, not on the composite hilbert space of k1k2.
For the spellchecker issue, |
@@ -92,4 +106,4 @@ symbollabel(x::SZero) = "𝟎" | |||
basis(x::SZero) = nothing | |||
|
|||
Base.show(io::IO, x::SZero) = print(io, symbollabel(x)) | |||
Base.iszero(x::SymQObj) = isa(x, SZero) | |||
Base.iszero(x::Union{SymQObj, Symbolic{Number}, Symbolic{Complex}}) = isa(x, SZero) |
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 think the new iszero
implementations here are type piracy. Where are they needed?
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 fixes a bug mentioned in #54 as well: #54 (comment).
I marked this PR as a draft because I first want to clean up the scaling and flattening functions, and also implement a |
The downstream buildkite error is known and unrelated. |
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 did a quick review, I have not had a chance to go through everything yet. I think there are a few minor issues with basis checks.
Just FYI, QuantumInterface.jl provides some basis checking foundations and error types: https://github.com/qojulia/QuantumInterface.jl/blob/main/src/bases.jl#L140-L180 |
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.
looks good!
I left a bunch of comments related to an annoyance with QuantumInterface and created an issue to track it qojulia/QuantumInterface.jl#25
I would suggest merging this without addressing that issue, as otherwise there will be too much delay (and it is not a correctness issue).
I left a few other minor comments. Feel free to merge this when you have chance to address them.
Thanks for setting all of this up!
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 forgot to check the coverage. There are a couple of lines that would be nice to test (and one of them is related to a broken doctest)
head(x::KrausRepr) = :kraus | ||
children(x::KrausRepr) = [:kraus; x.krausops] | ||
kraus(xs::Symbolic{AbstractOperator}...) = KrausRepr(collect(xs)) | ||
basis(x::KrausRepr) = basis(first(x.krausops)) |
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.
coverage review: could you add a test for this line
Added trace and partial trace functionalities with examples and tests. Also created a symbolic superoperator
SSuperOperator
, including an option to express instances of this composite type in the Kraus representation with the functionkraus
. Future work can be done to create additional superoperator representations and conversions between each of them.In the process of developing this branch, I encountered a few bugs mentioned in #53 and #54, which I fix in this PR.