Conversion of SizedArray to Array reshapes

[mateuszbaran]

I'm trying to port some improvements I've made in HybridArrays to SizedArray here. Full support for wrapping arbitrary AbstractArrays is a fairly large project and noticeable increase in complexity of SizedArray so I'm not sure if it's actually a good idea.

Anyway, here is a small change that:

1. makes sure that converting back to Array retains given static shape,
2. changes Array(::SizedArray) so that it makes a copy (since Array(::Array) also makes one).

[coveralls]

Coverage increased (+0.02%) to 81.607% when pulling 602a97d on mateuszbaran:mbaran/array-convert-reshape into 1c53f4a on JuliaArrays:master.

[c42f]

I do like the constructor changes because they increase consistency with Base (and also with MArray). I this is just a bug we are correcting.

I'm less sure about convert; it looks like Base doesn't necessarily make a copy here, so we would be justified in only unwrapping. But the fact that SizedArray is a weird "wrap plus reshape" container (rather than just a wrapper which preserves shape) is kind of unfortunate and forces us to return a reshape for consistency.

Hmm. Tentatively I think this might be fine to merge as it fixes some obscure bugs and removes what I think is unintended functionality. On the other hand it's technically a breaking change and is a bit of a compatibility trap without a clear way forward for a deprecation warning.

It's possible to get some idea of the usage of SizedArray by using the code search for pkg.julialang.org: https://pkg.julialang.org/docs/search?q=%5CbSizedArray%5Cb (be sure to click on the "code" tab). From the look of this, there's really not a lot of people using SizedArray systematically. But there's also the fact that Size can be used as a constructor which I think might be a misfeature (even if convenient). It's much harder to get a feel for that without a type-inference-aware code search, though https://pkg.julialang.org/docs/search?q=%5CbSize%5C%28%5B%5E%29%5D%2a%5C%29%5C%28 could be an approximation.

[c42f]

It seems that convert doesn't copy in Base:

julia> a = [1 2]
1×2 Array{Int64,2}:
 1  2

julia> b = convert(Array, a)
1×2 Array{Int64,2}:
 1  2

julia> b[1] = 0
0

julia> a
1×2 Array{Int64,2}:
 0  2

[mateuszbaran]

I'm less sure about convert; it looks like Base doesn't necessarily make a copy here, so we would be justified in only unwrapping. But the fact that SizedArray is a weird "wrap plus reshape" container (rather than just a wrapper which preserves shape) is kind of unfortunate and forces us to return a reshape for consistency.

reshape doesn't copy when there is no need to actually reshape so doing reshape + convert in convert should be reasonable. I'll make a change.

But the fact that SizedArray is a weird "wrap plus reshape" container (rather than just a wrapper which preserves shape) is kind of unfortunate and forces us to return a reshape for consistency.

I'm actually sort of using this feature in Manifolds.jl (through SizedAbstractArray) for reshaped views. It will be replaced with SSubArray from HybridArrays.jl when I get to it though. When I was playing with this it didn't really work as generally as StaticArrays.jl seems to allow. Essentially undef initialization appears impossible (or unreasonable) unless M=N or M=1 (and IIRC at the moment only the M=N case works in StaticArrays). I really think all other cases should be disallowed.

[c42f]

Ok, rather than just letting users discover the shape change "by surprise", how about we check whether size(sa.data) != size_to_tuple(S), and if so, emit a depwarn(...). Keep this around for one release and then remove it.

It's not a perfect solution (technically not a "full deprecation cycle") but might be ok given that this is relatively obscure functionality.

[mateuszbaran]

OK, that sounds reasonable. I doubt anyone actually uses this "functionality" intentionally.

[c42f]

Should copy though? Also don't need the where {S} now.

[mateuszbaran]

OK, I'll keep this change then. It should copy and Array{T} should also change the element type, right? But not reshape for now.

[mateuszbaran]

I forgot that these constructors don't change element type, nevermind.

I've changed them so that they make copies. Is this the right set of changes for the next version of StaticArrays.jl?

[c42f]

should also change the element type, right

Hmm, good question actually. I think we'll fail to do the right thing for convert, because for non-matching eltype we will dispatch to the fallback convert(::Array, ::StaticArray)? I guess this has been a bug for a while.

[c42f]

This is a subtle and tricky deprecation. I'd appreciate any thoughts from @andyferris who originally implemented SizedArray.

Andy — to summarize, the way that SizedArray unwraps when converting back to Array is kind of inconsistent. It doesn't preserve the shape for example (the constructors also don't create a copy which is also inconsistent).

So... what to do. One way forward (currently implemented here) is to remove the arbitrary reshape support from SizedArray so that unwrapping is more straightforward. I think this would clarify the design in a nice way but it would remove some functionality.

(Side note: I just noticed that our reshape for MArray is arguably broken because it doesn't share the underlying data. Ugh.)

[andyferris]

I'm totally OK with ditching the ability to reshape an array. That was actually an over-ambitious attempt to step towards hybrid arrays at a time where I was also worried about reshaping and/or nesting things. I probably should have deleted it a while ago...