StackViews.jl icon indicating copy to clipboard operation
StackViews.jl copied to clipboard

The size of StackView over scalars?

Open findmyway opened this issue 3 years ago • 5 comments

julia> StackView([rand(3,4),rand(3,4)]) |> size
(3, 4, 2)

julia> StackView([rand(3),rand(3)]) |> size
(3, 2)

julia> StackView([3, 3]) |> size
(2, 1)

Shouldn't the last one be of size (2,)?

findmyway avatar May 24 '22 13:05 findmyway

Looks like a constructor bug to me. Should probably change https://github.com/JuliaArrays/StackViews.jl/blob/2fb714fa54f0fa5b70927e8d63e8519f5b695515/src/StackViews.jl#L83-L84

- StackView(slices::AbstractArray...; dims=Val(_default_dims(slices))) = StackView(slices, dims)
- StackView{T}(slices::AbstractArray...; dims=Val(_default_dims(slices))) where T = StackView{T}(slices, dims)
+ StackView(s1, slices::AbstractArray...; dims=Val(_default_dims(slices))) = StackView((s1, slices...), dims)
+ StackView{T}(s1, slices::AbstractArray...; dims=Val(_default_dims(slices))) where T = StackView{T}((s1, slices...), dims)

johnnychen94 avatar May 24 '22 13:05 johnnychen94

I'm developing a package that relies on this feature. Is it possible to apply the change and tag a new release?

findmyway avatar May 25 '22 04:05 findmyway

Just played with it a bit. It looks like there's no simple way to change this behavior -- it would be quite weird if you consider

a = [3, 3]
b = [2, 2]

StackView((a,) ) # (2, 1) -- it would be inconsistent if the size is one dimenional less.
StackView((a, b)) # (2, 2)

it's quite consistent a behavior. Also this seems to be also what cat does:

julia> cat([3, 3], dims=2)
2×1 Matrix{Int64}:
 3
 3

Thus I'm not going to change it here.

For your package usage, you can, however, override this behavior by providing a thin wrapper:

lazystack(x, xs...) = StackView(x, xs...)
lazystack(x::AbstractArray{<:Number}) = x

julia> lazystack([rand(3,4),rand(3,4)]) |> size
(3, 4, 2)

julia> lazystack([rand(3,),rand(3,)]) |> size
(3, 2)

julia> lazystack([3, 3]) |> size
(2,)

johnnychen94 avatar May 25 '22 11:05 johnnychen94

I see. That's fine.

But I still think the behavior is inconsistent.

The analogy you posted above doesn't apply here.


julia> StackView([rand(2,3)]) |> ndims
3

julia> StackView([rand(2)]) |> ndims
2

julia> StackView([rand(0)]) |> ndims
2

So as you can see, when reducing the dimension of inner elements. The expected ndims should be 1 for the last one.


StackView((a,) ) # (2, 1) -- it would be inconsistent if the size is one dimenional less.
StackView((a, b)) # (2, 2)

Here you're using a tuple. In this case, adding new elements (b here) doesn't change the dimension of inner elements, so it is consistent that StackView((a,)) returns (2,2)


julia> cat([3, 3], dims=2)
2×1 Matrix{Int64}:
 3
 3

By definition, the first argument of cat is only one element here. While in the example I posted above, it is wrapped in a container.

findmyway avatar May 25 '22 12:05 findmyway

Thanks for the explanation. I get your point now.

StackViews's current implementation assumes each slice to be an array-like object so getindex doesn't really handle the scalar case well.

I'll try to take this into consideration when I rewrite this -- just not something that would happen in the near future, though...

Things getting even busier 🙈

johnnychen94 avatar May 25 '22 12:05 johnnychen94