Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The size of StackView over scalars? #3

Open
findmyway opened this issue May 24, 2022 · 5 comments
Open

The size of StackView over scalars? #3

findmyway opened this issue May 24, 2022 · 5 comments

Comments

@findmyway
Copy link

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,)?

@johnnychen94
Copy link
Member

Looks like a constructor bug to me. Should probably change

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(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)

@findmyway
Copy link
Author

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

@johnnychen94
Copy link
Member

johnnychen94 commented May 25, 2022

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,)

@findmyway
Copy link
Author

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.

@johnnychen94 johnnychen94 reopened this May 25, 2022
@johnnychen94
Copy link
Member

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 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants