Inconsistencies with multi-argument constructors

[timholy]

Given that Scalar <: SArray, the behavior of

StaticArrays.jl/src/Scalar.jl

Line 10 in 552bc75

@inline Scalar(a::AbstractArray) = Scalar{typeof(a)}((a,))

is inconsistent with

StaticArrays.jl/src/convert.jl

Line 5 in 552bc75

@inline (::Type{SA})(a::AbstractArray) where {SA <: StaticArray} = convert(SA, a)

Fortunately, in a few cases there's a very welcome ambiguity error:

julia> sa = Scalar((1,))
Scalar{Int64}((1,))

julia> Scalar(sa)
ERROR: MethodError: Scalar{T} where T(::Scalar{T}) is ambiguous. Candidates:
  (::Type{Scalar{T} where T})(a::AbstractArray{T,0} where T) in StaticArrays at /home/tim/.julia/dev/StaticArrays/src/Scalar.jl:11
  (::Type{Scalar{T} where T})(a::AbstractArray) in StaticArrays at /home/tim/.julia/dev/StaticArrays/src/Scalar.jl:10
  (::Type{SA})(a::StaticArray) where SA<:StaticArray in StaticArrays at /home/tim/.julia/dev/StaticArrays/src/convert.jl:4
Possible fix, define
  Scalar{T} where T(::StaticArray{S,T,0} where T where S<:Tuple)
Stacktrace:
 [1] top-level scope at none:0

but mostly this just silently does things differently for 0 dimensions than for any other dimension.

I don't think you can really have both. Can you choose which behavior you want to keep?

[andyferris]

Right. From a user perspective, we tend to make available the "varargs list of elements" constructor, like SVector(x, y, z).

Do you know what the current best practice is for array constructors vs convert? What do other packages do? I guess this is a bit like the current Number debate... except we've been moving toward having an AbstractArray constructor interface...

[c42f]

The common case here is to convert from a normal array to the particular StaticArray type and this is the same behaviour as Base, for example Vector(1:3) == [1,2,3]. I don't think we can change that without a lot of breakage.

The main place I've used Scalar(a) is for wrapping so that broadcast will not broadcast across the dimensions of a. Which is perhaps why we had this constructor originally (?) However I've always thought Scalar was a poor name for that use case, and that there should probably be something in Base for this in any case.

So IMO we should just deprecate the special Scalar constructor.

[andyferris]

I don't think this is a Scalar issue - it holds for all length-1 static arrays.

We have the "varargs list of elements" constructor on purpose. I particularly don't like special cases where certain lengths (typically 0 or 1) don't follow the overall pattern. Currently, this seems odd to me:

julia> SVector(1:3)
ERROR: UndefVarError: S not defined
Stacktrace:
 [1] Size(::Type{SArray{Tuple{S},T,1,S} where T where S}) at /home/ferris/.julia/dev/StaticArrays/src/traits.jl:86
 [2] length(::Type{SArray{Tuple{S},T,1,S} where T where S}) at /home/ferris/.julia/dev/StaticArrays/src/abstractarray.jl:2
 [3] convert(::Type{SArray{Tuple{S},T,1,S} where T where S}, ::UnitRange{Int64}) at /home/ferris/.julia/dev/StaticArrays/src/convert.jl:20
 [4] SArray{Tuple{S},T,1,S} where T where S(::UnitRange{Int64}) at /home/ferris/.julia/dev/StaticArrays/src/convert.jl:5
 [5] top-level scope at none:0

julia> SVector(1:3, 1:3)
2-element SArray{Tuple{2},UnitRange{Int64},1,2}:
 1:3
 1:3

julia> SVector(1:3, 1:3, 1:3)
3-element SArray{Tuple{3},UnitRange{Int64},1,3}:
 1:3
 1:3
 1:3

If we want to support T(array) -> convert(T, array) then I wonder if we should also drop the 2nd and 3rd from the above?

If we drop the 2nd and 3rd from the above, we lose our nice constructors :(

If we do nothing the situation is confusing.

I feel a bit stuck between a rock and a hard place. Any ideas?

[c42f]

So according to that progression, SVector(1:3) should yield the type SArray{Tuple{1},UnitRange{Int64},1,1}.

If one wants SVector(1,2,3) instead, you need to use SVector{3}(1:3). We normally enforce that people supply the static size to help out the compiler and I thought we used to give a nice explanatory message about supplying static sizes rather than "S is not defined".

So if we say that SVector{N}(a) results in constructing SVector{N} with the N elements of a, and that SVector(x) always results in an SVector with a single element x we kind of have a consistent solution for SVector. Though it's confusing.

On the other hand, for Scalar and user defined FieldVectors without variable size N, there's no way to spell the difference in constructor between the copying vs wrapping version.

It's definitely tricky. It would arguably be most consistent to remove the constructors like SVector(1,2,3) and go to a pure macro or other solution but this would be extremely breaking and also remove one of the most useful notations in the library. I think we could possibly do this only if we came up with a more compelling notation for small StaticArray literals. We've already discussed this at length in #24 and there were some cute ideas but nothing ultimately satisfying. Time to revisit it again, perhaps.

[c42f]

I think we could possibly do this only if we came up with a more compelling notation for small StaticArray literals.

Oh, and also go through a proper deprecation cycle of course.

[timholy]

I feel a bit stuck between a rock and a hard place. Any ideas?

Mind-reading is so easy when the only types ever living in a container are Float64, Float32, and Int, because you always know what the user "means." Now we're in scary-land.

I think it's most important to retain consistency with Base, even if arguably Base's behavior is wrong.
Consequently we should probably get rid of the multi-arg constructors and force people to wrap in tuples, yielding the following behaviors

SVector(1:3) == [1, 2, 3]
SVector((1:3,)) == [1:3]
SVector(1, 2, 3) -> depwarn
SVector((1,2,3)) == [1,2,3]
SVector(((1,2,3),)) == [(1,2,3)]
Scalar([1]) -> depwarn
Scalar(([1],)) gives a 0-dim array

Am I missing important cases?

[andyferris]

To be clear what's on my mind at the moment, I am seriously thinking of following the OP in JuliaLang/julia#29350 (except for arrays, not numbers) and dropping T(x) = convert(T, x) for T <: StaticArray, and more importantly never relying on this assumption for AbstractArrays in my code.

I think the arguments are basically the same as in that issue:

• It seems to me that the constructor is something particular to the concrete type. For example, Julia structs need some kind of constructor to fill their field slots. Since we can't reliably anticipate the needs of the concrete type, it seems presumptuous for an abstract interface to make requirements of the concrete constructors. (Note, one can of course define constructors for AbstractArray(...) or StaticArray(...)).
• convert is a clear semantic that can only mean one thing. The constructor can be ambiguous. (I've also been puzzling over what this "one thing" that convert needs to provide actually is - I think it is something like isequal(x, convert(T, x)::T), which is what things like indexing of AbstractDict and AbstractSet assume. There are exceptions like some Float64 -> Float32 conversions, which arguably might be more semantically sound as round methods, I don't know).
• There is prior art here. E.g. SubArray(1:3) doesn't correspond to a valid method, but is an AbstractArray that is exported from Base. This alone makes using T(array) quite dangerous in generic code in practice, since T <: SubArray can pop up quite often.
• We already know we need a wider range of factories (beyond convert and similar) for construction of AbstractArrays. E.g. there's no pattern in Base like similar that works for immutable arrays. There's also no way to turn a Generator into a given concrete AbstractArray - though I suspect they could all handle them (given that the shape and eltype match). Etc, etc.

[andyferris]

Sorry @timholy I spent so long writing my post that I missed yours.

I guess my question to you is this: is the error in SubArray(1:3) a bug, or a feature?

I think it's most important to retain consistency with Base, even if arguably Base's behavior is wrong.

Yes, it's true that we've generally been pretty religious about following the semantics of Base and LinearAlgebra in this package..

I'm still left with this thought: To me, the priority of consistency would go towards the language itself over its Base standard library - everything from lowering of struct ... end onwards points to the fact that constructors are a property of concrete types, depend on their specific fields, etc.

[c42f]

@andyferris ok good argument. I can see it would work pretty cleanly to just write convert for these things in the generic context. In addition to those considerations... Fixing the generic code in packages which tries to create StaticArrays from normal Arrays to use convert rather than a constructor seems a lot less breaking than fixing the mountains of ad hoc code which assumes SVector(1,2,3) means what it currently does.

[timholy]

I recognize a lot of merit in the distinctions you're making @andyferris. Just be careful about writing StaticArrays according to an imagined Julia 2.0 and forcing that way of thinking on your users, when Julia 2.0 is hopefully at least 5 years away. That's a long time to maintain a disconnect between Base and the way this package works.

[c42f]

But does Base really work this way? The SubArray counter argument seemed kind of convincing to me.

[timholy]

Yes it does, because SubArray requires extra arguments. Consider PaddedViews: can you build one without specifying the pad value? No, and that's wonderful. Therefore this is not a problem.

[andyferris]

I think the point is that if SubArray is free to choose the requirements of it's constructors, as does PaddedView - wouldn't we have the same freedom with SArray?

I take seriously what you are saying about fracturing the interface - but I'm not sure either of the two options you present in the OP would do that, would it? I mean, are all abstract arrays meant to follow (::Type{T})(a::AbstractArray) where {T <: AbstractArray} = convert(T, a)? (I used to think that was implied but now I'm leaning the other way, it doesn't appear to be a requirement of the AbstractArray interface after all?)

(And yes, the broader strokes of my arguments above can't be implemented willy-nilly straight away).

Yes it does, because SubArray requires extra arguments

Is this meaning: it's OK for T(array) to either throw an error, or to be a "copy constructor", but nothing else?

(Sorry for all the questions, Tim!)

[timholy]

(Not that I think I have definitive answers, just opinions.)

In my view errors are nothing to worry about, but magic is. You're arguing, with a great deal of merit, that there's too much magic right now. I agree, but given that SVector is the "official" static version of Vector I think it would be too crazy to have SVector(1:3) return something different from Vector(1:3).

Again, I'm sympathetic with this: heck, I raised a variant of this back in 2012, one of the very few times I've ever used "wart" in conjunction with Julia. But any sufficiently complicated system is going to have warts. I'm just arguing this package---because it is clearly designed to be an adjunct on Base---should choose to have the same warts. That's all.

So I am basically advocating #518 (comment) as a solution. Unless I've missed important cases, that is.

[andyferris]

Thanks Tim for your insights, you've given me much to think about.

On a practical level, I do have concerns about

SVector(1, 2, 3) -> depwarn

since it seems highly disruptive (and is basically the only way I use this package).

We could, for instance, completely punt in the case of one-argument constructors, since not many people use length-1 static arrays. But then again, I was asked to properly support length-0 static arrays... so I suspect this may upset someone...

[timholy]

What's wrong with SVector((1,2,3))?

[andyferris]

Extra typing, I guess? (See #24 - people have argued hard to reduce the current typing)

Really, I want something as compact as [x,y,z] for this kind of work (making 3-vectors can be a really frequent operation when you deal with geometry all day long. The status quo is honestly a PITA at the REPL currently anyway).

My take on SVector(1,2,3) is that the typename follows the ecosystem (like DArray, etc) and from there the constructor is really the minimum possible typing/complexity while remaining pretty obvious in it's meaning.

The StaticArray interface does expect that T(::Tuple) will work, but e.g. it's always seemed to me that a FieldVector should be allowed to have a typical struct-like constructor. (We also support length-1 field vectors...).

[c42f]

Right, for people doing geometry with this package (which was the original reason for creating it) a compact readable literal notation is a big deal. For these people the SVector constructor could easily be the most used function in the entire library. (This is what I meant by extremely breaking!)

[mschauer]

The discoverability of Scalar((a,)) would be bad.

[c42f]

I think this issue is the main conundrum which has been holding up release of StaticArrays 1.0.

I had another think about this while reviewing over at #626 (review) and had some thoughts.

A proposal:

• Create @SA (like @SArray, but perhaps with less features). The name @SA should be short for usability, but I think that an overall reduction of our number of exported macros will compensate for exporting such a short name.
• Deprecate the non-extensible zoo of constructor-like macros we currently have. Possibly have @MA as a concession for a short way to write the canonical mutable static array MArray.
• Deprecate the non-tuple SVector constructors to deal with the problems raised in this issue.

Example usage:

@SA(1)    # Construct 0-dimesional `SArray`
@SA[1,2]  # Constructor SVector{2,Int}
@SA[1 2;  # Construct SMatrix{2,2,Int}
    3 4]

@SA Float64[1 2; # Construct SMatrix{2,2,Float64}
            3 4]

# Use in an expression was greatly improved by https://github.com/JuliaLang/julia/pull/23547
# (note lack of space between `@SA` and `[`
@SA[1,2,3] .+ 1

# For constructing other StaticArray subtypes, recommend composition via conversion, rather than
# defining a zoo of macros `@MMatrix`, `@MVector` etc etc.
MMatrix(@SA[1 2;
            3 4])

# For example, from issue #626, a 3x3 stress tensor
Stress(@SA[1 2 3;
           4 5 6;
           7 8 9])

For FieldVector, I think we need to let people decide on their own constructors for their domain. Reasonable defaults will need some consideration though.

[c42f]

Ok, I've added #633 as one possible alternative syntax for constructors.

Thinking about this for StaticArrays-1.0, I do think Tim's proposal in #518 (comment) is extremely consistent. A staged approach to this would be to pursue a convenient literal syntax like #633 (or equivalent alternative), and see how it pans out in the StaticArrays-1.x release series. If it works out well, we can consider dropping the multi-argument constructors at a later date.

I guess I'm still not sure that a rush for consistency is worth the potential breakage of a lot of practical code. At least staging the deprecation this way will allow people a lot of time to adjust, and providing an even more convenient syntax will give them an incentive 😃