Skip to content
This repository was archived by the owner on Feb 28, 2024. It is now read-only.

Conversation

@PeraltaFede
Copy link

@PeraltaFede PeraltaFede commented Mar 23, 2021

Bug description

Calling multiple times .fit(X, y) whenever self.noise exists causes self.kernel to add new WhiteKernel noise on every call, inefficiently adding kernels if the base kernel is not None.

Fixes #1010

Steps to reproduce

from skopt.learning.gaussian_process import gpr, kernels  #gpr is the package with the bug
import numpy as np
np.random.seed(0) # for reproduction

gp = gpr.GaussianProcessRegressor(kernel=kernels.RBF(), alpha=1e-7, noise=0.01)
train_inputs = np.array([0, 0.1, 0.2]).reshape(-1, 1)
train_targets = np.array([1, 1.5, 0.6])
for i in range(10):
    gp.fit(train_inputs, train_targets)
    test_inputs = np.arange(0, 0.25, 0.05).reshape(-1, 1)
    gp.predict(test_inputs)
    print(str(gp.get_params()['kernel__k1']).count("WhiteKernel")) # number of WhiteKernels in self.kernel increases with each .fit
    train_inputs = np.vstack([train_inputs, np.random.rand()])
    train_targets = np.append(train_targets, np.random.rand())

Fix

If, however, we check if a WhiteKernel is present before adding a new one on the .fit function, only one WhiteKernel is added after many .fit() calls

        if self.noise and not _param_for_white_kernel_in_Sum(self.kernel)[0]:
            if self.noise == "gaussian":
                self.kernel = self.kernel + WhiteKernel()
            else:
                self.kernel = self.kernel + WhiteKernel(
                    noise_level=self.noise, noise_level_bounds="fixed"
                )

@kernc
Copy link
Contributor

kernc commented Mar 24, 2021

The fix looks good. Can you also write a simple unit test for it?

self.kernel = self.kernel + WhiteKernel(
noise_level=self.noise, noise_level_bounds="fixed"
)
if self.noise and not _param_for_white_kernel_in_Sum(self.kernel)[0]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as moving the whole thing into the constructor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is self.kernel can be None in the constructor. Therefore, calling _param_for_white_kernel_in_Sum(self.kernel) will raise NoneType associated exceptions.

Will make some tests and add them to test_gpr.py.

Copy link
Author

@PeraltaFede PeraltaFede Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added unit test.

The test fails with the old implementation, passes with the new one.

@kernc let me know if further changes are needed, thanks in advance

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GPR with noise .fit bug

2 participants