Spate of (largely cosmetic) changes to distributions.py, alternate inverse_wishart implementation, etc. #3
Conversation
…onsolidated 'namespace injection' code
rwishart and rwishart_cov were producing different results for the same random seed (the random variables inside are generated in the same order)
clarify use of covariance (scale) instead of precision * Implemented inverse wishart distribution parameterized using precision matrix * More unit tests to check consistency of cov / precision based wishart / inverse wishart-based samples / likelihoods * A little bit of code reformatting and fixed earlier M-q reformats
|
Thanks very much, Wes, I'll have a look at this Monday. Anand |
|
Very nice. One thing is that I don't think you'd expect rwishart and rwishart_cov to give the same result for the same random seed. The same random seed means that the same random variates will be produced, but one is using an LL^T factorization whereas the other is effectively using a UU^T factorization, meaning they can both be correct yet give different answers. I think the RV's generated by the two are identically distributed, see http://gist.github.com/644869 . Do you agree? If so, the existing implementation is preferable because it saves a matrix inversion and so should be a bit faster. Anand |
|
You're right about rwishart_cov, looking more carefully at the linear algebra and it makes sense now. Also I changed a parameter name in the inverse_wishart functions from Tau to Sigma since it's expecting a covariance matrix and not a precision matrix-- but I see this is breaking a unit test. I don't want to break anyone's code so I can change this back-- let me know what I should do. I can make these two changes and let you pull if everything else looks good. Thanks, |
|
BTW I'll try to be cleaner about proposing changes to the codebase next time. I assume the correct git workflow is: make a branch wesm/set-of-changes-or-new-feature |
|
Cool. As far as the name change, we want to keep the parameterizations consistent with Wikipedia unless there's a good reason not to, and I think your change does that. However, I'd prefer C to Sigma in order to keep consistency with the mv normal distribution. As far as breaking peoples' code, it's definitely a concern but I don't know who is using the Wishart distribution right now. I'll post to the ML and ask. Thanks for sticking with this! Anand |
|
Regarding cleanliness, you're doing fine! |
|
People have had a while to speak up now if the name change causes them problems... I think we're good to go ahead. |
|
OK I will make the name change a bit later and let you know when it's safe to merge. Thanks! |
|
Merge away-- let me know if you have any trouble. Cheers, |
Fixed ImportError for submodules
Sorry about all the spam today guys. I'm a 1st-year PhD student at Duke and I decided it'd be time well-invested to help develop PyMC rather than cooking up my own bayesian stats library in Python.
The only significant things here are:
I apologize in advance that you may find me compulsively reformatting code to be more in line with PEP 8. The main thing that bothers me is lines of code being too wide for my emacs buffers-- I hope I'm not the only one. I'd like to think this will make the code easier for others to read.
Cheers,