Fix for #3270 #3293
Fix for #3270 #3293
Conversation
…ov.random method.
| @@ -998,6 +1013,90 @@ def logp(self, x): | |||
|
|
|||
| return norm + logp_lkj + logp_sd + det_invjac | |||
|
|
|||
| def _random(self, n, eta, size=1): | |||
| eta_sample_shape = (size,) + eta.shape | |||
There was a problem hiding this comment.
Since there are large overlap with the LKJCorr random implementation, we should isolate the function outside of the class so both LKJCorr and LKJCorrCholesky can call it.
There was a problem hiding this comment.
Ok. I honestly just copied LKJCorr.random and took the cholesky decomposition at the end because I don't know if there is an algorithm that efficiently samples the cholesky decomposition straight away.
One other thing that I wanted to ask some help with was writing a test like lkj's distribution random, but for the cholesky decomposition with the sd_dist.
There was a problem hiding this comment.
Right, I need to double check the paper but if I remember correctly, LKJCorr random generate a cholesky factor for the correlation matrix, so you just need to add the diagonal.
There was a problem hiding this comment.
@lucianopaz @twiecki Yes the current implementation generate a Cholesky factor (P in the random generation code), we can then use the LDL decomposition https://en.wikipedia.org/wiki/Cholesky_decomposition#LDL_decomposition to generate the LKJCorrCholesky.
…at was introduced in this PR.
|
Needs a rebase. |
pymc3/distributions/distribution.py
Outdated
| @@ -245,6 +246,23 @@ def parent(self): | |||
| return self._parent | |||
|
|
|||
|
|
|||
| class _DrawValuesContextBlocker(six.with_metaclass(InitContextMeta, | |||
|
Thanks @lucianopaz ! |
Thanks! However, this PR still had some rough edges. In particular, I still had not implemented tests for |
|
@lucianopaz OK, sounds good. Let's apply the WIP label in the future. |
close #3246 and close #3270
It was quite hard to get the mixture class
randommethod to work. To finish the fix for #3270 I also had to write a random method for theLKJCholeskyCovclass. It was quite a cumbersome fix and I still need to add new tests to the testsuite.