Implement S(quare) type parameter
#114
Conversation
Codecov Report
@@ Coverage Diff @@
## master #114 +/- ##
==========================================
- Coverage 96.19% 96.06% -0.14%
==========================================
Files 5 5
Lines 342 330 -12
==========================================
- Hits 329 317 -12
Misses 13 13
Help us with your feedback. Take ten seconds to tell us how you rate us. |
| @@ -5,6 +5,7 @@ for f in (:adjoint, :eigvecs, :inv, :pinv, :transpose) | |||
| end | |||
|
|
|||
| LinearAlgebra.diag(B::BlockDiagonal) = map(i -> getindex(B, i, i), 1:minimum(size(B))) | |||
| LinearAlgebra.diag(B::BlockDiagonal{T, V, true}) where {T, V} = mapreduce(diag, vcat, B.blocks) | |||
There was a problem hiding this comment.
this is the first use case
julia> b = BlockDiagonal([rand(3, 3) for _ in 1:100]);
julia> @btime diag(b); # this PR
20.859 μs (199 allocations: 137.12 KiB)
julia> @btime diag(b); # master
67.951 μs (298 allocations: 139.84 KiB)
There was a problem hiding this comment.
what are these timings showing us? is the second timing using the old version of the package?
There was a problem hiding this comment.
Yep, added the comment now.
| @@ -157,12 +158,11 @@ function _mul!(C::BlockDiagonal, A::BlockDiagonal, B::BlockDiagonal, α::Number, | |||
| return C | |||
| end | |||
|
|
|||
| function LinearAlgebra.:\(B::BlockDiagonal{T, V, false}, vm::AbstractVecOrMat) where {T, V} | |||
There was a problem hiding this comment.
this is the second use case, going from if to dispatch
| @@ -0,0 +1,4 @@ | |||
| Base.@deprecate( | |||
| BlockDiagonal{T, V}(blocks) where {T, V}, | |||
There was a problem hiding this comment.
why deprecate this rather than just computing S in this case?
There was a problem hiding this comment.
I was on the fence, but in the end it seemed simpler to have fewer constructors. If we decide to go with the suggestion on your other comment we should maybe bring this back?
| @@ -5,6 +5,7 @@ for f in (:adjoint, :eigvecs, :inv, :pinv, :transpose) | |||
| end | |||
|
|
|||
| LinearAlgebra.diag(B::BlockDiagonal) = map(i -> getindex(B, i, i), 1:minimum(size(B))) | |||
| LinearAlgebra.diag(B::BlockDiagonal{T, V, true}) where {T, V} = mapreduce(diag, vcat, B.blocks) | |||
There was a problem hiding this comment.
what are these timings showing us? is the second timing using the old version of the package?
| return new{T, V}(blocks) | ||
| function BlockDiagonal{T, V, S}(blocks::Vector{V}) where {T, V<:AbstractMatrix{T}, S} | ||
| infer_S = all(is_square.(blocks)) | ||
| S == infer_S || throw(ArgumentError("inferred S $infer_S must be equal to S $S")) |
There was a problem hiding this comment.
this defines the default constructor meaning we cannot constrcut a BlockDiagonal without having to compute all(is_square.(blocks)) every time... which seems like quite an overhead
we're going to be constructing a new BlockDiagonal quite a lot (as the output most mathematical operations)
can we instead have the default constructor expect the S arg and have a constructor that allows us passing S when we already know it (and similarly have outer-constructors that compute the S only when not provided)
There was a problem hiding this comment.
Yeah, that's a good point. I approached this defensively to prevent bugs by preserving the invariant and didn't give much consideration to efficiency.
Doing some minimal benchmarking it looks like the overhead is small, but not negligible. E.g. consider a simple operation (-) where presumably the impact of checking is the relatively the largest*.
julia> const blocks = [rand(3, 3) for _ in 1:100];
julia> const bd = BlockDiagonal(blocks);
julia> @btime -bd; # master
5.222 μs (104 allocations: 13.50 KiB)
julia> @btime -bd; # this PR
5.416 μs (106 allocations: 13.61 KiB)
julia> @btime all(BlockDiagonals.is_square.(blocks));
185.885 ns (2 allocations: 112 bytes)so the effect is about 5% slowdown. I am somewhat nervous of allowing the users to make S inconsistent with the actual state of the blocks, but since most users will just call BlockDiagonal(blocks) maybe that's ok, and allows us to speed up (keep at the same speed really) common math operations by calling the inner constructor directly. What do you think?
*The constructor alone is orders of magnitude slower of course, but that's probably not a relevant comparison since it will always be done in addition to some other operation on blocks.
Closes #107
Needed for #103 , two other use cases highlighted in the review