Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Let's try being a little more flexible with compat...
kshyatt
force-pushed
the
ksh/mps
branch
2 times, most recently
from
1e62663 to
dcb2294
Compare
| @assert !isnothing(I) | ||
| @views dst_dense .= src_dense[:, I] | ||
| # deal with the case where the output is not in-place | ||
| dst_dense === dst_block || copyto!(dst_block, dst_dense) |
There was a problem hiding this comment.
Just out of curiosity, is this ever in-place?
I guess the dst_dense always comes from a similar_dense, which allocates a fresh array?
There was a problem hiding this comment.
I guess in theory, it could be? For now I agree it's always out of place
| TensorKit.foreachblock(t, D) do c, (tblock, bs...) | ||
| tblock′ = lmul!(bs..., copy_dense!(similar_dense(tblock), tblock)) |
There was a problem hiding this comment.
| TensorKit.foreachblock(t, D) do c, (tblock, bs...) | |
| tblock′ = lmul!(bs..., copy_dense!(similar_dense(tblock), tblock)) | |
| TensorKit.foreachblock(t, D) do c, (tblock, Dblock) | |
| tblock′ = lmul!(Dblock, copy_dense!(similar_dense(tblock), tblock)) |
This is just because the splatting in the lmul! looks a bit strange to me since there is no non-2-arg version :)
| TensorKit.foreachblock(t, D) do c, (tblock, bs...) | ||
| tblock′ = rmul!(copy_dense!(similar_dense(tblock), tblock), bs...) |
There was a problem hiding this comment.
| TensorKit.foreachblock(t, D) do c, (tblock, bs...) | |
| tblock′ = rmul!(copy_dense!(similar_dense(tblock), tblock), bs...) | |
| TensorKit.foreachblock(t, D) do c, (tblock, Dblock) | |
| tblock′ = rmul!(copy_dense!(similar_dense(tblock), tblock), Dblock) |
| if eltype(t) === T && typeof(space(t)) === typeof(P) | ||
| return T | ||
| elseif isconcretetype(T) | ||
| elseif isconcretetype(T) || T isa Union |
There was a problem hiding this comment.
Nice catch! this is probably already solving a lot of issues?
|
|
||
| # handle this separately because the storagetype of `AbstractTensorMap` is | ||
| # *always* Vector no matter the actual data storage type | ||
| TK.storagetype(t::BlockTensorMap{AbstractTensorMap{E, S, N₁, N₂}}) where {E, S, N₁, N₂} = TK.promote_storagetype(values(t.data)...) |
There was a problem hiding this comment.
| TK.storagetype(t::BlockTensorMap{AbstractTensorMap{E, S, N₁, N₂}}) where {E, S, N₁, N₂} = TK.promote_storagetype(values(t.data)...) | |
| TK.storagetype(t::BlockTensorMap{AbstractTensorMap{E, S, N₁, N₂}}) where {E, S, N₁, N₂} = | |
| foldl(TK.promote_storagetype, values(t.data)) |
This is just to prevent the compiler from trying to overspecialize on the different amount of nonzeros.
|
|
||
| # handle this separately because the storagetype of `AbstractTensorMap` is | ||
| # *always* Vector no matter the actual data storage type | ||
| TK.storagetype(t::SparseBlockTensorMap{AbstractTensorMap{E, S, N₁, N₂}}) where {E, S, N₁, N₂} = TK.promote_storagetype(values(t.data)...) |
There was a problem hiding this comment.
| TK.storagetype(t::SparseBlockTensorMap{AbstractTensorMap{E, S, N₁, N₂}}) where {E, S, N₁, N₂} = TK.promote_storagetype(values(t.data)...) | |
| TK.storagetype(t::SparseBlockTensorMap{AbstractTensorMap{E, S, N₁, N₂}}) where {E, S, N₁, N₂} = | |
| foldl(TK.promote_storagetype, nonzero_values(t)) |
Note that I changed this for nonzero_values to prevent us depending on values only giving the stored ones, shouldn't matter in the end
| TC′ = TK.promote_permute(TC, sectortype(S)) | ||
| M = TK.promote_storagetype(TK.similarstoragetype(A, TC′), TK.similarstoragetype(B, TC′)) | ||
| # explicitly compute storagetype here to work around eltype of AbstractTensorMap | ||
| MA = TK.similarstoragetype(TK.storagetype(A), TC′) |
There was a problem hiding this comment.
I'm so sorry to keep doing this, but shouldn't TK.similarstoragetype(A, TC') already give you this, and if not, maybe we should specialize TK.similarstoragetype(::BlockTensorMap, ...) instead of fixing this here?
| SafeTestsets = "0.1" | ||
| Strided = "2.3.3" | ||
| TensorKit = "0.16.4" | ||
| TensorKit = "0.16, 0.17" |
There was a problem hiding this comment.
| TensorKit = "0.16, 0.17" | |
| TensorKit = "0.16.4" |
I think the minimal bound was important, so at least it should be "0.16.4, 0.17". I would be happy to already release this in a new patch, but only if we wait for the TensorKit v0.17 release, otherwise I would merge this but wait until TensorKit releases v0.17 before tagging this. Do you have a preference about what is most convenient? I would prefer to keep the 0.16.4 for now
| return KernelAbstractions.get_backend(first(BA.blocks)) | ||
| end | ||
|
|
||
| function Base.copyto!(dest::BM, src::TA) where {T <: Number, TA <: AnyGPUMatrix{T}, BM <: BlockMatrix{T, Matrix{TA}}} |
There was a problem hiding this comment.
This is a small design question, but I would almost prefer to use Base.copy! instead of copyto!. I think that is slightly more idiomatic, since we are copying arrays and care about axes, rather than simply copying regions of memory. What do you think?
2 participants
OK, I think we do actually need the GPU-specific
copyto!for things to work. We also need some explicit promotion strategies due to the extremely annoying wayAbstractTensorMapstorage types work 😸