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

Size of StaticArray type #3

Open
nchisholm opened this issue Mar 23, 2022 · 10 comments
Open

Size of StaticArray type #3

nchisholm opened this issue Mar 23, 2022 · 10 comments

Comments

@nchisholm
Copy link

nchisholm commented Mar 23, 2022

Using ArrayInterface v5.05 and StaticArrays v1.42:

julia> import ArrayInterface as Arr

julia> using StaticArrays

julia> Arr.known_size(SArray{Tuple{2,3}})
(6,)

julia> Arr.size(StaticArray{Tuple{2,3}})
(static(6),)

Are those the intended results? I expected (2, 3) and (static(2), static(3)), respectively. (Though, things work fine for instances of StaticArrays.)

I think what is happening is that IteratorSize(::Type{<:StaticArray}) is defaulting to HasLength() while it might better off be HasShape{N}(), so the wrong method gets called at some point. If that's the case, perhaps its also worth opening a bug report on StaticArrays.

Also, should ArrayInterface.size produce an error if given a Type as an argument? What should be the behavior if one of the dimensions is unknown?

@chriselrod
Copy link
Contributor

chriselrod commented Mar 23, 2022

Are those the intended results?

No.

Also, should ArrayInterface.size produce an error if given a Type as an argument?

That's what it does for Matrix{Float64}, for example. I'm a little surprised this works.

What should be the behavior if one of the dimensions is unknown?

If we wanted to support it, nothing. But given that it doesn't work for Matrix{Float64}, it seems to be currently unsupported.

@nchisholm
Copy link
Author

nchisholm commented Mar 23, 2022

Aha:

julia> import ArrayInterface as Arr

julia> using StaticArrays

julia> Base.IteratorSize(StaticArray{Tuple{2,3}})
Base.HasLength()

julia> Base.IteratorSize(StaticArray{Tuple{2,3}, <:Any, 2})
Base.HasShape{2}()

julia> Arr.known_size(StaticArray{Tuple{2,3}, <:Any, 2})
(2, 3)

So a StaticArray will only produce HasShape{N} if its third type parameter is specified. Then I do no think the problem is with ArrayInterface.

@chriselrod
Copy link
Contributor

It should probably be defined like this:

julia> itersize(::Type{<:SArray{S}}) where {N,S<:Tuple{Vararg{Any,N}}} = Base.HasShape{N}()
itersize (generic function with 1 method)

julia> itersize(SArray{Tuple{3}})
Base.HasShape{1}()

julia> itersize(SArray{Tuple{3,4}})
Base.HasShape{2}()

julia> itersize(SArray{Tuple{3,4,5}})
Base.HasShape{3}()

So that they only need the first parameter.

@nchisholm
Copy link
Author

@chriselrod: As a temporary workaround, I type-pirated something like your code above into the (unpublished) package I am working on, and it did does fix the problem.

Also:

What should be the behavior [of ArrayInterface.size] if one of the dimensions is unknown?

If we wanted to support it, nothing. But given that it doesn't work for Matrix{Float64}, it seems to be currently unsupported.

Could it be useful to have an ArrayInterface.size method (or a separate function) that, when called on a Type that HasShape{N}(), either returns an NTuple{N,StaticInt} equivalent to the known_size or otherwise throws an "indeterminate size" error? I have something like this in my own code.

@chriselrod
Copy link
Contributor

chriselrod commented Mar 23, 2022

or otherwise throws an "indeterminate size" error? I have something like this in my own code.

I'd prefer to return nothings and let the user decide how to handle this being unknown. They're free to throw if that is appropriate.

As a temporary workaround, I type-pirated something like your code above into the (unpublished) package I am working on, and it did does fix the problem.

You could make a PR to StaticArrays.jl

@nchisholm
Copy link
Author

I'd prefer to return nothings and let the user decide how to handle this being unknown. They're free to throw if that is appropriate.

Good point.

You could make a PR to StaticArrays.jl

Sure - I can try to do this whenever I get the chance in the next few of days.

@nchisholm
Copy link
Author

I am thinking something like the following would be quite helpful.

  1. known_size(::Type{AbstractArray}) = nothing to indicate that that an AbstractArray should have a size, but its undefined because the necessary type parameter is free. Currently, known_size(AbstractArray) == (nothing,) instead because IteratorSize(AbstractArray) == HasLength().
  2. Similarly, known_size(StaticArray) and known_size(StaticArray{<:NTuple{N,Any}, <:Any, N} where N)) should also return nothing. Currently, known_size(StaticArray) returns a "missing size error" thrown by the call to StaticArrays.Size.
  3. Currently, known_size(StaticArray{Tuple{2,3}, Float64, 1}) == (2,3), i.e., the last "ndims" type parameter is ignored even though it is in conflict with the first "size" type parameter. Perhaps this should be an error.
  4. known_size(AbstractArray{T,2}) == (nothing, nothing), which is already the case.
  5. known_size(StaticArray{Tuple{3,2}}) should be (3,2). This would be the case if StaticArrays defined IteratorSize discussed above.

To give context, this behavior would be potentially useful for function I am writing that takes an iterator with a statically known size and element type and tries to create an array of the given type, if that type is compatible with the dimensions of iter.

materialize(target_type::Type{<:AbstractArray}}, iter::SizedIterator)

For example, if one passed SArray as target_type, then the size details can be filled in from iter. However, if one passed SVector but ndims(iter) > 1, I want to throw an error to warn the user that something is wrong.

@Tokazama
Copy link
Member

I'm assuming these issues are arising from manually writing out types like StaticArray{Tuple{3,2}}. I'd suggest instead dispatching on the return of ArrayInterface.size(x::StaticArray), which should be just as fast as dispatching on a subtype of StaticArray with specific sizes.

known_size(::Type{T}) should always return a tuple just like size(A). I'm not really sure what the utility of known_size is if we don't even know the number of dimensions. I don't have a strong opinion about what throwing an error or not for a UnionAll; but size(x) is intended to correspond to the dimensions of x. It would be very odd for size to suddenly act like length.

I believe our goal has been to define traits on types that correspond to actual instances, and so far we've just let traits on a UnionAll fail wherever it may. I think Julia as a whole is still working towards better error messages (although the stack traces are super easy to figure out, even if they are a bit verbose). I'm open to appropriately returning an error, but we would need to think about at what point that error should occur and if it's really worth the trouble since it's probably a misuse of the trait.

@nchisholm
Copy link
Author

nchisholm commented Aug 15, 2022

Currently, there is still some unexpected behavior still happening for concrete StaticArray types.

import ArrayInterface as Arr

A = SMatrix{3,3}(1:9)
T = typeof(A)

Arr.known_size(A)    # OK --> (3, 3)
Arr.known_size(T)    # OK --> (3, 3)
Arr.size(A)    # OK --> (static(3), static(3))
Arr.size(T)    # ?? --> (static(9),)

So, size returns the linear dimension instead of the matrix as a tuple of StaticInts, though known_size is fine.

@Tokazama
Copy link
Member

size was never intended to be called on the type. We should probably document this

@ChrisRackauckas ChrisRackauckas transferred this issue from JuliaArrays/ArrayInterface.jl Feb 18, 2023
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

3 participants