Closed GiggleLiu closed 1 year ago
@GiggleLiu what did you have planned here before this can get merged? I suppose tests for von_neumann_entropy, relative_entropy and cross_entropy are missing for now. And is there anything I can help with to get this into a mergeable state?
Yeah, we need more tests. It would be great if you can help with the test writing! ;D
I have some questions regarding the desired behaviour of @assert_locs_safe .
In lines 38-40 of YaoArrayRegister/src/error.jl AddressList s are defined via
const AddressVector{T} = Vector{T} where {T<:Union{Integer,UnitRange}}
const AddressNTuple{N,T} = NTuple{N,T} where {T<:Union{Integer,UnitRange}}
const AddressList{T} = Union{AddressVector{T},AddressNTuple{N,T} where N}
which has the effect that [(1, 2), (3, 4)] isa AddressList
but confusingly not [(1, 2), (3, 4)] isa AddressNTuple
or [(1, 2), (3, 4)] isa AddressVector
because [(1, 2), (3, 4)] isa AddressList{Tuple{Int,Int}}
. Should [(1, 2), (3, 4)]
be an acceptable AddressList or not?
mutual_information(ghz_state(4), (1, 2), (3, 4))
errors, because nonempty_minimum((1, 2))
is not defined. This is fixed easily enough by defining nonempty_minimum(x::Tuple) = minimum(x)
. But if we do that, we get that @assert_locs_safe 4 [(1, 5), (2, 3)]
doesn't error, even though 5 is clearly out of bounds. Should I add another method islocs_inbounds(locs::Vector{NTuple{N,T}})
that makes sure this won't happen?[copied from slack]
I think it is a very good point.
@assert_locs_safe
is designed for put
and kron
, so it should not support [(1,3), (2,4)]
.
To get the following code work, we can collect part1
and part2
into vectors.
@assert_locs_safe nqudits(dm) vcat(part1, part2)
Thanks for the input. It turned out that @assert_locs_safe n part1 ∪ part2
also works, and also looks a bit cleaner IMHO.
Thanks for the input. It turned out that
@assert_locs_safe n part1 ∪ part2
also works, and also looks a bit cleaner IMHO.
Using union can not detect LocationConflictError
.
julia> using Yao
julia> YaoBlocks.@assert_locs_safe 5 [1,2,3]
julia> YaoBlocks.@assert_locs_safe 5 [1,2,2,3]
ERROR: LocationConflictError: locations conflict.
Stacktrace:
[1] top-level scope
@ ~/.julia/dev/Yao/lib/YaoBlocks/src/error.jl:117
julia> YaoBlocks.@assert_locs_safe 5 [1,2] ∪ [2,3]
julia> YaoBlocks.@assert_locs_safe 5 vcat([1,2], [2,3])
ERROR: LocationConflictError: locations conflict.
Stacktrace:
[1] top-level scope
@ ~/.julia/dev/Yao/lib/YaoBlocks/src/error.jl:117
Good spot, thanks!
Also, I remember that we discussed removing LinearAlgebra.ishermitian(::DensityMatrix)
since the user is not really expected to do algebra with the density matrices. None of the tests cover that line (nor the line Base.adjoing(::DensityMatrix)
, so should I go ahead and remove them?
Good spot, thanks!
Also, I remember that we discussed removing
LinearAlgebra.ishermitian(::DensityMatrix)
since the user is not really expected to do algebra with the density matrices. None of the tests cover that line (nor the lineBase.adjoing(::DensityMatrix)
, so should I go ahead and remove them?
Yes, please go ahead, thank you for your effort!
I think I got this PR into a somewhat mergeable state now. Is there way I can alter this PR with the commits on my fork, or should I simply open a separate PR?
just open a separate one, that's the simplest
This can be closed since #461 got merged
Codecov Report
Continue to review full report at Codecov.