Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use un-pivoted Cholesky triangle to sample from sparse MvNormalCanon #1218

Merged
merged 4 commits into from
Nov 19, 2020

Conversation

ElOceanografo
Copy link
Contributor

Fixes #1200.

Note I also rewrote the tests--the SuiteSparse Cholesky decomposition returns a different decomposition than the Base.LinearAlgebra function (since the Cholesky factorization is not in general unique). As a result, the random samples from an MvNormalCanon distribution with a sparse precision matrix are not in general identical to those from an MvNormalCanon or MvNormal, even if the seeds are identical. As a result, these tests only check for approximate statistical equality, rather than strict numerical equality of the samples. Not sure if there's a better way to handle that.

@codecov-io
Copy link

codecov-io commented Nov 10, 2020

Codecov Report

Merging #1218 (26a53cb) into master (ae2d6c5) will decrease coverage by 1.81%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
- Coverage   81.79%   79.97%   -1.82%     
==========================================
  Files         116      116              
  Lines        6558     5963     -595     
==========================================
- Hits         5364     4769     -595     
  Misses       1194     1194              
Impacted Files Coverage Δ
src/multivariate/mvnormalcanon.jl 80.39% <100.00%> (-2.07%) ⬇️
src/univariate/discrete/hypergeometric.jl 59.45% <0.00%> (-3.96%) ⬇️
src/univariate/continuous/biweight.jl 56.66% <0.00%> (-3.94%) ⬇️
src/multivariate/mvtdist.jl 57.30% <0.00%> (-3.93%) ⬇️
src/matrix/wishart.jl 84.04% <0.00%> (-3.81%) ⬇️
src/univariate/continuous/epanechnikov.jl 54.54% <0.00%> (-3.79%) ⬇️
src/univariate/continuous/pgeneralizedgaussian.jl 58.06% <0.00%> (-3.71%) ⬇️
src/univariate/continuous/triweight.jl 55.88% <0.00%> (-3.58%) ⬇️
src/univariate/continuous/beta.jl 61.79% <0.00%> (-3.51%) ⬇️
src/multivariates.jl 78.46% <0.00%> (-3.12%) ⬇️
... and 92 more

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 ae2d6c5...26a53cb. Read the comment docs.

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for a small issue when using the non-GPL version of Julia

test/mvnormal.jl Outdated Show resolved Hide resolved
@ElOceanografo
Copy link
Contributor Author

Bump...@andreasnoack, does this need anything else before it can merge?

@andreasnoack
Copy link
Member

No. Looks good.

@andreasnoack andreasnoack merged commit f643520 into JuliaStats:master Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDMats doesn't account for pivoting in Cholesky of sparse matrices
3 participants