Constrain type in `to_vec(::AbstractArray/Vector)` #156

mzgubic · 2021-04-20T18:10:56Z

I've looked into whether we can simply constrain the to_vec generic AbstractVector/Array to Strided, and it almost works.

It's just these ChainRules tests which need the SubArray in the Union (I didn't investigate why the SubArrays inside the Strided Union do not contain this case):
https://github.com/JuliaDiff/ChainRules.jl/blob/38caf4bfdb8af616fcbe7626d10699608af21904/test/rulesets/Base/arraymath.jl#L36-L42

@willtebbutt this seems to solve a lot of issues that we were seeing (#149, #141).

Is there a reason we haven't changed this before? I guess partly because it is breaking, but are there any bad things that can happen if we go ahead with this? It seems like using the struct fallback is a better choice for most things that subtype AbstractArrays?

willtebbutt · 2021-04-20T18:30:41Z

Is there a reason we haven't changed this before? I guess partly because it is breaking, but are there any bad things that can happen if we go ahead with this? It seems like using the struct fallback is a better choice for most things that subtype AbstractArrays?

I think the answer is just that no one has taken the time to fix it -- I had personally imagined that it would be annoying to have to modify a bunch of existing code in ChainRules, but if that's not the case, then great.

Regarding bad things happening -- there might be a couple of surprised users, and perhaps the occassional issue asking why an AbstractArray tangent isn't being generated, but that's about it.

willtebbutt · 2021-04-21T15:03:49Z

src/to_vec.jl

@@ -53,7 +53,7 @@ function to_vec(x::StridedVector)
    return x_vec, Vector_from_vec
 end

-function to_vec(x::Union{SubArray, StridedArray})
+function to_vec(x::Union{SubArray, Base.ReshapedArray, StridedArray})


Maybe the parent array of SubArray should be restricted to e.g. StridedArray or something?

We want e.g. a

julia> sa = @view randn(T, 10)[1:4] julia> typeof(sa) SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true}

to dispatch on the current SubArray implementation. Unfortunately StridedArray is a Union of several more specific SubArrays (not just in the parent argument)

So we have two options:

Narrow the dispatch of the current SubArray (very verbose*)

Loosen the dispatch for StridedArray to Union{StridedArray, SubArray} so that the current SubArray becomes more specific.

And similarly for the ReshapedArray.

I don't particularly like either option but it's probably better than the status quo.

*Number 1) looks like this, I want to nope out of that 😂

StridedSubArray = Union{ SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real, N} where N}, Tuple{AbstractUnitRange, Vararg{Any, N} where N}}}, DenseArray}, MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}, N} where N}}, SubArray{T, 1, A, I, L} where {A<:Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {T, N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real, N} where N}, Tuple{AbstractUnitRange, Vararg{Any, N} where N}}}, DenseArray}, IsReshaped, S}, SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real, N} where N}, Tuple{AbstractUnitRange, Vararg{Any, N} where N}}}, DenseArray}, MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}, N} where N}}, DenseArray}, I<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}, Base.AbstractCartesianIndex, Base.ReshapedArray{T, N, A, Tuple{}} where {T, N, A<:AbstractUnitRange}}, N} where N}, L}} where T) }

@willtebbutt how do you feel about that?

The thing I'm concerned about is if someone attempts to to_vec something like

view(Diagonal(randn(5)), 1:5, 1:3)

*Number 1) looks like this, I want to nope out of that 😂

Lol yeah, that's really not fun.

Is there a reason that we can't go with something like

Union{StridedArray, SubArray{T, D, <:StridedArray} where {T, D}}

and just recurse into ReshapedArrays?

The dispatch for the Diagonal example is the same (the dedicated SubArray method) whether we constrain the parent of the SubArray to be strided or not.

The example breaks, but because SparseArray.SparseMatrixCSC does not have a to_vec defined.

The status is:

All tests pass with the current PR

Removing the to_vec(Base.SubArray) breaks some tests

test_to_vec(view(Diagonal(randn(5)), 1:5, 1:3); check_inferred=false), (the thing you suggested should be checked) breaks on master (StackOverflowError), on the current PR (SparseMatrixCSC error), and on the current PR with SubArray removed from the Union (SparseMatrixCSC error).

Ahh I see.

I've run it further locally, and found that the following seems to resolve everything for me:

restrict the array methods to DenseVector and DenseArray, from Union{StridedArray...}

add a specific method for SubArray:

function to_vec(x::SubArray) x_vec, from_vec = to_vec(x.parent) SubArray_from_vec(x_vec) = view(from_vec(x_vec), x.indices...) return x_vec, SubArray_from_vec end

if you don't have this, it thinks that the integer fields should also be to_veced.
3. comment out the SubArray-specific method

Oh yeah that's much better. Are we happy with that?

I'm happy with it, but I feel that we should get an additional review. Maybe @oxinabox or @sethaxen ?

Actually with this one ChainRules test fails, and the reason is that the current implementation returns the whole parent vector

julia> a = rand(7); julia> b = @view a[2:5]; julia> v, back = to_vec(b); julia> v 7-element Vector{Float64}: 0.9586867155061674 0.8025760289977295 0.9784238101274141 0.16440182965461236 0.17784328126321847 0.01704930118685577 0.8408143603848348

Keeping the old version below works just fine. No issues now StridedArray has been replaced by DenseArray (where we used to have AbstractArray)

to_vec(x::SubArray) = to_vec(copy(x))

willtebbutt · 2021-04-28T13:05:37Z

This is looking pretty good now I think. Do we need to bump the minor version, or would a patch bump suffice?

mzgubic · 2021-04-28T13:07:49Z

I've made sure ChainRules tests are fine but technically it is still a breaking change, no?

willtebbutt · 2021-04-28T13:18:54Z

I've made sure ChainRules tests are fine but technically it is still a breaking change, no?

Im not sure which bit is really breaking. Could you elaborate?

mzgubic · 2021-04-28T13:25:26Z

julia> subtypes(AbstractArray)
31-element Vector{Any}:
 AbstractRange
 Base.LogicalIndex
 Base.ReinterpretArray
 Base.ReshapedArray
 Base.SCartesianIndices2
 BitArray
 CartesianIndices
 Core.Compiler.AbstractRange
 Core.Compiler.BitArray
 Core.Compiler.LinearIndices
 DenseArray
 LinearAlgebra.AbstractQ
 LinearAlgebra.AbstractTriangular
 LinearAlgebra.Adjoint
 LinearAlgebra.Bidiagonal
 LinearAlgebra.Diagonal
 LinearAlgebra.Hermitian
 LinearAlgebra.LQPackedQ
 LinearAlgebra.SymTridiagonal
 LinearAlgebra.Symmetric
 LinearAlgebra.Transpose
 LinearAlgebra.Tridiagonal
 LinearAlgebra.UpperHessenberg
 LinearIndices
 PermutedDimsArray
 SparseArrays.AbstractSparseArray
 StaticArrays.StaticArray
 StaticArrays.TrivialView
 SubArray
 SuiteSparse.CHOLMOD.FactorComponent
 Test.GenericArray

I guess unless a special to_vec is defined for these types (as well as any subtypes of AbstractArray in other packages, e.g. BlockDiagonal) they will now change to the fallback method. I suppose this is breaking though it could also be interpreted as a bugfix since they mostly broke before anyway?

willtebbutt · 2021-04-28T13:57:26Z

I suppose this is breaking though it could also be interpreted as a bugfix since they mostly broke before anyway?

This is my view -- if anyone opens an issue indicating that we broke their code, I'm more than happy to add a to_vec for the array type that broke.

A third opinion would be great though.

oxinabox · 2021-04-28T14:28:32Z

test/to_vec.jl

+        test_to_vec(SVector{2, T}(1.0, 2.0); check_inferred = false)
+        test_to_vec(SMatrix{2, 2, T}(1.0, 2.0, 3.0, 4.0); check_inferred = false)


Suggested change

test_to_vec(SVector{2, T}(1.0, 2.0); check_inferred = false)

test_to_vec(SMatrix{2, 2, T}(1.0, 2.0, 3.0, 4.0); check_inferred = false)

test_to_vec(SVector{2, T}(1.0, 2.0); check_inferred=false)

test_to_vec(SMatrix{2, 2, T}(1.0, 2.0, 3.0, 4.0); check_inferred=false)

Bluetyle spacing in kwargs

changed in other places as well now

oxinabox · 2021-04-28T14:31:19Z

I think we need a test for a custom wrapper array, something like is like NamedDimsArray.
but without the names.
Probably

struct WrapperArray{T<:AbstractArray} <: AbstractArray
    data::T
end
# minimal methods that just delegate

and make sure that still acts in a way we are happy with.

oxinabox

I think we can say this is a patch

Miha Zgubic added 2 commits April 20, 2021 18:56

make strided

b711591

bump version

7795bd6

mzgubic changed the title ~~Constrain type in to_vec(::AbstractArray/Vector)~~ WIP Constrain type in to_vec(::AbstractArray/Vector) Apr 20, 2021

Miha Zgubic added 4 commits April 21, 2021 14:32

FillVector should work

404ef6b

rename local variable vals to values

c29d71b

remove inference checks

9d5bd94

Unions because dispatch is weird

b4d0c03

willtebbutt reviewed Apr 21, 2021

View reviewed changes

mzgubic changed the title ~~WIP Constrain type in to_vec(::AbstractArray/Vector)~~ Constrain type in to_vec(::AbstractArray/Vector) Apr 23, 2021

Miha Zgubic added 3 commits April 28, 2021 11:03

move from StridedArray to DenseArray

0400793

use old SubArray implementation

3122d89

use old version

c94df20

oxinabox reviewed Apr 28, 2021

View reviewed changes

Miha Zgubic added 2 commits April 28, 2021 15:45

add a wrapperarray test

023f262

blue style kwargs spacing

a326978

oxinabox approved these changes Apr 28, 2021

View reviewed changes

Miha Zgubic added 2 commits April 28, 2021 16:03

patch bump

6887b08

move wrapper outside of the testset

1d05438

mzgubic merged commit 5935f4a into master Apr 28, 2021

mzgubic deleted the mz/to_vec branch April 28, 2021 15:57

This was referenced Apr 30, 2021

Remove redundant to_vec method JuliaArrays/BlockDiagonals.jl#69

Merged

infinite recursion of to_vec for wrapped arrays #141

Open

nickrobinson251 mentioned this pull request Jun 21, 2021

Jacobian shape error with FillArrays.Zeros vector #177

Closed

Kolaru mentioned this pull request Jul 13, 2021

Implement to_vec for matrix factorizations #128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constrain type in `to_vec(::AbstractArray/Vector)` #156

Constrain type in `to_vec(::AbstractArray/Vector)` #156

mzgubic commented Apr 20, 2021

willtebbutt commented Apr 20, 2021

willtebbutt Apr 21, 2021

mzgubic Apr 22, 2021

mzgubic Apr 23, 2021

willtebbutt Apr 23, 2021 •

edited

Loading

mzgubic Apr 23, 2021

mzgubic Apr 26, 2021 •

edited

Loading

willtebbutt Apr 27, 2021

mzgubic Apr 28, 2021

willtebbutt Apr 28, 2021

mzgubic Apr 28, 2021

willtebbutt commented Apr 28, 2021

mzgubic commented Apr 28, 2021

willtebbutt commented Apr 28, 2021

mzgubic commented Apr 28, 2021

willtebbutt commented Apr 28, 2021

oxinabox Apr 28, 2021

mzgubic Apr 28, 2021

oxinabox commented Apr 28, 2021

oxinabox left a comment

		test_to_vec(SVector{2, T}(1.0, 2.0); check_inferred = false)
		test_to_vec(SMatrix{2, 2, T}(1.0, 2.0, 3.0, 4.0); check_inferred = false)

Constrain type in to_vec(::AbstractArray/Vector) #156

Constrain type in to_vec(::AbstractArray/Vector) #156

Conversation

mzgubic commented Apr 20, 2021

willtebbutt commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willtebbutt Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzgubic Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willtebbutt commented Apr 28, 2021

mzgubic commented Apr 28, 2021

willtebbutt commented Apr 28, 2021

mzgubic commented Apr 28, 2021

willtebbutt commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox commented Apr 28, 2021

oxinabox left a comment

Choose a reason for hiding this comment

Constrain type in `to_vec(::AbstractArray/Vector)` #156

Constrain type in `to_vec(::AbstractArray/Vector)` #156

willtebbutt Apr 23, 2021 •

edited

Loading

mzgubic Apr 26, 2021 •

edited

Loading