-
Notifications
You must be signed in to change notification settings - Fork 414
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 LKJ
#1066
Add LKJ
#1066
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1066 +/- ##
==========================================
+ Coverage 79.45% 80.62% +1.17%
==========================================
Files 112 113 +1
Lines 5514 5611 +97
==========================================
+ Hits 4381 4524 +143
+ Misses 1133 1087 -46
Continue to review full report at Codecov.
|
Hi, could you post a screenshot of the docstring of |
Do you know in which sense is this "uniform" if eta=1? |
# Section 3.2 in LKJ (2009 JMA) | ||
# 1. Initialization | ||
R = ones(typeof(η), d, d) | ||
β = η + 0.5d - 1 |
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.
Maybe
β = η + 0.5d - 1 | |
β = η + d/2 - 1 |
or should this have some oftype
?
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.
β won't be promoted to whatever η is?
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.
η = 1f0
d = 4
julia> typeof(η), typeof(η + 0.5d - 1)
(Float32, Float64)
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.
Right, thanks. Since Beta
suffers from #960 it doesn't end up mattering, unfortunately.
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.
Lets fix it anyway.
# 2. | ||
for k in 2:d - 1 | ||
# (a) | ||
β -= 0.5 |
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.
Cf 115
One could test that importance sampling is successful, e.g.
for R ~ LKJ(eta) and U ~ LKJ(1) and some functional f |
The density is constant in R. The determinant term collapses to 1 and you just have f(R; η) = c₀. |
Fair enough. I can add that. |
Yes, I can see that, but I suspect there is something to say what that distribution actually is. Or in other words, what is the reference measure for the densities? |
@mschauer It's a density with respect to Lebesgue measure, so uniform here just means the density is one over the Lebesgue measure of the support. The m = d(d-1)/2 free elements of a correlation matrix live in a subset of [-1, 1]ᵐ that has finite and strictly positive Lebesgue measure. So not some measure zero submanifold that requires a funky dominating measure to define a density. More here and here. Consequently though, the log integrating constant here is currently off by a sign. Fixing that now. |
Thank you, this is nice. What is the price we pay for the test in terms of time? |
0.21 seconds |
Thanks for all of the help on this, as usual. Let me know what else you want to see for this to be considered for merging. |
Looks great to me. @johnczito maybe add a reference in the testset from the last commit to indicate where the stan test set comes from (documentation link or something) |
Awesome, the tests were green before, so I'll just merge it here, thanks! |
@mschauer Nice! And that's a code snippet I'll definitely be stealing... |
This PR adds the LKJ distribution, which is a distribution over correlation matrices. It can be found in other statistical computing platforms (here, here, or here, for instance).
Note: One of the unit tests runs a hypothesis test, so this PR adds
HypothesisTests.jl
as an extra testing dependency in the.toml
. Let me know if I have done this incorrectly, or if I should exclude this change for now.