Fix #556, use IdentityUnitRange in place of Base.Slice

[pablosanjose]

This is my attempt to fix #556, using OffsetArrays/#62 and the similar code in base/abstractarrays.jl as inspiration. My understanding of the underlying Slice/IdentityUnitRange mechanisms is a bit sketchy, so please review with some care (@timholy)
With this PR, the cases in #556 (s[:, 1:1] and s[:, [true, false]]) both work and yield MArrays. In the 0.7 days, this resulted in an Array so I'm assuming we've improved over 0.7?

However s[1:2, 1:1] always yields an Array, both in 0.7, 1.1 and with this PR. This suggests to me that work is perhaps not complete here.

[coveralls]

Coverage decreased (-0.02%) to 66.277% when pulling 61a4762 on pablosanjose:slices into ae14694 on JuliaArrays:master.

[pablosanjose]

Okay, so I think the travis tests fail because the type of s[:, 1:2] is different in versions <=0.7 than in versions >= 1.0. How does one handle this kind of situation?

[mbauman]

I just started looking into this myself from the PkgEval failures in JuliaLang/julia#30374 (comment) — this is the root cause of the AstroLib failure over there.

This seems to be a good fix. My first attempt at a fix (before finding this PR) was to peel back things at a lower level. We could inform Base that SOneTo can serve as an IdentityUnitRange itself with Base.IdentityUnitRange(s::SOneTo) = s, but that's not sufficient and it feels a bit more hacky. It might allow StaticArrays and OffsetArrays to play together a bit more nicely, but I'm not sure yet.

Regarding the fact that s[1:2, 1:1] returns an Array whereas s[:, 1:1] and s[:, [true, false]] return MArrays — I think that's a feature! I would expect, however that s[SOneTo(2), SOneTo(1)] would return an MArray. The key is that the latter constructs have statically sized indices, so they can return a statically sized output.

Edit: Oh, my last paragraph is wrong. s[:, 1:1] and s[:, [true, false]] have a static number of rows but not columns. Hmm.

[mbauman]

I don't think I'd include Integer here — if you want to get a static array from an indexing slice, then I think you should have to use a static array of indices to do so.

[pablosanjose]

If I don't include it, the method similar(::Type{A},::Type{T}, shape::Tuple{SOneToLike, Vararg{SOneToLike}}) in line 134 will not get dispatched to by the similar call from base (which is the origin of the error here), and the whole thing stops working. This seems to be the approach suggested in Base, I think. Check https://github.com/JuliaLang/julia/blob/e83693749032d28e9f5f0f75695fad46729ea2b4/base/abstractarray.jl#L581 and surrounding code/comments.

[mbauman]

Ah, right, of course. We need dispatch on a Tuple with at least one SOneTo to accomplish what we want here, which of course we cannot currently express (directly).

Here's what I think I'd do, in pseudocode:

const PossiblyStaticAxis = Union{Integer, OneTo, SOneTo, IdentityUnitRange{<:SOneTo}}
similar(…, ::Tuple{PossiblyStaticAxis, Vararg{PossiblyStaticAxis}}) = static_similar(…)

static_similar(…, ::Tuple{SOneTo, Vararg{SOneTo}}) = actually_static_similar(…)
static_similar(…, ::Tuple{Any, Vararg{Any}}) = similar(…, map(length, …))

I think that should do the trick — and hopefully make things work much better all around. The method tables of similar are disastrously messy, so this is indeed quite hard.

[pablosanjose]

Yeah, I confess I struggle a bit getting my head around this :-). Thanks for your suggestion Matt. Feel free to take over this PR if you want. Otherwise I'll implement your suggestion tomorrow and report back.

[pablosanjose]

The problem with the modification you propose, as far as I can see, is that it only dispatches to actually_static_similar those shapes that are all SOneTo. The original PR first converts each PossiblyStaticAxis element of shape into an integer (the size along each direction), regardless of whether it is an SOneTo, and IdentityUnitRange or an actual Int. It then transforms it into a Size and proceeds.
Another little problem is that I think OneTo needs not be a subtype of PossiblyStaticAxis, as it gets converted to and Int in base, before getting here. After playing with it for a while I've sort of convinced myself that the current PR is probably ok, without considerably reworking the design.

One of the problems of the current design, it seems to me, is the difficulty of making it yield and MArray when shape is an NTuple{Int}, which is why s[1:2, 1:2] isa Array instead of returning an MArray as I think it should. If I try to add a specific dispatch for this, like

similar(::SA, ::Type{T}, shape::Tuple{Integer, Vararg{Integer}}) where {SA<:StaticArray,T} = similar(SA, T, Size(shape))

I run into an ambiguity with the default similar method fallback in base similar(a::AbstractArray, ::Type{T}, dims::Dims{N}).

EDIT: Scratch that, it's as easy as doing

similar(::SA, ::Type{T}, shape::Base.Dims) where {SA<:StaticArray,T} = similar(SA, T, Size(shape))

I added this to the current PR, which now has this nice added bonus

julia> s = @SArray[1 2; 3 4]
2×2 SArray{Tuple{2,2},Int64,2,4}:
 1  2
 3  4

julia> s[1:2, 1:1]
2×1 MArray{Tuple{2,1},Int64,2,2}:
 1
 3

[pablosanjose]

@mbauman : Oops, didn't realise you had began another PR on this and I pushed another commit. No problem, go ahead and close this when you are satisfied. Do have a look at my reply above, particularly to the possibility of returning an MArray for Base.Dims slices

[pablosanjose]

Closing in favor of #569