-
Notifications
You must be signed in to change notification settings - Fork 60
More tweaks #375
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
base: main
Are you sure you want to change the base?
More tweaks #375
Changes from all commits
9e1ebfe
ea787ae
c3d33e2
eb6b985
c4653c8
b46e23e
5932066
46e5037
9fdc185
e948370
28dca1e
259309e
f23ce5c
3504de3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -53,9 +53,7 @@ storagetype(t) = storagetype(typeof(t)) | |||||||||||||
| function storagetype(::Type{T}) where {T <: AbstractTensorMap} | ||||||||||||||
| if T isa Union | ||||||||||||||
| # attempt to be slightly more specific by promoting unions | ||||||||||||||
| Ma = storagetype(T.a) | ||||||||||||||
| Mb = storagetype(T.b) | ||||||||||||||
| return promote_storagetype(Ma, Mb) | ||||||||||||||
| return promote_storagetype(T.a, T.b) | ||||||||||||||
| else | ||||||||||||||
| # fallback definition by using scalartype | ||||||||||||||
| return similarstoragetype(scalartype(T)) | ||||||||||||||
|
|
@@ -103,8 +101,15 @@ similarstoragetype(X::Type, ::Type{T}) where {T <: Number} = | |||||||||||||
|
|
||||||||||||||
| # implement on tensors | ||||||||||||||
| similarstoragetype(::Type{TT}) where {TT <: AbstractTensorMap} = similarstoragetype(storagetype(TT)) | ||||||||||||||
| similarstoragetype(::Type{TT}, ::Type{T}) where {TT <: AbstractTensorMap, T <: Number} = | ||||||||||||||
| similarstoragetype(storagetype(TT), T) | ||||||||||||||
| function similarstoragetype(::Type{TT}, ::Type{T}) where {TT <: AbstractTensorMap, T <: Number} | ||||||||||||||
| return similarstoragetype(storagetype(TT), T) | ||||||||||||||
| end | ||||||||||||||
|
kshyatt marked this conversation as resolved.
|
||||||||||||||
| function similarstoragetype(::Type{<:AbstractTensorMap{T, S, N₁, N₂}}, ::Type{TA}) where {T <: Number, TA <: DenseVector, S, N₁, N₂} | ||||||||||||||
| return similarstoragetype(TA, T) | ||||||||||||||
| end | ||||||||||||||
| function similarstoragetype(t::AbstractTensorMap{T, S, N₁, N₂}, ::Type{TA}) where {T <: Number, TA <: DenseVector, S, N₁, N₂} | ||||||||||||||
| return similarstoragetype(typeof(t), TA) | ||||||||||||||
| end | ||||||||||||||
|
Comment on lines
+107
to
+112
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think my comment got lost in the github UI, but these methods seem a bit strange to me. Since
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes because the problem here is again the AbstractTensorMap, which has a bad default storagetype of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But who is calling this function? For example, I'm assuming this is a result of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying this over at QuantumKitHub/BlockTensorKit.jl@677d141 |
||||||||||||||
|
|
||||||||||||||
| # implement on arrays | ||||||||||||||
| similarstoragetype(::Type{A}) where {A <: DenseVector{<:Number}} = A | ||||||||||||||
|
|
||||||||||||||
|
kshyatt marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -280,7 +280,7 @@ end | |
| # ---------------- | ||
| function TO.tensoradd_type(TC, A::DiagonalTensorMap, ::Index2Tuple{1, 1}, ::Bool) | ||
| M = similarstoragetype(A, TC) | ||
| return DiagonalTensorMap{TC, spacetype(A), M} | ||
| return DiagonalTensorMap{scalartype(M), spacetype(A), M} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit torn by this, does this ever happen that the two don't match? I think we would like I just realized that this is probably because of trying to pass |
||
| end | ||
|
|
||
| function TO.tensorcontract_type( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -21,7 +21,6 @@ struct TensorMap{T, S <: IndexSpace, N₁, N₂, A <: DenseVector{T}} <: Abstrac | |||
| end | ||||
| return TensorMap{T, S, N₁, N₂, A}(data, space) | ||||
| end | ||||
|
|
||||
| # constructors from data | ||||
| function TensorMap{T, S, N₁, N₂, A}( | ||||
| data::A, space::TensorMapSpace{S, N₁, N₂} | ||||
|
|
@@ -34,6 +33,9 @@ struct TensorMap{T, S <: IndexSpace, N₁, N₂, A <: DenseVector{T}} <: Abstrac | |||
| return new{T, S, N₁, N₂, A}(data, space) | ||||
| end | ||||
| end | ||||
| # constructors from another TensorMap -- no-op | ||||
| TensorMap{T, S, N₁, N₂, A}(t::TensorMap{T, S, N₁, N₂, A}) where {T, S <: IndexSpace, N₁, N₂, A <: DenseVector{T}} = t | ||||
|
kshyatt marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I would actually be in favor of only having the second implementation, since there is a subtle semantic difference that I would like to retain: In general, (although this is very poorly documented), the difference between a constructor and a converter is that a converter is meant to be as lossless as possible, and is allowed to take ownership of all parts of the original data. On the other hand, a constructor is typically meant to make an independent copy. You can see this for regular Where it becomes a bit more vague is of course that you want to have a constructor that takes the different fields and builds up an object from that, without having to create an independent copy (e.g. the inner constructors of a type). This is also where the Julia docs are completely underspecified, so this is more something we should try and figure out ourselves. I would say that in this case though, creating an independent copy is probably the safest solution, and it is probably easier to reason in generic code that if you call julia> a = rand(3)
3-element Vector{Float64}:
0.3155466339261911
0.6265705423537413
0.6035244268238111
julia> b = Array(a)
3-element Vector{Float64}:
0.3155466339261911
0.6265705423537413
0.6035244268238111
julia> b[1] = 1
1
julia> a
3-element Vector{Float64}:
0.3155466339261911
0.6265705423537413
0.6035244268238111
julia> c = convert(Array, a)
3-element Vector{Float64}:
0.3155466339261911
0.6265705423537413
0.6035244268238111
julia> c[1] = 1
1
julia> a
3-element Vector{Float64}:
1.0
0.6265705423537413
0.6035244268238111 |
||||
| TensorMap{T, S, N₁, N₂, A}(t::TensorMap{T, S, N₁, N₂}) where {T, S <: IndexSpace, N₁, N₂, A <: DenseVector{T}} = TensorMap(A(t.data), space(t)) | ||||
|
|
||||
| """ | ||||
| Tensor{T, S, N, A<:DenseVector{T}} = TensorMap{T, S, N, 0, A} | ||||
|
|
@@ -407,11 +409,6 @@ for randf in (:rand, :randn, :randexp, :randisometry) | |||
| end | ||||
| end | ||||
|
|
||||
| # Moving arbitrary TensorMaps to CPU | ||||
| #----------------------------- | ||||
| to_cpu(t::TensorMapWithStorage{T, Vector{T}}) where {T} = t # no op | ||||
| to_cpu(t::TensorMap) = convert(TensorMapWithStorage{scalartype(t), similarstoragetype(scalartype(t))}, t) | ||||
|
|
||||
| # Efficient copy constructors | ||||
| #----------------------------- | ||||
| Base.copy(t::TensorMap) = typeof(t)(copy(t.data), t.space) | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -379,7 +379,7 @@ function blas_contract!( | |||||
| bstyle = BraidingStyle(sectortype(C)) | ||||||
| bstyle isa SymmetricBraiding || | ||||||
| throw(SectorMismatch("only tensors with symmetric braiding rules can be contracted; try `@planar` instead")) | ||||||
| TC = scalartype(C) | ||||||
| TC = storagetype(C) # without this, Anew below has wrong storagetype | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I understand that this is a way to make the issues go away, but I don't think this is really the right strategy. If For example, what We might also just want to schedule a meeting about this and talk through the implications, since I do agree that there are advantages and disadvantages to making things work more automatic, and I might just be overly dense about this.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is not having this breaks all of the more complex MPSKit support, which is why I added it in the first place.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is reworking how all of this works right now worth forcing everyone in the group who wants to try GPU + MPSKit to wait (be realistic here) another 6 months?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the relevant arrays are already on the GPU but this is currently undetectable because of previously made decisions that obviously never took the concept of "different array storage locations" into account. My strong preference would be:
Then at least we will have a working version to see if things break, and MPSKit can (finally) start moving again. |
||||||
|
|
||||||
| # check which tensors have to be permuted/copied | ||||||
| copyA = !(TO.isblascontractable(A, pA) && scalartype(A) === TC) | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.