-
Notifications
You must be signed in to change notification settings - Fork 563
ConstantKernel
#2511
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
ConstantKernel
#2511
Conversation
027fc30
to
2e65b14
Compare
@@ -82,6 +82,7 @@ def find_version(*file_paths): | |||
"nbclient<=0.7.3", | |||
"nbformat<=5.8.0", | |||
"nbsphinx<=0.9.1", | |||
"lxml_html_clean", |
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.
Orthogonal dependency fix, see e.g. https://github.com/assafelovic/gpt-researcher/pull/424/files for a similar fix.
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.
Hmm that link doesn't really have much information on what exactly this fixes / why this dependency is needed. Is there more info on this?
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.
There was a change in some package required for the sphinx compilation recently, which outsourced part of the functionality into this new package (there's more info in the description tab of the linked PR).
The doc compilation will fail for any upcoming commit without this, so this is independent of the other changes made here.
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.
Copying over the error message from the link, which was raised on this commit as well before I added the line:
ImportError: lxml.html.clean module is now a separate project lxml_html_clean.
Install lxml[html_clean] or lxml_html_clean directly.
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.
lgtm, thanks!
FYI @gpleiss @jacobrgardner - IIRC we had a constant kernel in the past but removed it. @SebastianAment's results on the numerical issues in the MAP estimation due to the unregularized kernel are interesting and suggest that using such a kernel may be preferable to using a ConstantMean
.
@@ -82,6 +82,7 @@ def find_version(*file_paths): | |||
"nbclient<=0.7.3", | |||
"nbformat<=5.8.0", | |||
"nbsphinx<=0.9.1", | |||
"lxml_html_clean", |
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.
Hmm that link doesn't really have much information on what exactly this fixes / why this dependency is needed. Is there more info on this?
2e65b14
to
eeb21c1
Compare
Head branch was pushed to by a user without write access
eeb21c1
to
3e990b3
Compare
3e990b3
to
89dfb46
Compare
This commit adds
ConstantKernel
, moving the inference of additive constants into the kernel.Status quo: By default both GPyTorch and BoTorch are currently using a
ConstantMean
that is inferred during the optimization of the marginal likelihood. Because the mean parameter does not show up in the log determinant, it is un-regularized, which can lead to some counter-intuitive values for the constant mean, especially when the other parameters are constrained or subject to a non-trivial prior.The inference of the posterior mean of the constant using the kernel approach is carried out with the usual linear algebraic formulation, which is more robust. See the following for an illustrative example of what can go wrong with the current
ConstantMean
approach, and how theRBF + Constant
kernel behaves as expected.