slicedim inconsistency for BitVector vs. other AbstractVectors

[martinholters]

julia> typeof(slicedim(ones(10), 1, 1))
Float64

julia> typeof(slicedim(trues(10), 1, 1))
BitArray{0}

For consistency with getindex, the latter should probably just give Bool, or else the former should give Array{Float64,0}. Found by working on #20154, which would fix this by making the latter give Bool.

[timholy]

I suspect the 0-dimensional array is the more consistent choice. As you pointed out, if d > 1 it's going to return an array, so let's do the same for d == 1.

[martinholters]

Well, for the AbstractArray{T,N} case, we presently have, in pseudo-julia,

slicedim(x, d, i) = 
    if d <= N
        getindex(x, indices(x,1), ..., indices(x,d-1), i, indices(x,d+1), ..., indices(x,N))
    else
        getindex(x, indices(x,1), ..., indices(x,N), 1, ..., 1, i)
    end

where i always occurs in the d-th index position. And this holds for arbitrary i, not just integers, and even if getindex has some weird semantics.

Edit: Actually, the d>N case checks that i==1 (errors otherwise) and then returns getindex(x, indices(x,1), ..., indices(x,N)), assuming proper "trailing singleton dimension indexing" behavior of getindex to be equal to the above.

If we special-case AbstractArray{T,1}, should the return type be a subtype of AbstractArray{T,0} if i is of a specific type? (Integer?) Or if the getindex returns a T? Should it be an Array{T,0}? Not always, e.g. not for BitArray. Should the result be obtained by similar(x, ())? Wouldn't work e.g. for x::SparseMatrixCSC. I'm all ears for reasonable semantics here - slicedim always returning an array, even if 0-dimensional doesn't sound bad at all - but specifying them in a generic fashion seems a bit non-obvious. And it would be really nice if we could provide a generic AbstractArray implementation where performance was the only reason to specialize for specific array types.

[martinholters]

The docstring says "Equivalent to A[:,:,...,i,:,:,...] where i is in position d." So looks like the current BitArray behavior does not match what the docstring says. (But the behavior for d>N is also not covered by the docstring.)

[timholy]

Hmm, that is interesting. I may be biased by the old meaning of slice (now called view), in thinking that it should always return an array. What do others think this should do?

[pabloferz]

I'd say returning an array is more consistent and would avoid type stability issues. If that were the decision to follow then the docs would also have to be modified.

[andreasnoack]

I think the docs just haven't been updated to the APL indexing changes.

[martinholters]

@pabloferz It would not avoid type stability issues to return a (suitable subtype of) AbstractArray{T,0} if not at the same time we forbid the d>N, i==1 case, which returns a (suitable subtype of) AbstractArray{T,1}).

@andreasnoack But the APL indexing changes did not change the behavior when indexing a 1d array with an integer, did they? That always returned a single element, not wrapped in an array of any kind, or am I missing something?

But speaking of the APL indexing changes, would it make sense to do something like the opposite here and always return an array of the same dimensionality as was passed in? I.e. for the d<=N, it would be equivalent to

 getindex(x, indices(x,1), ..., indices(x,d-1), i:i, indices(x,d+1), ..., indices(x,N))

Seems a bit backwards, but doesn't make it necessarily wrong.

[andreasnoack]

But speaking of the APL indexing changes, would it make sense to do something like the opposite here and always return an array of the same dimensionality as was passed in? I.e. for the d<=N, it would be equivalent to

There are cases for and against but even though it could be considered inconsistent with APL indexing, we are preserving the dimensions in mapreducedim and the derived functions so I think it would make sense that mapslices behaves similarly.

[martinholters]

I don't think I've ever actually used slicedim, so I won't make the decision here. And no-one complained so far, so we can probably live with this for 0.6. But if we reach a decision post 0.6.0, we won't be able to backport as it would be a breaking change.

[martinholters]

Change here would be breaking, so unless the decision to live with the present inconsistency, should be done by 1.0.

[mbauman]

This is worth changing. Right now BitVectors aren't obeying the docs which says that slicedim(trues(10), 1, 1) should be equivalent to trues(10)[1].