[MRG] DOC clarify the kernel gradient for GaussianProcesses#18115
Merged
jnothman merged 6 commits intoscikit-learn:masterfrom Aug 15, 2020
Merged
Conversation
Contributor
Author
thomasjpfan
reviewed
Aug 7, 2020
Member
thomasjpfan
left a comment
There was a problem hiding this comment.
Thank you for the PR @rauwuckl !
I ran into this exact issue when reviewing other kernel related code. (I also thought it was respect to hyperparamter.
I think the eval_gradient's docstrings in:
def __call__(self, X, Y=None, eval_gradient=False):
eval_gradient : bool, default=False
Determines whether the gradient with respect to the kernel
hyperparameter is determined.should be updated as well.
XREF #8373 (comment)
…s://github.com/rauwuckl/scikit-learn into documentation_gaussian_process_kernel_gradient
Contributor
Author
|
I added a commit changing it to: def __call__(self, X, Y=None, eval_gradient=False):
"""Return the kernel k(X, Y) and optionally its gradient....
eval_gradient : bool, default=False
Determines whether the gradient with respect to the log of the
kernel hyperparameter is determined.
"""does that look good to you @thomasjpfan ? |
thomasjpfan
approved these changes
Aug 8, 2020
Member
thomasjpfan
left a comment
There was a problem hiding this comment.
LGTM! Thank you @rauwuckl !
Contributor
Author
|
@thomasjpfan |
Member
|
We need to wait for another reviewer to approve this PR. |
Contributor
Author
|
ping @amueller |
jnothman
approved these changes
Aug 15, 2020
sklearn/gaussian_process/kernels.py
Outdated
| Determines whether the gradient with respect to the kernel | ||
| hyperparameter is determined. | ||
| Determines whether the gradient with respect to the log of the | ||
| kernel hyperparameter is determined. |
Member
There was a problem hiding this comment.
can we change "is determined" to "is computed" here and below, while we are at it?
Member
|
Thank you @rauwuckl |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
Fixes #11581
Fixes #8393
Fixes #11116
Also see this comment
What does this implement/fix? Explain your changes.
The gradients of the kernels in the gaussian process module are computed with regard to the log-transformed hyper-parameter. So far this is not reflected clearly in the documentation and is thus confusing for people wanting to implement their own custom kernels. See for example the following issues:
#14206
#8373
#8359
Here I add 2 lines of explanation to the user guide, that specify the equation for the kernel gradient and emphasise the log-transform of the parameters. Further I add a mention of the log-transform to the doc string of the call method of existing kernels.
Any other comments?
See the discussion in this issue for a more detailed reasoning of this necessary documentation change.