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

Add identitysuperoperator and dagger for SuperOperator #116

Merged
merged 5 commits into from
Jul 21, 2023

Conversation

akirakyle
Copy link
Contributor

I'm not sure that I've implemented these in the right way as I'm still getting the hang of Julia's type system. So I'm opening this as a draft PR with the hope that maybe I can get some feedback on how I should actually implement these, as I am needing them for my research. My guess is that maybe superoperators need to have types similar to DenseOpAdjType and SparseOpAdjType implemented so that adjoints can be done lazily?

@Krastanov
Copy link
Collaborator

@akirakyle , thank you for the numerous submissions you have made over the last two days! It will not be immediate, but over the next 7 days we should be able to provide feedback on your contributions and help getting them merged! Your work on all of these is very much appreciated!

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #116 (f98f96d) into master (d04256d) will decrease coverage by 0.06%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   93.64%   93.59%   -0.06%     
==========================================
  Files          25       25              
  Lines        3084     3089       +5     
==========================================
+ Hits         2888     2891       +3     
- Misses        196      198       +2     
Impacted Files Coverage Δ
src/QuantumOpticsBase.jl 100.00% <ø> (ø)
src/superoperators.jl 81.51% <60.00%> (-0.95%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Krastanov
Copy link
Collaborator

@amilsted , as I was reviewing this, I noticed the following in the behavior of identityoperator which surprised me:

julia> b = FockBasis(10)
Fock(cutoff=10)

julia> identityoperator(b) |> typeof
Operator{FockBasis{Int64}, FockBasis{Int64}, LinearAlgebra.Diagonal{ComplexF64, FillArrays.Ones{ComplexF64, 1, Tuple{Base.OneTo{Int64}}}}}

julia> identityoperator(create(b)) |> typeof
Operator{FockBasis{Int64}, FockBasis{Int64}, SparseArrays.SparseMatrixCSC{ComplexF64, Int64}}

julia> identityoperator(dense(create(b))) |> typeof
Operator{FockBasis{Int64}, FockBasis{Int64}, Matrix{ComplexF64}}

Could you confirm that it is on purpose that identityoperator(op::OperatorType) returns an operator of type OperatorType? I can see arguments for that and I can see arguments for not preserving the operator type and rather always returning Eye data, so I wanted to make sure.

@Krastanov
Copy link
Collaborator

Krastanov commented Jul 5, 2023

@akirakyle , this looks great! Could you make the following two additions?

  • a identitysuperoperator(b::Basis) method that uses Eye similarly to the square identityoperator - the Eye object has a lot of extra niceties available to it than a typical sparse operator
  • add a docstring to identitysuperoperator
  • a few tests of the functionality you have created along the lines of:
b = GenericBasis(2)⊗GenericBasis(3)
i1 = identitysuperoperator(b)
i2 = identitysuperoperator(sparse(spre(create(b))))
i3 = identitysuperoperator(dense(spre(create(b))))
@test i1*state == i2*state == i3*state == state == dagger(i1)(state) == ...

Lastly, we should probably declare a method-less identitysuperoperator in QuantumInterface and import and extend it here. We use QuantumInterface to declare functions that other libraries might want to extend without depending on QuantumOptics, e.g. QuantumClifford and QuantumSymbolics -- if that seems too convoluted to deal with right now, please just go ahead with the rest of the changes and I will take care of QuantumInterface.

Thank you for the valuable work you are contributing!

@amilsted
Copy link
Collaborator

amilsted commented Jul 5, 2023

Could you confirm that it is on purpose that identityoperator(op::OperatorType) returns an operator of type OperatorType? I can see arguments for that and I can see arguments for not preserving the operator type and rather always returning Eye data, so I wanted to make sure.

Yeah, this is how it has worked for a long time (since before I became involved, anyway!). I suppose it works analogously to one() on AbstractArray subtypes. Indeed, one() on operator types calls identityoperator. I agree this is not obviously the best outcome in all circumstances.

@akirakyle
Copy link
Contributor Author

identitysuperoperator(b::Basis) method that uses Eye similarly to the square identityoperator - the Eye object has a lot of extra niceties available to it than a typical sparse operator

Lastly, we should probably declare a method-less identitysuperoperator in QuantumInterface and import and extend it here.

Just so I understand correctly before working on this, should I create a identitysuperoperator.jl file in QuantumInterface which mirrors the set of functions in identityoperator.jl then implement identitysuperoperator here in a similar way which does method dispatch based on a ::Type{T} argument called from QuantumInterface?

@Krastanov
Copy link
Collaborator

Just so I understand correctly before working on this, should I create a identitysuperoperator.jl file in QuantumInterface which mirrors the set of functions in identityoperator.jl then implement identitysuperoperator here in a similar way which does method dispatch based on a ::Type{T} argument called from QuantumInterface?

I would suggest something much simpler as a start. Just declare a function identitysuperoperator end without any method definitions in QuantumInterface and import it here. That way QuantumClifford also can use identitysuperoperator for its own types without depending on QuantumOpticsBase. Down the line we might want something more sophisticated, but I prefer we do the simpler fast thing first.

@Krastanov
Copy link
Collaborator

An identitysuperoperator was added to QuantumInterface 0.3.1 so now we should be able to just import it from there.

QuantumInterface 0.3.1 registration: JuliaRegistries/General#87597

@Krastanov
Copy link
Collaborator

@akirakyle , I made a small change to use the identitysuperoperator from the QuantumInterface namespace, so that this can also be cohesively used with QuantumClifford and company. If you have the bandwidth, could you tackle the two outstanding requests:

  • identitysuperoperator(::Basis) that internally uses the sparse Eye type
  • Some test of functionality, to cover all the new additions

@akirakyle
Copy link
Contributor Author

Thanks for adding identitysuperoperator to QuantumInterface. I've now added a identitysuperoperator(::Basis) method and the simplest of tests.

@akirakyle akirakyle marked this pull request as ready for review July 18, 2023 20:22
@akirakyle
Copy link
Contributor Author

Just realized I forgot to add tests for dagger, will do here shortly...

@akirakyle
Copy link
Contributor Author

Simple tests added for dagger

@Krastanov Krastanov merged commit 0d68b84 into qojulia:master Jul 21, 2023
8 of 11 checks passed
@akirakyle akirakyle deleted the identitysuperoperator branch July 24, 2023 19:27
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.

3 participants