prior predictive mvnornal, use cholesky-based random sampling #3766
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3766 +/- ##
==========================================
+ Coverage 90.63% 90.64% +0.01%
==========================================
Files 133 133
Lines 20328 20318 -10
==========================================
- Hits 18424 18418 -6
+ Misses 1904 1900 -4
|
There was a problem hiding this comment.
It was really hard to find the actual changes in the middle of all the style things that black did. However, from what I saw, this doesn't seem to address the problems when chol is passed to MvNormal. At least a test for both parameterizations should be added.
One other thing is that multivariate distributions are not well implemented for sample_prior_predictive and shape issues will pop out when you start making the distribution parameters' RVs themselves. With this change, you avoid problems when mu is an RV but once we make cov an RV, things will blow up. What's worse is that any exceptions raised there will be silenced and an array full of nans will be returned.
pymc3/distributions/multivariate.py
Outdated
| mu, cov = draw_values([self.mu, self.cov], point=point, size=size) | ||
| if mu.shape[-1] != cov.shape[-1]: | ||
| raise ValueError("Shapes for mu and cov don't match") | ||
|
|
||
| try: | ||
| dist = stats.multivariate_normal( | ||
| mean=mu, cov=cov, allow_singular=True) | ||
| dist = stats.multivariate_normal(mean=None, cov=cov, allow_singular=True) |
There was a problem hiding this comment.
This may fix the nans seen in #3758 but once someone makes cov an RV, the same shape issues will pop up again. The exception will be silenced here and the user will only get back nans (which I think is wrong).
|
Sorry for not pointing to the actual changes In the example #3758, there is not problem if chol is passed, only when cov is passed. I am aware this is not a fix to all the current problems with MVnormal, just to the one in the opened issue. Regarding the exception you mentioned, that what exactly was it was going on before this change, the error was silenced and the only indication of problems was the nan returned. I agree is not the best scenario, maybe it is better to remove the try-except. What do you thing we should do, close this PR and work in a more complete solution? accept this in the meantime? |
aloctavodia
changed the title
|
I see now that this will only fix the nan problem in #3758, but there is still a problem with |
This should fix #3758