WIP Add size to abstract StaticArray type #134

andyferris · 2017-04-02T13:05:23Z

This is a continuation of #127 whereby we add a Tuple{...} size descriptor to the abstract StaticArray type.

I'm previewing this WIP to get feedback on the interface, such as whether we want StaticArray{Size, T, N}, or have the dimensions after T, or after N. Any comments appreciated, otherwise I'll carry on.

andyferris · 2017-04-03T12:52:39Z

OK, bulk of work is done, but I haven't updated the tests so many are failing.

More alarmingly, I'm running into an inference crash in the unit tests; similar things have been reported elsewhere.

Tests have not been updated

Still having an inference problem in test/solve.jl. Can't reproduce, however.

andyferris · 2017-04-19T04:59:51Z

This is ready for a look at. There is a problem with inference (world age failure) in a testset for solve, but it makes no sense to me.

I would still like some feedback on design here, please. :)

coveralls · 2017-04-19T05:09:58Z

Coverage decreased (-0.6%) to 69.105% when pulling 2086cb1 on size-in-staticarray-type into b643ff5 on master.

c42f · 2017-04-19T12:23:02Z

Afraid I don't have the energy to look at this tonight... maybe tomorrow.

coveralls · 2017-04-19T21:44:17Z

Coverage increased (+0.03%) to 69.697% when pulling d6480b0 on size-in-staticarray-type into b643ff5 on master.

c42f

Right, I read all this. A bit of a big change, once again (nice effort).

I'm surprised that this didn't cut down on the duplication more than it did.

On the design, I still think this is worth doing to be able to dispatch on size, and I'm not sure how better to arrange the type parameters. What you've got here is consistent with the way it worked in FixedSizeArrays, and I didn't regret the parameter ordering there (that I remember).

If only there were a better solution to all this than the Tuple hack.

c42f · 2017-04-20T11:22:23Z

src/SArray.jl

-    SArray{Size, T, L}(x::NTuple{L, T})
-    SArray{Size, T, L}(x1, x2, x3, ...)
+    SArray{S, T, L}(x::NTuple{L, T})
+    SArray{S, T, L}(x1, x2, x3, ...)


Perhaps drop the L from the constructor usage docs here (the docs below go on to suggest that this is the right thing). Is there any reason people should ever need to specify L explicitly?

This I always find a bit weird. In some sense, we are documenting the type and the constructors. The one without T and L is documented below.

The L is necessary to know about whenever you want to specify it as an element type of an array or a member of a struct, so this was my effort to get the user to find out about it's existence.

Though now I notice that I forgot the N!

Ah, righto. Yes, I find the constructor/type dichotomy odd too. I often wish the doc system would provide more help here, in terms of autodocumenting the type signature docs, in a way which is systematic but not intrusive. Oh ho, look at this trick:

module A """ $(B) B is a thing with some type parameters """ type B{N,T,X,Y,Z} end end help?> A.B A.B{N,T,X,Y,Z} B is a thing with some type parameters

Kinda close?

Still, even if this was neater, it relies on package authors being systematic and good at writing docs...

c42f · 2017-04-20T11:25:11Z

src/SArray.jl

+@inline SArray(a::StaticArray) = SArray{size_tuple(a)}(Tuple(a)) # TODO fixme
+
+# Simplified show for the type
+show(io::IO, ::Type{SArray{S, T, N}}) where {S, T, N} = print(io, "SArray{$S,$T,$N}")


c42f · 2017-04-20T11:31:51Z

src/SArray.jl

-@pure Size{S}(::Type{SArray{S}}) = Size(S)
-@pure Size{S,T}(::Type{SArray{S,T}}) = Size(S)
-@pure Size{S,T,N}(::Type{SArray{S,T,N}}) = Size(S)
-@pure Size{S,T,N,L}(::Type{SArray{S,T,N,L}}) = Size(S)


c42f · 2017-04-20T11:34:44Z

src/SMatrix.jl

@@ -53,9 +53,12 @@ end
    end
 end

-@inline convert{S1,S2,T}(::Type{SMatrix{S1,S2}}, a::StaticArray{T}) = SMatrix{S1,S2,T}(Tuple(a))
+@inline convert{S1,S2,T}(::Type{SMatrix{S1,S2}}, a::StaticArray{<:Any, T}) = SMatrix{S1,S2,T}(Tuple(a))


Why do we want this anyway? It looks like reshape() rather than a constructor.

It does allow reshaping... but primarily it provides the functionality to change T.

Should we tighten this up at some point?

(perhaps a separate PR)

Sure. Chuck in a TODO?

c42f · 2017-04-20T11:37:18Z

src/SUnitRange.jl

@@ -1,8 +1,23 @@
 # `Start` and `End` should be `Int`
-struct SUnitRange{Start,End} <: StaticVector{Int}
+struct SUnitRange{Start, End, L} <: StaticVector{L, Int}


Would this work just as well if parameterized only on Start and L, with End being computed via a pure function?

I guess we could - users can use the @pure constructor on line 20.

c42f · 2017-04-20T11:39:58Z

src/SizedArray.jl

@@ -93,4 +84,4 @@ Creates a `SizedArray` wrapping `array` with the specified statically-known
 `dims`, so to take advantage of the (faster) methods defined by the static array
 package.
 """
-(::Size{S}){S}(a::Array) = SizedArray{S}(a)
+(::Size{S}){S}(a::Array) = SizedArray{Tuple{S...}}(a)


I'm still not convinced this is a good pun!

True. Make a PR :)

c42f · 2017-04-20T11:44:13Z

src/abstractarray.jl

-
-default_similar_type{T,S}(::Type{T}, s::Size{S}, ::Type{Val{0}}) = Scalar{T}
-default_similar_type{T,S}(::Type{T}, s::Size{S}, ::Type{Val{1}}) = SVector{S[1],T}
-default_similar_type{T,S}(::Type{T}, s::Size{S}, ::Type{Val{2}}) = SMatrix{S[1],S[2],T,prod(s)}
 default_similar_type{T,S,D}(::Type{T}, s::Size{S}, ::Type{Val{D}}) = SArray{Tuple{S...},T,D,prod(s)}


Does this work for the Scalar case? I seem to get

julia> SArray{Tuple{},Int,0,0}(()) ERROR: ArgumentError: Size mismatch in Static Array parameters. Got size Tuple{}, dimension 0 and length 0. Stacktrace: [1] check_array_parameters(...) at /home/chris/.julia/v0.6/StaticArrays/src/util.jl:57 [2] StaticArrays.SArray{Tuple{},Int64,0,0}(::Tuple{}) at /home/chris/.julia/v0.6/StaticArrays/src/SArray.jl:22

The length of a Scalar is 1.

Try SArray{Tuple{},Int,0,1}((3,))

Also prod([]) === 1

c42f · 2017-04-20T12:13:42Z

test/solve.jl

+    # I see an inference error in the combined testset below. I can't reproduce
+    # at the REPL or in a function for individual n, etc... speculatively, it
+    # might be reusing A, b with different types in the same (non-toplevel)
+    # scope, for which I've come accross inference bugs in the past.


Somewhat disturbing.

Seems mitigated by the fact that this test case is rather odd with its loop over types though. May not be a problem in practice?

I think it's not a problem in practice, or at least I couldn't find any evidence of a problem.

c42f

Looks good to me, cheers.

Various doc updates as fallout from merging #127, #134.

This is two commits squashed together, with significant modifications to backport to julia-0.5. b1714d9 ----------------------------- * Unify SArray,SMatrix,SVector After the recent refactor something that used to be an SMatrix became a two dimensional SArray (or vice versa - who can keep track). That situation seems unfortunate and unnecessary, so this is an attempt to unify them. This is decently breaking, because it requires the SArray dimension tuple to be e.g. `Tuple{1,2}` rather than `(1,2)`, but should allow us to avoid quite a bit of duplication. These are just the minimal changes to get this running. I would imagine, we could remove some further duplication in follow up patches. * Bring Scalar/MArray into the fold c4008ff ----------------------------- WIP Add size to abstract StaticArray type (#134) * Add size to abstract StaticArray type * Update SUnitRange + bufix

andyferris force-pushed the size-in-staticarray-type branch from a647fca to 770f5c9 Compare April 3, 2017 12:49

andyferris mentioned this pull request Apr 4, 2017

Define size more explicitly, helping the compiler #137

Closed

andyferris force-pushed the size-in-staticarray-type branch from 770f5c9 to 33d3153 Compare April 5, 2017 06:38

c42f mentioned this pull request Apr 7, 2017

Add upper bound on StaticArrays version. JuliaGeometry/Rotations.jl#28

Merged

Andy Ferris added 2 commits April 19, 2017 14:06

Add size to abstract StaticArray type

a000ab2

Tests have not been updated

Fixed some subtyping issues

a0418f6

andyferris force-pushed the size-in-staticarray-type branch from ce3714d to a0418f6 Compare April 19, 2017 04:07

More bugfixes.

2086cb1

Still having an inference problem in test/solve.jl. Can't reproduce, however.

andyferris mentioned this pull request Apr 19, 2017

bring back fixedsizearrays #138

Merged

c42f mentioned this pull request Apr 19, 2017

Tag a new release? #142

Closed

SimonDanisch added a commit that referenced this pull request Apr 19, 2017

merge #134

380c44b

Workaround strange inference failure in tests

d6480b0

c42f reviewed Apr 20, 2017

View reviewed changes

Fix SUnitRange problems and remove a type parameter

3bc67c6

c42f approved these changes Apr 21, 2017

View reviewed changes

andyferris merged commit c4008ff into master Apr 21, 2017

c42f added a commit that referenced this pull request Apr 24, 2017

Various doc updates as fallout from merging #127, #134.

cbed3da

c42f added a commit that referenced this pull request Apr 24, 2017

Various doc updates as fallout from merging #127, #134.

71ae355

c42f added a commit that referenced this pull request Apr 24, 2017

Merge pull request #144 from JuliaArrays/Tuple-doc-updates

4d74ac7

Various doc updates as fallout from merging #127, #134.

SimonDanisch deleted the size-in-staticarray-type branch May 17, 2018 10:57

oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this pull request Apr 4, 2023

bump version (JuliaArrays#134)

7b77672

WIP Add size to abstract StaticArray type #134

WIP Add size to abstract StaticArray type #134

Uh oh!

Conversation

andyferris commented Apr 2, 2017

Uh oh!

andyferris commented Apr 3, 2017

Uh oh!

andyferris commented Apr 19, 2017

Uh oh!

coveralls commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c42f commented Apr 19, 2017

Uh oh!

coveralls commented Apr 19, 2017

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c42f Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyferris Apr 21, 2017 • edited by c42f Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Apr 19, 2017 •

edited

Loading

c42f Apr 21, 2017 •

edited

Loading

andyferris Apr 21, 2017 •

edited by c42f

Loading