overload Base.rest

[simeonschaub]

This makes the new lhs slurping syntax return an S/MVector if the rhs is an S/MArray. See JuliaLang/julia#37410

[mateuszbaran]

Thanks, that's interesting. Could you also add a method for SizedArray?

[simeonschaub]

I don't think that we can implement this for SizedArray efficiently, since Base.rest relies on the iteration state of the wrapped AbstractArray, so we can't say anything about the resulting length in general in a constant-prop-friendly way. All we could do is implementing it as something like let r=Base.rest(a, st...); SizedArray{length(r)}(r) end, but I am not sure if this is a good idea.

[andyferris]

Ah, awesome, thanks. :)

Could you make this instead for generic
StaticVector using similar_type and decrementing the length appropriately?

I’m not sure about matrices, etc, any ideas?

[mateuszbaran]

I don't think that we can implement this for SizedArray efficiently, since Base.rest relies on the iteration state of the wrapped AbstractArray, so we can't say anything about the resulting length in general in a constant-prop-friendly way. All we could do is implementing it as something like let r=Base.rest(a, st...); SizedArray{length(r)}(r) end, but I am not sure if this is a good idea.

I've changed your implementation to

@inline Base.rest(a::SizedArray{S}, st=(nothing, 0)) where S = SizedVector{StaticArrays.tuple_prod(S)-st[2]}(Base.rest(Tuple(a), st[2] + 1))
@inline Base.rest(a::SArray, st=(nothing, 0)) = SVector(Base.rest(Tuple(a), st[2] + 1))
@inline Base.rest(a::MArray, st=(nothing, 0)) = MVector(Base.rest(Tuple(a), st[2] + 1))

and it constant-propagates fine?

julia> using StaticArrays

julia> @inline Base.rest(a::SizedArray{S}, st=(nothing, 0)) where S = SizedVector{StaticArrays.tuple_prod(S)-st[2]}(Base.rest(Tuple(a), st[2] + 1))

julia> @inline Base.rest(a::SArray, st=(nothing, 0)) = SVector(Base.rest(Tuple(a), st[2] + 1))

julia> @inline Base.rest(a::MArray, st=(nothing, 0)) = MVector(Base.rest(Tuple(a), st[2] + 1))

julia> as = SizedMatrix{2,2}([1 2; 3 4])
2×2 SizedMatrix{2, 2, Int64, 2, Matrix{Int64}} with indices SOneTo(2)×SOneTo(2):
 1  2
 3  4

julia> function ff(sss); a, b... = sss; return sum(b); end
ff (generic function with 1 method)

julia> ff(as)
9

julia> @code_typed ff(as)
CodeInfo(
1 ─ %1  = Base.getfield(sss, :data)::Matrix{Int64}
│         Base.arrayref(true, %1, 1)::Int64
│   %3  = Base.getfield(sss, :data)::Matrix{Int64}
│         Base.arrayref(false, %3, 1)::Int64
│   %5  = Base.getfield(sss, :data)::Matrix{Int64}
│   %6  = Base.arrayref(false, %5, 2)::Int64
│   %7  = Base.getfield(sss, :data)::Matrix{Int64}
│   %8  = Base.arrayref(false, %7, 3)::Int64
│   %9  = Base.getfield(sss, :data)::Matrix{Int64}
│   %10 = Base.arrayref(false, %9, 4)::Int64
│   %11 = $(Expr(:foreigncall, :(:jl_alloc_array_1d), Vector{Int64}, svec(Any, Int64), 0, :(:ccall), Vector{Int64}, 3, 3))::Vector{Int64}
│         Base.arrayset(false, %11, %6, 1)::Vector{Int64}
│         Base.arrayset(false, %11, %8, 2)::Vector{Int64}
│         Base.arrayset(false, %11, %10, 3)::Vector{Int64}
│   %15 = Base.arrayref(false, %11, 1)::Int64
│   %16 = Base.arrayref(false, %11, 2)::Int64
│   %17 = Base.add_int(%15, %16)::Int64
│   %18 = Base.arrayref(false, %11, 3)::Int64
│   %19 = Base.add_int(%17, %18)::Int64
└──       return %19
) => Int64

[simeonschaub]

Ah, I didn't look into this closely enough. I thought SizedArray just forwarded calls to iterate to the underlying AbstractArray, but it looks like it always uses the standard Base implementation of iterate for AbstractArrays with IndexLinear(), so specializing Base.rest on SizedArray as well seems sensible to me.

julia> @inline Base.rest(a::SizedArray{S}, st=(nothing, 0)) where S = SizedVector{StaticArrays.tuple_prod(S)-st[2]}(Base.rest(Tuple(a), st[2] + 1))

Is converting a to a Tuple here a good idea? I thought the point of using SizedArray over S/MArray was to avoid excessive specializations due to large tuples, but I might be wrong.

[simeonschaub]

Could you make this instead for generic
StaticVector using similar_type and decrementing the length appropriately?

I am not sure, what assumtion StaticArray wants to make about how its subtypes implement the iteration protocoll. I don't think it's safe to assume in general that the iteration state always consists of linear indices, which makes Base.rest quite hard to implement generically for all StaticArrays.

I’m not sure about matrices, etc, any ideas?

The way it's implemented in Base, Base.rest always returns AbstractVector for an AbstractArray of any dimensions, which made the most sense to me.

[mateuszbaran]

Ah, I didn't look into this closely enough. I thought SizedArray just forwarded calls to iterate to the underlying AbstractArray, but it looks like it always uses the standard Base implementation of iterate for AbstractArrays with IndexLinear(), so specializing Base.rest on SizedArray as well seems sensible to me.

Until recently it just wrapped a normal Array and I've just extended it to arbitrary AbstractArrays to the point that I needed it. Maybe it would make sense for SizedArray to forward iteration to the wrapped AbstractArray, I don't know enough about iteration to tell. At the very least it's commonly assumed in SizedArray that elements are linearly indexable starting with 1. Reviewing and modifying the entire codebase to support other iteration methods would be a big project.

Is converting a to a Tuple here a good idea? I thought the point of using SizedArray over S/MArray was to avoid excessive specializations due to large tuples, but I might be wrong.

As far as I know you get the same specializations unless you operate on the wrapped array. Converting to Tuple should be not surprising, and people can always use the wrapped array if they choose so.

Currently, at least for me, the main reason to use SizedArray is to wrap SubArrays.

EDIT: "should be surprising" -> "should not be surprising"

[mateuszbaran]

And one more thing, you need to propagate the element type to the constructor to make it work for static arrays of length 1:

@inline Base.rest(a::SizedArray{S}, st=(nothing, 0)) where {S,T} = SizedVector{StaticArrays.tuple_prod(S)-st[2],T}(Base.rest(Tuple(a), st[2] + 1))
@inline Base.rest(a::SArray{S,T}, st=(nothing, 0)) where {S,T} = SVector{StaticArrays.tuple_prod(S)-st[2],T}(Base.rest(Tuple(a), st[2] + 1))
@inline Base.rest(a::MArray{S,T}, st=(nothing, 0)) where {S,T} = MVector{StaticArrays.tuple_prod(S)-st[2],T}(Base.rest(Tuple(a), st[2] + 1))

julia> a, b... = SA[1]
1-element SVector{1, Int64} with indices SOneTo(1):
 1

julia> b
0-element SVector{0, Int64} with indices SOneTo(0)

I think you should add this as a test case.

[simeonschaub]

Hmm, the MacOS runner seems to be hanging during Pkg setup...

[andyferris]

This is nice; it's also very close to the generic version I requested. Instead of worrying about loops over concrete types it should just be:

@inline function Base.rest(a::StaticArray, (_, i) = (nothing, 0))
    newlen = StaticArrays.tuple_prod(Size(a)) - i
    return similar_type(typeof(a), Size(newlen))(Base.rest(Tuple(a), i + 1))
end

[andyferris]

Is converting a to a Tuple here a good idea? I thought the point of using SizedArray over S/MArray was to avoid excessive specializations due to large tuples, but I might be wrong.

The purpose of SizedArray is precisely to get the specializations applied to an already-allocated Array (or AbstractArray these days) - the point partly being convenience of using an already existing object, partly having a mutable container and not having to reallocate it just to apply Size information, and partly because MArray only works for isbits element types (so - "convenience, speed, correctness").

[simeonschaub]

This is nice; it's also very close to the generic version I requested. Instead of worrying about loops over concrete types it should just be:

@inline function Base.rest(a::StaticArray, (_, i) = (nothing, 0))
    newlen = StaticArrays.tuple_prod(Size(a)) - i
    return similar_type(typeof(a), Size(newlen))(Base.rest(Tuple(a), i + 1))
end

See my comment above. The problem with that is that it would make the specific implementation of iterate part of the implicit API of StaticArray. I am not sure, this is a good idea. It would also cause problems, if someone wanted to implement a StaticArray with offset indices, for example.
Actually, I believe SizedArrays can also have offset indices, so the current implementation would not be valid then.

[andyferris]

it would make the specific implementation of iterate part of the implicit API of StaticArray

It already is! :)

By default StaticArrays have one-based linear indexing - I have written much "generic" code here under this assumption. If you look at SizedArray.jl there is no preservation of the parents keys or axes or IndexStyle (in fact, will it even work at all with an offset array? The index is passed through). Everything, and I mean everything including Rotations.jl and other packages, relies on the first few lines of abstractarray.jl.

If someone wanted to specialize a static array that is a bit different they surely could but I'd expect them to overide many of the provided methods, including rest.

To be clear - I am very thankful that you are considering such cases, but there is so much work to be done to properly support axes and Cartesian indexing styles and Slice that I feel it would be worth doing the "mixed-static-dynamic-sized" arrays at the same time (e.g. 3xN matrices), which I would do from a clean slate, to be honest (and hey, it wouldn't be the first time that this package (or its predesessors) were rewritten :) ).

[simeonschaub]

Bump. Can this be merged?