search

invenia / PDMatsExtras.jl

Extra Positive (Semi-)Definite Matricies
MIT License
8 stars 6 forks source link

Ar/whiten #32

Closed AlexRobson closed 2 years ago

AlexRobson commented 2 years ago

Adds whiten for WoodburyPDMat.

It doesn't do anything special other than densifies and calls PDMats unwhiten. Without this, we'll hit a cholesky error as PDMats tries to call cholesky on the WoodburyPDMat. 

julia> whiten(W, x)
ERROR: MethodError: no method matching cholesky(::WoodburyPDMat{Float64, Matrix{Float64}, Diagonal{Float64, Vector{Float64}}, Diagonal{Float64, Vector{Float64}}})
codecov[bot] commented 2 years ago

Codecov Report

Merging #32 (766743e) into master (e6506b0) will increase coverage by 0.68%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   73.45%   74.13%   +0.68%     
==========================================
  Files           4        4              
  Lines         113      116       +3     
==========================================
+ Hits           83       86       +3     
  Misses         30       30
Impacted Files Coverage Δ
src/woodbury_pd_mat.jl 93.87% <100.00%> (+0.39%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us.

wil-j-wil commented 2 years ago

This looks good. Should we add unwhiten while we're at it?

AlexRobson commented 2 years ago

This looks good. Should we add unwhiten while we're at it?

unwhiten is actually already implemented :) - If I recall correctly somewhere one of the typical operations we do (e.g. log pdf) calls it, can't remember which though.

https://github.com/invenia/PDMatsExtras.jl/blob/master/src/woodbury_pd_mat.jl#L104

as an aside - unwhiten is basically the inverse of this, and there the matrix is also densifies. The commentary there around densifying applies as much here as there in that we are densifying to get the root.