scikit-learn / scikit-learn

scikit-learn: machine learning in Python
https://scikit-learn.org
BSD 3-Clause "New" or "Revised" License
60.04k stars 25.39k forks source link

Gaussian Process Regressor Kernel Default inaccurate #17871

Open MaxVanDeursen opened 4 years ago

MaxVanDeursen commented 4 years ago

Describe the issue linked to the documentation

In the documentation of the GPR it states that the kernel "1.0 * RBF(1.0)" is used as default when None is passed and that the hyperparameters of the kernel is fitted during the fit method. However, the default kernel (as seen here) is instead C(1.0, length_scale_bounds="fixed") * RBF(1.0, length_scale_bounds="fixed"). This results in the described kernel differing from the actual default kernel, as the latter does not allow for parameter tuning.

Suggest a potential alternative/fix

There's two ways that a fix could be made:

I use an issue to raise this rather than creating a merge request because I am not sure which of the above solutions is better. I personally think that it is better for the default behavior of the GPR to actually be fittable, which would require the second option to be chosen. However, I understand that people might disagree.

hkennyv commented 4 years ago

Thanks for raising this issue @MaxVanDeursen , I ran into this same wrinkle the other day. It's not explicitly clear what's going on either unless you dig into the implementation.

I think my vote would be for removing the x_bounds and making the default: 1.0 * RBF(1.0). This seems like the most sensible default and is consistent with the "de-facto default kernel for GPs" that is described in the reference material included [1]. I believe the intended behavior should be to optimize the hyperparameters as well unless the user explicitly is passing in their own kernel using the x_bounds = "fixed" kwarg.

[1] https://www.cs.toronto.edu/~duvenaud/cookbook/

jnothman commented 4 years ago

Let's start by changing the docs to match/clarify the reality, then we can consider a change of default behaviour later / at the same time.

hkennyv commented 4 years ago

Thanks for the guidance, I'm going to link my comment I made on a similar issue (#12444) where i ran the unit tests after changing the default behavior. It looks like the tests passed for:

but quite a few of them fail for:

It seems like it is mostly due to GaussianProcessRegressor.kernel_ not getting set if kernel=None when creating the GPR object.