Julia: Document Base.elsize and requirements for DenseArray subtypes better

[oschulz]

Update: not a regression, see remarks by Matt below

I'm not sure whether this technically is a regression or not: With Julia v1.5.0-rc1 and v1.6-nightly, some linear algebra breaks on ElasticArrrays.ElasticArray since Base.elsize is not defined:

julia> using Pkg; pkg"add [email protected]"

julia> using ElasticArrays, LinearAlgebra

julia> lmul!(cholesky(Array{Float32}(I(2))).L, view(ElasticArray(Float32[1 1; 1 1]), :, 1))
ERROR: MethodError: no method matching elsize(::Type{ElasticArray{Float32,2,1,Array{Float32,1}}})
Closest candidates are:
  elsize(::Type{var"#s91"} where var"#s91"<:(Array{T,N} where N)) where T at array.jl:222
  elsize(::Base.CodeUnits{T,S} where S<:AbstractString) where T at strings/basic.jl:724
  elsize(::Random.UnsafeView{T}) where T at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Random/src/RNGs.jl:440
  ...
Stacktrace:
 [1] elsize(::ElasticArray{Float32,2,1,Array{Float32,1}}) at ./abstractarray.jl:153
 [2] _memory_offset(::ElasticArray{Float32,2,1,Array{Float32,1}}, ::Int64, ::Vararg{Int64,N} where N) at ./abstractarray.jl:1009
 [3] unsafe_convert(::Type{Ptr{Float32}}, ::SubArray{Float32,1,ElasticArray{Float32,2,1,Array{Float32,1}},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true}) at ./subarray.jl:402
 [4] trmv!(::Char, ::Char, ::Char, ::Array{Float32,2}, ::SubArray{Float32,1,ElasticArray{Float32,2,1,Array{Float32,1}},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/blas.jl:1171
 [5] lmul!(::LowerTriangular{Float32,Array{Float32,2}}, ::SubArray{Float32,1,ElasticArray{Float32,2,1,Array{Float32,1}},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/triangular.jl:737
 [6] top-level scope at REPL[4]:1

When I specialize Base.elsize, all is fine:

julia> Base.elsize(::Type{ElasticArray{T,N,M,V}}) where {T,N,M,V} = Base.elsize(V)

julia> lmul!(cholesky(Array{Float32}(I(2))).L, view(ElasticArray(Float32[1 1; 1 1]), :, 1))
2-element view(::ElasticArray{Float32,2,1,Array{Float32,1}}, :, 1) with eltype Float32:
 1.0
 1.0

This doesn't happen with Julia v1.4. As to whether it's a change in behavior - maybe I should have defined Base.elsize before and just never ran into trouble? On the other hand, Base.elsize isn't documented, so one probably wouldn't expect to have to specialize it for a custom array type.

I'll make a new release of ElasticArrays that defines Base.eltype, but I thought I should report this.

Tested with

julia> versioninfo()
Julia Version 1.5.0-rc1.0
Commit 24f033c951 (2020-06-26 20:13 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake)

and

julia> versioninfo()
Julia Version 1.6.0-DEV.378
Commit c76960224f (2020-07-06 08:44 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake)

[oschulz]

I just released ElasticArrays v1.2.3, it specializes Base.elsize for Julia v1.5 compatibility. To reproduce the issue described above, use ElasticArrays v1.2.2.

[mbauman]

Hm, interesting; this is from #36405. In short, it was definitely a bug that SubArray wasn't considering the elsize in its pointer implementation, and I believe pointer(::ElasticArray, i) would be similarly broken on all 1.x releases. It just so happens that pointer(::SubArray{_,_,<:ElasticArray}) _was_ doing the correct thing in your situation.

We definitely need to document elsize more thoroughly. It needs to appear here: https://docs.julialang.org/en/v1.6-dev/manual/interfaces/#man-interface-strided-arrays-1

[oschulz]

Ah, I see. So elsize should be specialized for custom array types that specialize pointer, but is not for type that just implement getindex, etc.?

[mbauman]

It's needed in this case since you subtype DenseArray, which is itself a member of the StridedArray union, which is what necessitates the definition of pointer because it's hitting the methods that call BLAS. I don't believe we've ever documented what requirements there are to subtype DenseArray (and you probably just defined pointer because you noticed it was broken without it).

[oschulz]

Ahh, right, thanks. I implemented pointer from the beginning, mainly for BLAS support - but I guess it would have been broken without, I just never noticed. :-)

I'll just rename this to "Document Base.eltype and requirements for DenseArray subtypes" Ok?

[oschulz]

Thanks for the explanations, Matt!

[dlfivefifty]

Note that its not specific to subtypes of DenseArray as pointed out in the closed #36910, where PseudoBlockArray was also affected.