RFC: Allows blocks to be <:AbstractVector or <:Tuple #56
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,20 @@ | ||
| using BlockDiagonals | ||
| using Documenter | ||
|
|
||
| makedocs(; | ||
| modules=[BlockDiagonals], | ||
| format=Documenter.HTML(prettyurls=false), | ||
| pages=[ | ||
| "Home" => "index.md", | ||
| ], | ||
| repo="https://github.com/invenia/BlockDiagonals.jl/blob/{commit}{path}#{line}", | ||
| sitename="BlockDiagonals.jl", | ||
| authors="Invenia Technical Computing", | ||
| strict=true, | ||
| checkdocs=:exports, | ||
| ) | ||
|
|
||
| deploydocs(; | ||
| repo="github.com/invenia/BlockDiagonals.jl", | ||
| push_preview=true, | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,22 @@ | ||
| # Core functionality for the `BlockDiagonal` type | ||
|
|
||
| """ | ||
| BlockDiagonal{T, V} <: AbstractMatrix{T} | ||
| BlockDiagonal(blocks::V) -> BlockDiagonal{T,V} | ||
|
|
||
| A matrix with matrices on the diagonal, and zeros off the diagonal. | ||
|
|
||
| !!! 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`. | ||
| """ | ||
| struct BlockDiagonal{T, V} <: AbstractMatrix{T} | ||
| blocks::V | ||
| end | ||
|
|
||
| function BlockDiagonal(blocks::V) where { | ||
| T, V<:Union{Tuple{Vararg{<:AbstractMatrix{T}}}, AbstractVector{<:AbstractMatrix{T}}} | ||
|
There was a problem hiding this comment. Do you really want all block matrices to have the same eltype? Or could you live with them having different eltypes, and There was a problem hiding this comment.
that's a great suggestion to be honest, i hadn't considered it. This currently just keeps There was a problem hiding this comment. trying this locally, and running tests, it does cause some inference issues for perhaps someone more familiar with ChainRules has some insight on why this might be (@willtebbutt , @mzgubic)? There was a problem hiding this comment. 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 There was a problem hiding this comment. Okay, this change would be non-breaking anyway, so let's leave it as a potentially follow-up #62 There was a problem hiding this comment. 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 |
||
| } | ||
| return BlockDiagonal{T, V}(blocks) | ||
| end | ||
|
|
||
|
|
@@ -22,15 +25,15 @@ BlockDiagonal(B::BlockDiagonal) = B | |
| is_square(A::AbstractMatrix) = size(A, 1) == size(A, 2) | ||
|
|
||
| """ | ||
| blocks(B::BlockDiagonal{T, V}) -> V | ||
|
|
||
| Return the on-diagonal blocks of B. | ||
| """ | ||
| blocks(B::BlockDiagonal) = B.blocks | ||
|
|
||
| # BlockArrays-like functions | ||
| """ | ||
| blocksizes(B::BlockDiagonal{T, V}) -> V | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. my thinking is that we'd use |
||
|
|
||
| Return the size of each on-diagonal block in order. | ||
|
|
||
|
|
@@ -151,7 +154,9 @@ function _block_indices(B::BlockDiagonal, i::Integer, j::Integer) | |
| p += 1 | ||
| j -= ncols[p] | ||
| end | ||
| if !isempty(nrows[1:(p-1)]) | ||
| i -= sum(nrows[1:(p-1)]) | ||
| end | ||
| # if row `i` outside of block `p`, set `p` to place-holder value `-1` | ||
| if i <= 0 || i > nrows[p] | ||
| p = -1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| # constructor | ||
| function ChainRulesCore.rrule(::Type{<:BlockDiagonal}, blocks::V) where {V<:AbstractVector} | ||
|
There was a problem hiding this comment. i think There was a problem hiding this comment. If we exclude the tuple we won't have an There was a problem hiding this comment.
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, (it's a question about how tightly to constrain |
||
| BlockDiagonal_pullback(Δ::Composite) = (NO_FIELDS, Δ.blocks) | ||
| return BlockDiagonal(blocks), BlockDiagonal_pullback | ||
| end | ||
|
|
@@ -27,7 +27,7 @@ function ChainRulesCore.rrule( | |
| ::typeof(*), | ||
| bm::BlockDiagonal{T, V}, | ||
| v::StridedVector{T} | ||
| ) where {T<:Union{Real, Complex}, V<:Vector{Matrix{T}}} | ||
|
There was a problem hiding this comment. this okay, @willtebbutt ? |
||
|
|
||
| y = bm * v | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to constrain
Tto at least aNumber, or are we happy with anything?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think if
Tisn't Number-like, some methods will fail. But 🤷 Would there be any gain from constraining things?