-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
FIX GaussianProcessRegression(normalize_y=True).predict(X, return_cov=True) #19706
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
Conversation
Problem with GPR predictions on multitarget data.
I'm not with merging rights here, but I think you need tests for this one, see bottom of #19703 for a test example. |
Thank you, I will do it |
@AidarShakerimoff thanks for your pull request. Linting errors are present. Do you mind running
on your repository? This will allow you to display all the errors. See also this page for more details about flake8. |
Thank you for your advice! |
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.
Please add a new test (or update an existing test) to check that if the target data is already normalized (before fit, for instance using sklearn.preprocessing.scale
), passing normalize_y=True
or normalize_y=False
makes predict
return similar results (on toy random datasets generated with make_regression
for instance).
Please check with return_cov=True
and with multi-target y.
Thank you very much, I will finish it soon. |
By the way, should I also try to handle cases when there is a correlation between classes of the output? Or should keep assuming that the classes are independent? |
multitarget now produces +1 dimenisonal std and cov
How can I solve changelog problem? |
I am trying to implement the following code that compares results of two GPR (normalized and not normalized) on the dataset that is already normalized.
The assertion provides error even when I work with single-target data.
Does it imply that there is something wrong in the ways how normalization is done in GPR (regardless of target's dimensionality)? |
The check verifies that this pull request is referenced in the changelog if necessary: this is a bugfix so it is worth an entry in the changelog. But I suggest to wait for a reviewer clarifying if this fix is useful in the bugfix release 0.24.2 (as #19703) or should go in 1.0. Thanks for your patience. |
Thank you for clarification! |
I don't want to merge #19703 with code that might not be working properly. Instead, I will add it here: # Test multi-target data
# This test should be uncommented when a solution to is found #19706
# n_samples = X.shape[0]
# rng = np.random.RandomState(0)
# Y_with_constant = np.concatenate([
# y[:, np.newaxis], # non-constant target
# np.full(shape=(X.shape[0], 1), fill_value=2) # constant target
# ], axis=1)
#
# # Ensure no tracebacks
# gpr.fit(X, Y_with_constant)
# Y_pred, Y_cov = gpr.predict(X, return_cov=True)
#
# assert_all_finite(Y_pred)
# assert_all_finite(Y_cov)
#
# # Assert correct shapes
# assert y_pred.shape == (X, Y)
# assert y_cov.shape == (X, Y) I will just add a |
Superseded by #21996 |
closing this PR then. |
Problem with GPR predictions on multitarget data. Fixes issues #17394 and #18065
What does this fix?
Initially, when a multitarget data was given, the _gpr.py was not able to undo the normalisation of
y_cov
andy_var
in the predict() function (lines 355 and 381). The problem was the fact that the code was trying to multiply two horizontal vectors in both cases and returned errorValueError: operands could not be broadcast together with shapes (a,) (b,)
(e.g. number of samples 'a' = 4, number of target components 'b' = 3). I solved this by reshaping they_var
vector and rows ofy_cov
into vertical form before multiplication with they_train_std
vector. As a result, we obtainy_std
of form(n_samples, n_output_dims)
andy_cov
of form(n_samples, n_samples, n_output_dims)
. When targets are single values the undo normalisation is performed as usual.The solution is based on the solution from Rasmussen and Williams' book for cases when classes of outputs are independent from each other.
Comment
This solution will work only if the target classes are independent.