RFC: Allows blocks to be <:AbstractVector or <:Tuple by nickrobinson251 · Pull Request #56 · JuliaArrays/BlockDiagonals.jl

nickrobinson251 · 2021-02-02T15:05:30Z

Closes Turn vector of matrices into a tuple? #33, supersedes Parametrize vector in BlockDiagonal #38 (as suggested by @dkarrasch at Parametrize vector in BlockDiagonal #38 (comment))
This is a breaking change, as the type param V is now different,
being the type of the blocks collection, rather than the eltype
of that collection.
This allows allows the collection to be something other than a
Vector e.g.
- a CuArray of matrices (allowing use on GPU hopefully)
- a Tuple with different concrete subtypes of AbstractMatrix{T}
  (avoiding heterogeneous block to being abstractly typed
  i.e. a way to avoid Vector{AbstractMatrix{T}})

TODO:

decide if we want this. I think we do... but i'd be interested in more feedback!
make tests nicer
- right now this just does the minimal (ugly) thing of looping over existing tests with both Tuple and Vector blocks -- we should probably refactor tests to make this nicer, and make future tests easier to add... probably in this PR if we can avoid making the diffs to big.
- open a a follow-up PR with the aesthetic changes
add tests with different collection types (e.g. CuArrays or NamedDimsArrays)
maybe add docs (doctests?) showing the different options / trade-offs for using Tuples versus Vectors?
[optional] benchmarks would be interesting (e.g. what's best when you have 20 non-homogenous blocks?) but may be best in a follow up PR (also we just want more benchmarks in general Add benchmarks #19)

codecov · 2021-02-02T15:08:02Z

Codecov Report

Merging #56 (d79d842) into master (fda7259) will decrease coverage by 3.20%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   94.04%   90.83%   -3.21%     
==========================================
  Files           5        5              
  Lines         235      240       +5     
==========================================
- Hits          221      218       -3     
- Misses         14       22       +8

Impacted Files	Coverage Δ
src/chainrules.jl	`100.00% <ø> (ø)`
src/linalg.jl	`82.85% <62.50%> (-10.80%)`	⬇️
src/blockdiagonal.jl	`82.14% <100.00%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fda7259...d5d29da. Read the comment docs.

willtebbutt · 2021-02-02T15:09:11Z

decide if we want this. I think we do... but i'd be interested in more feedback!

I for one definitely want this.

nickrobinson251 · 2021-02-02T15:14:10Z

Julia v1.0 is grumpy. I think the fix is to add cumsum(itr) to Compat.jl JuliaLang/Compat.jl#737

nickrobinson251 · 2021-02-15T21:56:03Z

+    blocks3 = [rand(rng, N1, N1), rand(rng, N2, N2), rand(rng, N2, N2)]
+
+    @testset for V in (Tuple, Vector)
+    b1 = BlockDiagonal(V(blocks1))


we shold introduce an indent under this new @testset for V in (Tuple, Vector) loop, but that'd give us huge diffs, and make it harder to see what's changed in the tests, so to make review easier i'll make that aesthetic change in a follow-up PR

willtebbutt · 2021-02-15T22:10:36Z

        bm::BlockDiagonal{T, V},
        v::StridedVector{T}
-    ) where {T<:Union{Real, Complex}, V<:Matrix{T}}
+    ) where {T<:Union{Real, Complex}, V}


We should continue to maintain strict type constraints here as this rule is not in general correct for blocks that aren't Matrix{<:Real}s (or nearly-Matrix{<:Real}s, like StridedMatrix{<:Real}s). Something like

V<:Vector{<:Matrix{<:Real}}

would actually be a good choice to my mind. See e.g. JuliaDiff/ChainRules.jl#232

love it. makes this PR much easier 😂

- This is a breaking change, as the type param `V` is now different, being the type of the `blocks` collection, rather than the `eltype` of that collection. - This allows allows the collection to be something other than a `Vector` e.g. - a `CuArray` of matrices (allowing use on GPU hopefully) - a `Tuple` with different concrete subtypes of `AbstractMatrix{T}` (avoiding heterogeneous block to being abstractly typed i.e. a way to avoid `Vector{AbstractMatrix{T}}`)

nickrobinson251 · 2021-02-15T22:52:28Z

@@ -1,5 +1,5 @@
 # constructor
-function ChainRulesCore.rrule(::Type{<:BlockDiagonal}, blocks::Vector{V}) where {V}
+function ChainRulesCore.rrule(::Type{<:BlockDiagonal}, blocks::V) where {V<:AbstractVector}


i think AbstractVector is fine here, but i'll rely on @willtebbutt to tell me if not

If we exclude the tuple we won't have an rrule for constructors of BlockDiagonals with tuple blocks. Are you asking if it's ok not to have rrule for tuple blocks, or am I missing the point of the question?

we won't have an rrule for constructors of BlockDiagonals with tuple blocks

correct, that'd be a nice feature to add as a follow-up, but out-of-scope for this PR

But my question is a different one. Before this PR, blocks were always a Vector, so we had rrule(::Type{<:BlockDiagonal}, blocks::Vector) whereas this PR allows them to be any AbstractVector (or Tuple but ignore that, since we're not adding that rrule here). So my question is, now that blocks can be any AbstractVector, are we okay with broadening the rrule from rrule(::Type{<:BlockDiagonal}, blocks::Vector) to rrule(::Type{<:BlockDiagonal}, blocks::AbstractVector) ?

(it's a question about how tightly to constrain rrules #56 (comment))

nickrobinson251 · 2021-02-15T22:52:41Z

        bm::BlockDiagonal{T, V},
        v::StridedVector{T}
-    ) where {T<:Union{Real, Complex}, V<:Matrix{T}}
+    ) where {T<:Union{Real, Complex}, V<:Vector{Matrix{T}}}


this okay, @willtebbutt ?

mzgubic · 2021-02-16T09:36:31Z

-    end
+!!! info "`V` type"
+    `blocks::V` should be a `Tuple` or `AbstractVector` where each component (each block) is
+    `<:AbstractMatrix{T}` for some common element type `T`.


do we want to constrain T to at least a Number, or are we happy with anything?

i think if T isn't Number-like, some methods will fail. But 🤷 Would there be any gain from constraining things?

mzgubic · 2021-02-16T09:43:19Z

 # BlockArrays-like functions
 """
-    blocksizes(B::BlockDiagonal) -> Vector{Tuple}
+    blocksizes(B::BlockDiagonal{T, V}) -> V


Do we want this to return a Vector or a Tuple depending on the block type? Or should we force it to return a vector/tuple always?

my thinking is that we'd use blocks::Tuple for performance reasons when you've a small number of heterogenous blocks, in which case getting small Tuples from methods that operate on blocks will also be slightly better for performance, so no reason to force a Vector to be returned. It doesn't make sense to force a Tuple return in general, since there could be many elements.

mzgubic · 2021-02-16T09:49:00Z

@@ -1,5 +1,5 @@
 # constructor
-function ChainRulesCore.rrule(::Type{<:BlockDiagonal}, blocks::Vector{V}) where {V}
+function ChainRulesCore.rrule(::Type{<:BlockDiagonal}, blocks::V) where {V<:AbstractVector}


If we exclude the tuple we won't have an rrule for constructors of BlockDiagonals with tuple blocks. Are you asking if it's ok not to have rrule for tuple blocks, or am I missing the point of the question?

mzgubic · 2021-02-16T09:53:18Z

+    U_Vt = ntuple(length(blocks(B))) do i
+        F = svd(getblock(B, i), full=full)
+        append!(S, F.S)
+        (F.U, F.Vt)


How come we aren't push!ing like above? Or vice versa if first.()/last.() is better?

because above we want U (likewise Vt) to be a Vector, so create a Vector and add the elements. Whereas here we want it to be a Tuple hence ntuple, and first.(ntup) will still give us a tuple.

rather than push! we should probably be preallocating a Vector of the right length and overwriting the values, but the two functions would still be different (since Tuples are immutable)

dkarrasch · 2021-02-16T10:34:53Z


-function BlockDiagonal(blocks::Vector{V}) where {T, V<:AbstractMatrix{T}}
+function BlockDiagonal(blocks::V) where {
+    T, V<:Union{Tuple{Vararg{<:AbstractMatrix{T}}}, AbstractVector{<:AbstractMatrix{T}}}


Do you really want all block matrices to have the same eltype? Or could you live with them having different eltypes, and T being promote_type(map(eltype, blocks)...)? See LinearMaps.jl. If you need them to be homogeneous, then you may wish to include some promotion mechanism.

could you live with them having different eltypes, and T being promote_type(map(eltype, blocks)...)?

that's a great suggestion

to be honest, i hadn't considered it. This currently just keeps T with the same meaning as on master.
Are there any downsides to T being promote_type(map(eltype, blocks)...)?

trying this locally, and running tests, it does cause some inference issues for rrule

BlockDiagonal * Vector: Error During Test at /Users/npr/repos/BlockDiagonals.jl/test/chainrules.jl:12 Got exception outside of a @test return type Array{Float64,1} does not match inferred return type Any Stacktrace: [1] error(::String) at ./error.jl:33 [2] _test_inferred(::Function, ::InplaceableThunk{Thunk{BlockDiagonals.var"#13#19"{Array{Float64,1},BlockDiagonal{Float64,Array{Array{Float64,2},1}}}},BlockDiagonals.var"#14#20"{Array{Float64,1},BlockDiagonal{Float64,Array{Array{Float64,2},1}}}}; kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /Users/npr/.julia/packages/ChainRulesTestUtils/2QRer/src/testers.jl:228 [3] _test_inferred(::Function, ::InplaceableThunk{Thunk{BlockDiagonals.var"#13#19"{Array{Float64,1},BlockDiagonal{Float64,Array{Array{Float64,2},1}}}},BlockDiagonals.var"#14#20"{Array{Float64,1},BlockDiagonal{Float64,Array{Array{Float64,2},1}}}}) at /Users/npr/.julia/packages/ChainRulesTestUtils/2QRer/src/testers.jl:227 [4] test_rrule(::typeof(*), ::BlockDiagonal{Float64,Array{Array{Float64,2},1}}, ::Vararg{Any,N} where N; output_tangent::ChainRulesTestUtils.Auto, fdm::FiniteDifferences.AdaptedFiniteDifferenceMethod{5,1,FiniteDifferences.UnadaptedFiniteDifferenceMethod{7,5}}, check_inferred::Bool, fkwargs::NamedTuple{(),Tuple{}}, rtol::Float64, atol::Float64, kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /Users/npr/.julia/packages/ChainRulesTestUtils/2QRer/src/testers.jl:187 [5] test_rrule(::Function, ::BlockDiagonal{Float64,Array{Array{Float64,2},1}}, ::Vararg{Any,N} where N) at /Users/npr/.julia/packages/ChainRulesTestUtils/2QRer/src/testers.jl:153 [6] top-level scope at /Users/npr/repos/BlockDiagonals.jl/test/chainrules.jl:15

perhaps someone more familiar with ChainRules has some insight on why this might be (@willtebbutt , @mzgubic)?

I don't know why this is (I assume the answer contains the word compiler). But I'd rather live with the same eltype than with this I think

Okay, this change would be non-breaking anyway, so let's leave it as a potentially follow-up #62

I think that's perfectly fine. But then you may wish to consider to include a constructor that by itself does not yet require equal eltypes, but first determines T via promotion and then map(B -> convert(AbstractMatrix{T}, B), blocks). This should be a no-op if all matrices have the same eltype, but otherwise relieve you from annoying method errors. Just a suggestion.

nickrobinson251 requested a review from willtebbutt February 2, 2021 15:05

nickrobinson251 commented Feb 2, 2021

View reviewed changes

Comment thread test/linalg.jl Outdated

nickrobinson251 force-pushed the npr/allow-tuple-or-any-vec branch 2 times, most recently from 5477835 to 70ddc17 Compare February 15, 2021 21:53

nickrobinson251 commented Feb 15, 2021

View reviewed changes

nickrobinson251 requested a review from mzgubic February 15, 2021 21:57

willtebbutt reviewed Feb 15, 2021

View reviewed changes

nickrobinson251 added 4 commits February 15, 2021 22:49

Simplify test setup for base_maths.jl

32f9d07

Simplify test setup for linalg.jl

8b3235c

Improve tests in blockdiagonal.jl

c9bf915

nickrobinson251 force-pushed the npr/allow-tuple-or-any-vec branch from 70ddc17 to c9bf915 Compare February 15, 2021 22:49

nickrobinson251 commented Feb 15, 2021

View reviewed changes

Update docs

d5d29da

nickrobinson251 force-pushed the npr/allow-tuple-or-any-vec branch from 9c0b6d3 to d5d29da Compare February 15, 2021 23:14

mzgubic reviewed Feb 16, 2021

View reviewed changes

dkarrasch reviewed Feb 16, 2021

View reviewed changes

nickrobinson251 mentioned this pull request Feb 17, 2021

Support having blocks with different eltypes #62

Open

nickrobinson251 closed this Oct 2, 2022

Conversation

nickrobinson251 commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

willtebbutt commented Feb 2, 2021

Uh oh!

nickrobinson251 commented Feb 2, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

nickrobinson251 Feb 16, 2021 • 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!

nickrobinson251 Feb 16, 2021 • 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!

nickrobinson251 Feb 16, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nickrobinson251 commented Feb 2, 2021 •

edited

Loading

codecov Bot commented Feb 2, 2021 •

edited

Loading

nickrobinson251 Feb 16, 2021 •

edited

Loading

nickrobinson251 Feb 16, 2021 •

edited

Loading

nickrobinson251 Feb 16, 2021 •

edited

Loading