Skip to content

Conversation

@sachinruk
Copy link
Contributor

As requested by #2569 creating a pull request to include cholesky decomposition of the Covariance matrices.

@twiecki
Copy link
Member

twiecki commented Sep 28, 2017

This looks good. Thanks for the contribution @sachinruk. Can you add a test?

@sachinruk
Copy link
Contributor Author

@twiecki could you show me an example test that was already there for normal MvGaussianRandomWalk? I can base my test off of that. Sorry my coding skills aren't the best.

@twiecki
Copy link
Member

twiecki commented Oct 4, 2017

Hm, I guess there is no existing unittest. Those are probably also hard to write in this case. Alternatively, can you post an example NB that shows this works?

@sachinruk
Copy link
Contributor Author

Done! Hope it's all good. https://github.com/sachinruk/pymc3/blob/chol_MV/docs/source/notebooks/MvGaussianRandomWalk_demo.ipynb if you want to see the Notebook before you accept.

@junpenglao
Copy link
Member

LGTM, @aseyboldt any comments?

@junpenglao
Copy link
Member

Is there any reason you are not going with the solution you proposed in #2569 using class MvGaussianRandomWalk(_QuadFormBase)?

@sachinruk
Copy link
Contributor Author

sachinruk commented Oct 18, 2017

From memory, I did try that but got an error which I forget what it is. _CovSet is a subset of the functions in _QuadFormBase. Actually I don't think I could have imported _QuadFormBase, because it was a hidden class. _CovSet is a subset of _QuadFormBase where just the functions that are necessary for MvGaussianRandomWalk have been copied across.

@sachinruk
Copy link
Contributor Author

Hey peeps, sorry to bug you all but is there anything more to do here?

@junpenglao junpenglao merged commit d586c6e into pymc-devs:master Oct 30, 2017
@junpenglao
Copy link
Member

Forgot about this - thanks for the contribution!

@sachinruk sachinruk deleted the chol_MV branch October 30, 2017 05:58
ColCarroll pushed a commit that referenced this pull request Nov 9, 2017
* Added cholesky to MVGaussianRandomWalk and MVTRandomWalk

* Included MvGaussianRandomWalk_demo in notebooks
@sachinruk sachinruk restored the chol_MV branch November 14, 2017 23:22
@sachinruk sachinruk deleted the chol_MV branch January 4, 2018 11:49
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.

3 participants