RFC: Unify SArray,SMatrix,SVector #127

Keno · 2017-03-30T00:04:02Z

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.

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.

coveralls · 2017-03-30T00:24:33Z

Coverage decreased (-0.4%) to 70.203% when pulling 40f748a on Keno:pull-request/40f748ad into 314f114 on JuliaArrays:master.

andyferris

Thanks @Keno for your efforts here.

We've been talking about this approach a lot, in the past and more recently. Before proceeding, I'd appreciate feedback from @JeffBezanson on whether using uninstantiable Tuple types like Tuple{3,4} as type parameters is a feature we will continue to support going forward? (In the past I've been bitten by abusing corner cases that happen to work in the current compiler get killed off in the future, e.g. GeneratedTypes comes to mind...). For context, the principle advantage (as I see it) of Tuple{3,4} over (3,4) as a type parameter is that you can do subtyping such that SVector{L} <: SArray{Tuple{L}} (and Tuple is the only type that has varargs type parameters - it would be sweeter to do this with the Size trait...). Also, FYI @StefanKarpinski @ararslan - this is the PR discussed on Slack/types.

Also, it seems reasonable to do the same thing for MArray/MVector/MMatrix, as well as incorporating Scalar into the party. Going forward, we could think about attaching these sizes to StaticArray also. (I've been rather enjoying the Size trait - but that would be more workable if we had trait-based dispatch inbuilt into the language).

Any opinions from the rest of the team? @c42f @SimonDanisch @timholy

andyferris · 2017-03-30T01:47:28Z

src/SArray.jl

-    if !(isa(Size, Tuple{Vararg{Int}}))
-        error("SArray parameter Size must be a tuple of Ints (e.g. `SArray{(3,3)}`)")
+@pure tuple_length(T) = length(T.parameters)
+@pure tuple_prod(T) = *(T.parameters...)


Doesn't work for zero-d arrays (I think SArray was previously broken in this case anyway, but it would ideally be nice to fix and make Scalar a zero-d SArray).

Sounds good.

andyferris · 2017-03-30T01:51:00Z

src/SArray.jl

+    error("Internal error. Please file a bug")
+end
+
+@generated function check_sarray_parameters{Size,T,N,L}(::Type{Size}, ::Type{T}, ::Type{Val{N}}, ::Type{Val{L}})


I've been moving to where syntax for parameterized functions (this is of course optional for this PR).

Shoul we parameterize {Size <: Tuple, ...} here?

Yes, seems reasonable.

andyferris · 2017-03-30T01:53:55Z

src/SArray.jl

@@ -19,56 +19,82 @@ immutable SArray{Size, T, N, L} <: StaticArray{T, N}
    data::NTuple{L,T}

    function (::Type{SArray{Size,T,N,L}}){Size,T,N,L}(x::NTuple{L,T})


Ideally, I'd love to remove Size as a type parameter - there is a trait type called Size already so this seems a little ambiguous. I'd meant to get around to it earlier. (This is optional w.r.t. this PR).

andyferris · 2017-03-30T02:00:27Z

src/SArray.jl

    end
 end

-@generated function (::Type{SArray{Size}}){Size, T <: Tuple}(x::T)
+function SArray{Size,T,N}(x::Tuple) where {Size, T, N}
+    Base.depwarn("SArray{(dim1,dim2,...),...} is deprecated, use SArray{Tuple{dim1, dim2, ...}, ...}", :SArray)


Would it be safer to check that Size is indeed an instance of Tuple{Vararg{Int}} before this depwarn, and error instead if not?

Maybe. Arguably, this should just be an error immediately. The only concern here would be something that the type system doesn't let you put in Tuples. Happy to go with whatever you prefer.

andyferris · 2017-03-30T02:30:31Z

After the recent refactor something that used to be an SMatrix became a
two dimensional SArray (or vice versa - who can keep track).

Yeah, I "simplified" similar_type so that canonically we used SVector for 1D containers and SMatrix for 2D containers. I'm happy to review this decision if that helps.

andyferris · 2017-03-30T02:31:22Z

@Keno, yesterday you mentioned a type stability issue in some work you were doing. Was this caused by SArray unexpectedly turning into SMatrix or something?

Keno · 2017-03-30T02:41:33Z

No, that's separate, will be a later PR. It's just a bit tricky, so I wanted to get some more experience with that change, so it's not a PR yet.

andyferris · 2017-03-30T02:52:52Z

Well, let me know if you want some help

andyferris · 2017-03-30T03:18:23Z

It's worth pointing out that we had a discussion with @JeffBezanson about Tuple{3,4}, etc - the most pertinent quote was "so far, I can't think of anything we could achieve by removing that feature".

I feel we can reasonably trust that this will continue working (or else be superseded by something better, like varargs type parameters in user defined types).

c42f · 2017-03-30T09:40:15Z

+1 for this change if Jeff thinks the ability to (ab)use Tuple like this is not going away. In FixedSizeArrays this choice worked reasonably well, I think. We can also put the fixed size back into the abstract StaticArray as a type parameter and reclaim the ability to do dispatch on the size without quite so many traits all over the place.

In all, it's amusing to see this design quirk of FixedSizeArrays come back again :-)

andyferris · 2017-03-30T11:06:50Z

Cool. Sounds good then.

Just to confirm - @SimonDanisch you also have experience with this approach. A good idea, in your opinion?

SimonDanisch · 2017-03-30T11:16:30Z

I never regretted it, besides the occasional fights with @JeffBezanson :)

Keno · 2017-04-01T00:37:43Z

Updated to include MArray/Scalar as well

Keno · 2017-04-01T00:38:31Z

Sorry I pushed some other things that are WIP, will back them out and re-push.

coveralls · 2017-04-01T00:43:22Z

Coverage decreased (-0.7%) to 69.528% when pulling 594150d on Keno:pull-request/40f748ad into bd3236a on JuliaArrays:master.

coveralls · 2017-04-01T00:48:15Z

Coverage decreased (-0.6%) to 69.628% when pulling 594150d on Keno:pull-request/40f748ad into bd3236a on JuliaArrays:master.

andyferris · 2017-04-01T22:53:50Z

OK, thanks @Keno - awesome work.

One question - would it be beneficial to overload show for show(io::IO, ::Type{SVector{N,T}}) where T so that we see SVector. SMatrix and Scalar as previously? It seems reasonable (to me) to do this for commonly used type aliases.

andyferris · 2017-04-02T12:08:28Z

src/MArray.jl

    end
 end

-@inline MArray(a::StaticArray) = MArray{size(typeof(a))}(Tuple(a))
+@inline MArray(a::StaticArray) = MArray{size_tuple(typeof(a))}(Tuple(a))


I think we lost size_tuple somehow...

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

andyferris reviewed Mar 30, 2017

View reviewed changes

Keno force-pushed the pull-request/40f748ad branch from 40f748a to ae1772d Compare April 1, 2017 00:37

Bring Scalar/MArray into the fold

594150d

Keno force-pushed the pull-request/40f748ad branch from ae1772d to 594150d Compare April 1, 2017 00:40

andyferris mentioned this pull request Apr 1, 2017

Automatic functionality a la FixedSizeArrays #114

Closed

andyferris merged commit b1714d9 into JuliaArrays:master Apr 2, 2017

andyferris reviewed Apr 2, 2017

View reviewed changes

andyferris mentioned this pull request Apr 2, 2017

WIP Add size to abstract StaticArray type #134

Merged

jrevels mentioned this pull request Apr 18, 2017

Tag a new release? #142

Closed

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.

		@@ -19,56 +19,82 @@ immutable SArray{Size, T, N, L} <: StaticArray{T, N}
		data::NTuple{L,T}

		function (::Type{SArray{Size,T,N,L}}){Size,T,N,L}(x::NTuple{L,T})

RFC: Unify SArray,SMatrix,SVector #127

RFC: Unify SArray,SMatrix,SVector #127

Uh oh!

Conversation

Keno commented Mar 30, 2017

Uh oh!

coveralls commented Mar 30, 2017

Uh oh!

andyferris 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!

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 commented Mar 30, 2017

Uh oh!

andyferris commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keno commented Mar 30, 2017

Uh oh!

andyferris commented Mar 30, 2017

Uh oh!

andyferris commented Mar 30, 2017

Uh oh!

c42f commented Mar 30, 2017

Uh oh!

andyferris commented Mar 30, 2017

Uh oh!

SimonDanisch commented Mar 30, 2017

Uh oh!

Keno commented Apr 1, 2017

Uh oh!

Keno commented Apr 1, 2017

Uh oh!

coveralls commented Apr 1, 2017

Uh oh!

coveralls commented Apr 1, 2017

Uh oh!

andyferris commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyferris commented Mar 30, 2017 •

edited

Loading