-
Notifications
You must be signed in to change notification settings - Fork 556
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
Corrected Multivariate Gaussian prior #775
Conversation
Some corrections including adapting to current version of pdinv, correcting the expressions of constant, pdf and its gradient, and adding the printing function. After some tests, seems to run as expected, similarly to the Gaussian prior which was already working.
Simple unit test for creating a kernel with Multivariate Gaussian prior over the lengthscales, then performing GP regression.
Codecov Report
@@ Coverage Diff @@
## devel #775 +/- ##
==========================================
- Coverage 54.24% 54.18% -0.06%
==========================================
Files 210 210
Lines 21347 21376 +29
Branches 2883 2890 +7
==========================================
+ Hits 11579 11583 +4
- Misses 9231 9243 +12
- Partials 537 550 +13 |
@@ -226,11 +233,11 @@ def pdf(self, x): | |||
|
|||
def lnpdf(self, x): | |||
d = x - self.mu | |||
return self.constant - 0.5 * np.sum(d * np.dot(d, self.inv), 1) | |||
return self.constant - 0.5 * np.dot(d.T, np.dot(self.inv, d)) |
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.
This implementation does not work when x has more than one dimension.
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.
Thanks for taking a look. I think you're right, but is the multivariate Gaussian prior not meant for vectors of hyperparameters anyway? If x is a vector it works, and I don't see why someone would want to put a multivariate Gaussian prior on a matrix of hyperparameters
@zhenwendai I think I understood what you meant. Multivariate Gaussians are only defined for random vectors not matrices, which is what I was saying. But you want the implementation to work when the prior is set on x of shape (n, 1) and not only (n,) is that right? That should be fixed now, this implementation can be used for x of shape (n,1) or (n,) to set a multivariate Gaussian prior. I added a test for each case. This has been working well for me for a while and I would really need it to be merged into the main code, it would be great if you could take a look! |
@zhenwendai would you be happy with this to be merged now? |
@monabf can you rebase this on |
Thanks @ekalosak, I merged the head of the devel branch into this pull request. Should be ready now, let me know if you need anything else! |
The merge from devel -> your branch is not showing up here. That's why the appveyor CI is failing: your branch has the python 2.7 build configured. The devel HEAD does not support python 2.7 nor does it require a passing build for python 2.7 - this is why I'd recommend rebasing on top of devel HEAD. This is just a technical issue with the way the CI is configured in your outdated branch, not a problem with your contribution per se. Should be a simple fix 🤞 |
Codecov Report
@@ Coverage Diff @@
## devel #775 +/- ##
==========================================
+ Coverage 54.38% 54.55% +0.17%
==========================================
Files 211 211
Lines 21534 21568 +34
Branches 2893 2894 +1
==========================================
+ Hits 11711 11767 +56
+ Misses 9273 9250 -23
- Partials 550 551 +1 |
Corrected code of Multivariate Gaussian prior on hyperparameters, and added a little unit test for it. Seems to be working for me!