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

Question: Error with creating Size-1 SVector of Size-1 SVector #1151

Closed
aafsar opened this issue Apr 3, 2023 · 7 comments · Fixed by #1153
Closed

Question: Error with creating Size-1 SVector of Size-1 SVector #1151

aafsar opened this issue Apr 3, 2023 · 7 comments · Fixed by #1153

Comments

@aafsar
Copy link
Contributor

aafsar commented Apr 3, 2023

I'm wondering why it is not allowed to create 1-element SVector of a 1-element SVector by using the constructor with type parameters are given? One can create the following nested SArrays with the explicit constructor:

  • 1-element SVector of a 2-element SVector
  • 2-element Svector of two 1-element SVector

Moreover using SA constructor seems to create 1-element SVector of a 1-element SVector.

using StaticArrays

# Inner SVectors
my_vector_1d = SVector{1, Float64}(0.8)
my_vector_2d = SVector{2, Float64}(0.8, 0.9)

# Container SVectors
# Size-1 SVector of Size-1 SVector
# my_container_faulty = SVector{1, SVector{1, Float64}}(my_vector_1d)  # gives error
my_container_a = SA[my_vector_1d]
# Size-2 SVector of Size-1 SVectors
my_container_b = SVector{2, SVector{1, Float64}}(my_vector_1d, my_other_vector_1d)
# Size-1 SVector of Size-2 Svector
my_container_c = SVector{1, SVector{2, Float64}}(my_vector_2d)

And the error message is:

julia> my_container_faulty = SVector{1, SVector{1, Float64}}(my_vector_1d)
ERROR: MethodError: Cannot `convert` an object of type Float64 to an object of type SVector{1, Float64}
Closest candidates are:
  convert(::Type{SVector{N, T}}, ::CartesianIndex{N}) where {N, T} at ~/.julia/packages/StaticArrays/a4r2v/src/SVector.jl:8
  convert(::Type{T}, ::Factorization) where T<:AbstractArray at /Applications/Julia-1.8.app/Contents/Resources/julia/share/julia/stdlib/v1.8/LinearAlgebra/src/factorization.jl:58
  convert(::Type{SA}, ::Tuple) where SA<:StaticArray at ~/.julia/packages/StaticArrays/a4r2v/src/convert.jl:179
  ...
Stacktrace:
 [1] macro expansion
   @ ~/.julia/packages/StaticArraysCore/U2Z1K/src/StaticArraysCore.jl:81 [inlined]
 [2] convert_ntuple
   @ ~/.julia/packages/StaticArraysCore/U2Z1K/src/StaticArraysCore.jl:77 [inlined]
 [3] SVector{1, SVector{1, Float64}}(x::Tuple{Float64})
   @ StaticArraysCore ~/.julia/packages/StaticArraysCore/U2Z1K/src/StaticArraysCore.jl:113
 [4] SVector{1, SVector{1, Float64}}(sa::SVector{1, Float64})
   @ StaticArrays ~/.julia/packages/StaticArrays/a4r2v/src/convert.jl:167
 [5] top-level scope
   @ REPL[54]:1

Also asked in Julia Discourse

@N5N3
Copy link
Contributor

N5N3 commented Apr 3, 2023

The default constructor for SArray tends to unwrap the input StaticArray eagerly unless we know it's always invalid.
For now, we only verify input's length so user has to add a tuple wrapper manually in this case.

julia> SVector{1}(SVector(1))
1-element SVector{1, Int64} with indices SOneTo(1):
 1

julia> SVector{1}((SVector(1), ))
1-element SVector{1, SVector{1, Int64}} with indices SOneTo(1):
 [1]

@aafsar
Copy link
Contributor Author

aafsar commented Apr 7, 2023

I see, thank you @N5N3! And I guess SA works because 1-element SVector is wrapped in a vector there.

A follow-up due to curiosity: The need to have an additional tuple wrapper occurs only for 1-element SVector of an 1-element SVector, which can be seen as an inconsistent behavior.

A relevant example case: if one works on the subsets of a given set of hyperplanes of a N+1 dimensional space, then one needs several subtypes of SVector{N, Float64}, where N can be 1, 2, 3 ... But in order to ensure that the computation work for any dimensions, one has to write separate methods wherever the subset might contain only one element, if one wants to steer away from type-piracy.

Maybe these cases are very infrequent and wouldn't worth any overhead, albeit very small, for the most users of the package, but I guess one way to ensure consistency is to provide an explicit constructor for SVector{1, SVector{1, T}} that doesn't unwrap. Would this make sense?

@N5N3
Copy link
Contributor

N5N3 commented Apr 7, 2023

And I guess SA works because 1-element SVector is wrapped in a vector there.

SA works because it always treats inputs as elements, If you check @code_warntype:

julia> @code_warntype SA[SVector(1)]
MethodInstance for getindex(::Type{SA}, ::SVector{1, Int64})
  from getindex(sa::Type{SA}, xs...) @ StaticArrays C:\Users\MYJ\.julia\packages\StaticArrays\a4r2v\src\initializers.jl:31
Arguments
  #self#::Core.Const(getindex)
  sa::Type{SA}
  xs::Tuple{SVector{1, Int64}}
Body::SVector{1, SVector{1, Int64}}
1nothing%2 = StaticArrays.length(xs)::Core.Const(1)
│   %3 = StaticArrays.Size(%2)::Core.Const(Size(1,))
│   %4 = StaticArrays.similar_type(sa, %3)::Core.Const(SArray{Tuple{1}})
│   %5 = (%4)(xs)::SVector{1, SVector{1, Int64}}
└──      return %5

You will find a Tuple wrapper is added to ensure this, which matches the general rule of our array literals

which can be seen as an inconsistent behavior.

The inner constructor of SArray only supports Tuple as inputs, all others are convenient extensions.
Perhaps we just extended too much.
IIRC there's some topic in this repo about dropping the syntax of SVector(x, y, z) and replace it with SVector((x, y, z)).

provide an explicit constructor for SVector{1, SVector{1, T}} that doesn't unwrap

Just as mentioned above, wrap your elements with a tuple would resolve all possible ambiguilty.
If your can ensure that x is the element of your output, just try SVector((x,)), there would be no extra overhead.

@aafsar
Copy link
Contributor Author

aafsar commented Apr 7, 2023

@N5N3 Thank you again for your detailed answer!

Perhaps we just extended too much.

Fair. I wouldn't be able to assess trade-offs, but as a user of the package, it is extremely beneficial that the interface of StaticArray is as close to standard Arrays as possible, even if it is through convenient extensions. For example, I can by-pass allocations by passing a Generator to construct a SVector except this "Size-1 x Size-1" case, but how can I do the tuple-wrapping to work-around it?

my_vector_a = SVector(0.8)  # 1-element SVector{1, Float64}
my_vector_b = SVector(0.9)  # 1-element SVector{1, Float64}
my_container = SVector(my_vector_a, my_vector_b)  # 2-element SVector{2, SVector{1, Float64}}
container_subset_a = SVector{2}(my_container[j] for j in 1:2)  # 2-element SVector{2, SVector{1, Float64}}
container_subset_b = SVector{1}(my_container[j] for j in 1:1)  # 1-element SVector{1, Float64}

@N5N3
Copy link
Contributor

N5N3 commented Apr 7, 2023

The result should be SVector{1, SVector{1, Float64}}.
There's no reason to "unwrap" input twice. This looks like a bug in sacollect (It misses the needed tuple wrapper.)

@aafsar
Copy link
Contributor Author

aafsar commented Apr 7, 2023

Could it be this line where the tuple wrapper is missed?

push!(stmts, :(SA($(args...))))

@aafsar
Copy link
Contributor Author

aafsar commented Apr 7, 2023

Opened PR #1153.

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

Successfully merging a pull request may close this issue.

2 participants