8000 [MRG+1] Make return_std faster in Gaussian Processes by minghui-liu · Pull Request #9236 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG+1] Make return_std faster in Gaussian Processes #9236

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

[MRG+1] Make return_std faster in Gaussian Processes #9236

wants to merge 9 commits into from

Conversation

minghui-liu
Copy link
Contributor
@minghui-liu minghui-liu commented Jun 28, 2017 •
edited by TomDLT
Loading

Reference Issue

Issue #9234

What does this implement/fix? Explain your changes.

  1. Cache result of K_inv
if not hasattr(self,'K_inv_'):
        L_inv = solve_triangular(self.L_.T, np.eye(self.L_.shape[0]))
        self.K_inv_ = L_inv.dot(L_inv.T)
  1. Change line 329 from
    y_var -= np.einsum("ij,ij->i", np.dot(K_trans, K_inv), K_trans)

to

    sum1 = np.dot(K_trans, self.K_inv_).T
    y_var -= np.einsum("ki,ik->k", K_trans, sum1)

Any other comments?

No.

@minghui-liu minghui-liu changed the title Make return_std 20x faster in Gaussian Processes [WIP] Make return_std 20x faster in Gaussian Processes Jun 28, 2017
@jnothman
Copy link
Member

flake8 failures. Please mark as MRG when fixed, unless you feel this requires more tests.

@jnothman jnothman changed the title [WIP] Make return_std 20x faster in Gaussian Processes [WIP] Make return_std faster in Gaussian Processes Jun 28, 2017
jnothman
jnothman reviewed Jun 28, 2017
View reviewed changes
sklearn/gaussian_process/gpr.py Outdated
L_inv = solve_triangular(self.L_.T, np.eye(self.L_.shape[0]))
K_inv = L_inv.dot(L_inv.T)
# cache result of K_inv computation
if not hasattr(self,'K_inv_'):
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to memoize this, we should not be storing a new public attribute at predict time. If be more comfortable if this were a private attribute, i.e. _K_inv

@jnothman
Copy link
Member

LGTM, thanks!

@jnothman jnothman changed the title [WIP] Make return_std faster in Gaussian Processes [MRG+1] Make return_std faster in Gaussian Processes Jun 28, 2017
TomDLT
TomDLT reviewed Jun 28, 2017
View reviewed changes
sklearn/gaussian_process/gpr.py
# decomposition L and its inverse L_inv
L_inv = solve_triangular(self.L_.T,
np.eye(self.L_.shape[0]))
self._K_inv = L_inv.dot(L_inv.T)
Copy link
Member

Choose a reason for hiding this comment

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

self._K_inv needs to be deleted if self.L_ is changed during a second fit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. Thank you. How do I test if self.L_ has changed?

Copy link
Member

Choose a reason for hiding this comment

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

self.L_ is changed only in this line, so you should add for example self._K_inv = Nonejust after, and change the condition to if self._K_inv is None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologize for the delay. I've made the changes.

@TomDLT
Copy link
Member
TomDLT commented Jul 11, 2017

Thanks for the update.

Could you also add a test in sklearn/gaussian_process/tests/test_gpr.py.
I would like to assert that self._K_inv is correctly reset after a new fit.

After that it should be ready for merging.

@minghui-liu
Copy link
Contributor Author

Hi. I have added a new test.

@amueller
Copy link
Member

Maybe a better test would be that doing fit(X), predict, fit(X2), predict gives a result independent of the first call of fit?

Needs a whatsnew entry?

@minghui-liu
Copy link
Contributor Author
minghui-liu commented Aug 2, 2017 •
edited
Loading

@amueller I have updated my test. Could you please have a look and let me know if it is right?

TomDLT
TomDLT approved these changes Aug 3, 2017
View reviewed changes
Copy link
Member
@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

LGTM

lesteve
lesteve reviewed Aug 3, 2017
View reviewed changes
sklearn/gaussian_process/gpr.py Outdated
# Compute variance of predictive distribution
y_var = self.kernel_.diag(X)
y_var -= np.einsum("ij,ij->i", np.dot(K_trans, K_inv), K_trans)
sum1 = np.dot(K_trans, self._K_inv).T
y_var -= np.einsum("ki,ik->k", K_trans, sum1)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you changed this? I thought this PR was only about caching self._K_inv. As @TomDLT mentioned in #9234 (comment), the einsum optimization was arleady done in #8591.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the change. Thank you @lesteve.

@jnothman
Copy link
Member

flake8 is failing

@minghui-liu
Copy link
Contributor Author

Fixed flake8

@lesteve
Copy link
Member
lesteve commented Aug 21, 2017 •
edited by jnothman
Loading

@minghui-liu Can you add an entry in doc/whats_new.rst?

@ogrisel
Copy link
Member
ogrisel commented Sep 1, 2017

Rebased, added whats new and merged as: 6e01fef.

@ogrisel ogrisel closed this Sep 1, 2017
@ogrisel ogrisel mentioned this pull request Sep 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

@lesteve lesteve lesteve left review comments

@TomDLT TomDLT TomDLT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@minghui-liu @jnothman @TomDLT @amueller @lesteve @ogrisel
0