Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use parent for similar(::PermutedDimsArray) #35304

Merged
merged 4 commits into from
Apr 6, 2020
Merged

Use parent for similar(::PermutedDimsArray) #35304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a39d329
Use parent for similar(::PermutedDimsArray)
Mar 30, 2020
55085da
add test using TSlow array type
Mar 31, 2020
66df48f
tidier isdefined check
Apr 4, 2020
e6e7b69
revert isdefined check
Apr 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion base/permuteddimsarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,11 @@ function PermutedDimsArray(data::AbstractArray{T,N}, perm) where {T,N}
end

Base.parent(A::PermutedDimsArray) = A.parent
Base.size(A::PermutedDimsArray{T,N,perm}) where {T,N,perm} = genperm(size(parent(A)), perm)
Base.size(A::PermutedDimsArray{T,N,perm}) where {T,N,perm} = genperm(size(parent(A)), perm)
Base.axes(A::PermutedDimsArray{T,N,perm}) where {T,N,perm} = genperm(axes(parent(A)), perm)

Base.similar(A::PermutedDimsArray, T::Type, dims::Base.Dims) = similar(parent(A), T, dims)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Transpose and Adjoint are for linear algebra which we currently support only for arrays with indexing starting with 1, it's OK for those to only support Dims. But PermutedDimsArray supports more general array types, so we have to consider the case where similar will receive axes rather than dims input. From

julia/base/abstractarray.jl

Lines 624 to 645 in 12acd69

similar(a::AbstractArray{T}) where {T} = similar(a, T)
similar(a::AbstractArray, ::Type{T}) where {T} = similar(a, T, to_shape(axes(a)))
similar(a::AbstractArray{T}, dims::Tuple) where {T} = similar(a, T, to_shape(dims))
similar(a::AbstractArray{T}, dims::DimOrInd...) where {T} = similar(a, T, to_shape(dims))
similar(a::AbstractArray, ::Type{T}, dims::DimOrInd...) where {T} = similar(a, T, to_shape(dims))
# Similar supports specifying dims as either Integers or AbstractUnitRanges or any mixed combination
# thereof. Ideally, we'd just convert Integers to OneTos and then call a canonical method with the axes,
# but we don't want to require all AbstractArray subtypes to dispatch on Base.OneTo. So instead we
# define this method to convert supported axes to Ints, with the expectation that an offset array
# package will define a method with dims::Tuple{Union{Integer, UnitRange}, Vararg{Union{Integer, UnitRange}}}
similar(a::AbstractArray, ::Type{T}, dims::Tuple{Union{Integer, OneTo}, Vararg{Union{Integer, OneTo}}}) where {T} = similar(a, T, to_shape(dims))
# similar creates an Array by default
similar(a::AbstractArray, ::Type{T}, dims::Dims{N}) where {T,N} = Array{T,N}(undef, dims)
to_shape(::Tuple{}) = ()
to_shape(dims::Dims) = dims
to_shape(dims::DimsOrInds) = map(to_shape, dims)::DimsOrInds
# each dimension
to_shape(i::Int) = i
to_shape(i::Integer) = Int(i)
to_shape(r::OneTo) = Int(last(r))
to_shape(r::AbstractUnitRange) = r
to avoid ambiguities it looks like we might need

similar(a::PermutedDimsArray, ::Type{T}, dims::Tuple{Union{Integer, OneTo}, Vararg{Union{Integer, OneTo}}}) where T
similar(a::PermutedDimsArray, ::Type{T}, dims::Dims{N}) where {T,N}
similar(a::PermutedDimsArray, ::Type{T}, axs::Tuple{Union{Integer, AbstractUnitRange}, Vararg{Union{Integer, AbstractUnitRange}}}) where T

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for looking up what's needed, I will add these methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what happens if I do add those methods:

using SparseArrays, OffsetArrays
sp = sparse(ones(3,3))

similar(transpose(sp), 3,3)
similar(transpose(sp), 0:2,0:2)

similar(PermutedDimsArray(sp, (2,1)), 3,3)     # isa Array
similar(PermutedDimsArray(sp, (2,1)), 0:2,0:2) # OffsetArray(Array)

# initial PR:
Base.similar(A::PermutedDimsArray, T::Type, dims::Base.Dims) = similar(parent(A), T, dims)

similar(PermutedDimsArray(sp, (2,1)), 3,3)     # isa SparseMatrixCSC
similar(PermutedDimsArray(sp, (2,1)), 0:2,0:2) # OffsetArray(SparseMatrixCSC)

# add extra methods:
Base.similar(A::PermutedDimsArray, ::Type{T}, dims::Tuple{Union{Integer, Base.OneTo}, Vararg{Union{Integer, Base.OneTo}}}) where T = similar(parent(A), T, dims)
Base.similar(A::PermutedDimsArray, ::Type{T}, axs::Tuple{Union{Integer, AbstractUnitRange}, Vararg{Union{Integer, AbstractUnitRange}}}) where T = similar(parent(A), T, axs)

similar(PermutedDimsArray(sp, (2,1)), 3,3)     # isa SparseMatrixCSC
similar(PermutedDimsArray(sp, (2,1)), 0:2,0:2) # is ambiguous. Candidates:
#   similar(A::PermutedDimsArray, ::Type{T}, axs::Tuple{Union{Integer, AbstractUnitRange},Vararg{Union{Integer, AbstractUnitRange},N} where N}) where T in Main at REPL[13]:1
#   similar(A::AbstractArray, ::Type{T}, inds::Tuple{Union{Colon, Integer, Base.IdentityUnitRange, Base.OneTo, UnitRange, OffsetArrays.IdOffsetRange},Vararg{Union{Colon, Integer, Base.IdentityUnitRange, Base.OneTo, UnitRange, OffsetArrays.IdOffsetRange},N} where N}) where T in OffsetArrays at /Users/me/.julia/packages/OffsetArrays/WmVaB/src/OffsetArrays.jl:102
# Possible fix, define
#   similar(::PermutedDimsArray, ::Type{T}, ::Tuple{Union{Integer, Base.IdentityUnitRange, Base.OneTo, UnitRange, OffsetArrays.IdOffsetRange},Vararg{Union{Integer, Base.IdentityUnitRange, Base.OneTo, UnitRange, OffsetArrays.IdOffsetRange},N} where N}) where T

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to solve this at the level of OffsetArrays, presumably. JuliaArrays/OffsetArrays.jl#87. I don't have a solution yet but I haven't had time to poke at it (and neither has anyone else, apparently).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't look easy... but I guess until a larger change lands, this defn is OK?


Base.unsafe_convert(::Type{Ptr{T}}, A::PermutedDimsArray{T}) where {T} = Base.unsafe_convert(Ptr{T}, parent(A))

# It's OK to return a pointer to the first element, and indeed quite
Expand Down
10 changes: 10 additions & 0 deletions test/arrayops.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
# Array test
isdefined(Main, :OffsetArrays) || @eval Main include("testhelpers/OffsetArrays.jl")
using .Main.OffsetArrays

isdefined(@__MODULE__, :T24Linear) || include("testhelpers/arrayindexingtypes.jl")

using SparseArrays

using Random, LinearAlgebra
Expand Down Expand Up @@ -683,6 +686,13 @@ end
y = [0, 0, 0, 0]
copyto!(y, x)
@test y == [1, 2, 3, 4]

# similar, https://github.com/JuliaLang/julia/pull/35304
x = PermutedDimsArray([1 2; 3 4], (2, 1))
@test similar(x, 3,3) isa Array
z = TSlow([1 2; 3 4])
x_slow = PermutedDimsArray(z, (2, 1))
@test similar(x_slow, 3,3) isa TSlow
end

@testset "circshift" begin
Expand Down