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

Add LKJCholesky #1339

Merged
merged 82 commits into from
Oct 15, 2021
Merged

Add LKJCholesky #1339

merged 82 commits into from
Oct 15, 2021

Conversation

sethaxen
Copy link
Contributor

@sethaxen sethaxen commented Jun 6, 2021

This PR adds an LKJCholesky distribution whose support are LinearAlgebra.Cholesky factorizations of correlation matrices, as discussed in #1336. Essentially all of the functionality is there; I just need to add tests.

@sethaxen
Copy link
Contributor Author

sethaxen commented Jun 7, 2021

In the current design, LKJCholesky has an uplo parameter that is only used to determine the triangle storage of sampled Cholesky objects (specified in the uplo field of Cholesky). If insupport, logkernel, etc are called on Cholesky with a different uplo, the distribution's uplo is ignored. This is analogous to logpdf for a matrix distribution working correctly when passed a structured matrix even if the random sample might be unstructured.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Great 👍

I think it would be good to remove the type computations whenever possible. It seems often one could just use sum. If one wants to ensure type stability often a better approach is to perform as many computations as possible and only fall back to e.g. oftype(result, Inf) if necessary in the end.

src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/matrix/lkj.jl Outdated Show resolved Hide resolved
src/matrix/lkj.jl Outdated Show resolved Hide resolved
src/matrix/lkj.jl Outdated Show resolved Hide resolved
src/matrix/lkj.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2021

Codecov Report

Merging #1339 (6ca4ecc) into master (42af709) will increase coverage by 0.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1339      +/-   ##
==========================================
+ Coverage   83.05%   83.50%   +0.45%     
==========================================
  Files         117      118       +1     
  Lines        6756     6864     +108     
==========================================
+ Hits         5611     5732     +121     
+ Misses       1145     1132      -13     
Impacted Files Coverage Δ
src/common.jl 66.66% <ø> (ø)
src/matrix/lkj.jl 99.13% <ø> (ø)
src/cholesky/lkjcholesky.jl 100.00% <100.00%> (ø)
src/show.jl 97.50% <0.00%> (+2.50%) ⬆️
src/univariate/continuous/ksdist.jl 72.72% <0.00%> (+2.59%) ⬆️
src/univariate/continuous/ksonesided.jl 76.92% <0.00%> (+76.92%) ⬆️

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 42af709...6ca4ecc. Read the comment docs.

src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Show resolved Hide resolved
src/cholesky/lkjcholesky.jl Outdated Show resolved Hide resolved
src/matrix/lkj.jl Outdated Show resolved Hide resolved
@sethaxen
Copy link
Contributor Author

Alright, I think I've addressed all the comments.

docs/src/cholesky.md Outdated Show resolved Hide resolved
test/lkjcholesky.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

Seems everyone is happy with the PR 🙂 Thanks a lot, @sethaxen!

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.

6 participants