Complete Array API for TArray
#82
Conversation
Most of the newly added methods are from Tracker.jl's TrackedArray.
src/tarray.jl
Outdated
| for F in (:iterate, :eltype, :length, :size, | ||
| :firstindex, :lastindex, :ndims, :axes, | ||
| :collect) | ||
| @eval Base.$F(a::TArray, args...) = $F(get(a), args...) | ||
| end |
There was a problem hiding this comment.
Same here, I would assume that ndims and eltype are defined automatically. I think one has only to define size and similar(::TArray, ::Type{T}, dims), and probably should define IndexStyle and axes to exploit the structure of the underlying array. Additionally, maybe length is useful, but it is not needed since the default is length(x) = prod(size(x)). See https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-array for more explanations.
There was a problem hiding this comment.
removed:
eltypelengthndimscollect
src/tarray.jl
Outdated
| Base.hcat(x::TArray, rest...) = hcat(get(x), _get.(rest)...) |> localize | ||
| Base.hcat(x::AbstractArray, y::TArray, rest...) = hcat(x, get(y), _get.(rest)...) |> localize | ||
| Base.vcat(x::TArray, rest...) = vcat(get(x), _get.(rest)...) |> localize | ||
| Base.vcat(x::AbstractArray, y::TArray, rest...) = vcat(x, get(y), _get.(rest)...) |> localize | ||
| Base.cat(x::TArray, rest...; dims) = cat(get(x), _get.(rest)...; dims = dims) |> localize | ||
| Base.cat(x::AbstractArray, y::TArray, rest...; dims) = | ||
| cat(x, get(y), _get.(rest)...; dims = dims) |> localize |
There was a problem hiding this comment.
I am a bit worried that some of these dispatches cause method invalidations (it happened before in MCMCChains).
There was a problem hiding this comment.
I think in that case the issue was more because we were dispatching on Array instead of AbstractArray, but I may be misremembering.
| function Base.summary(io::IO, x::TArray) | ||
| print(io, "Task Local Array: ") | ||
| summary(io, get(x)) | ||
| end |
There was a problem hiding this comment.
The default should be fine here, I guess? According to the documentation
For arrays, returns a string of size and type info, e.g. 10-element
Array{Int64,1}.
Examples
≡≡≡≡≡≡≡≡≡≡
julia> summary(1)
"Int64"
julia> summary(zeros(2))
"2-element Array{Float64,1}"
So the default "2-element TArray{Float64,2}" would be consistent with Base.
There was a problem hiding this comment.
I overwrote this just to remind the user this is a task-local array, IMHO, it's a trivial improvement, let me know if you strongly think it is not necessary :)
There was a problem hiding this comment.
I don't have a strong opinion here, I mainly thought that the type TArray printed in the summary already contains this information.
|
Maybe it would be nice to separate the implementations for different packages in different files to make the code easier to read. Probably it would be good to add more tests as well. BTW why did the PR re-add a |
I should probably wait a bit longer before merging. Overall I think this PR is correct and further improvements can be done in future PRs.
I think the script in |
Ah, I see. Maybe one could just keep it at the top level or in a separate |
|
OK, I will move it to |
No description provided.