8000 slight improvements to IndexKernel for the rank == 0 (diagonal) case by rmgarnett · Pull Request #2141 · cornellius-gp/gpytorch · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

slight improvements to IndexKernel for the rank == 0 (diagonal) case #2141

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

slight improvements to IndexKernel for the rank == 0 (diagonal) case #2141

wants to merge 2 commits into from

Conversation

rmgarnett
Copy link
@rmgarnett rmgarnett commented Sep 19, 2022 •
edited
Loading

This PR tweaks IndexKernel to have slightly better performance/behavior in the corner case of rank == 0, where the underlying covariance matrix is diagonal. Although this case is atypical from the point of view of multitask GPs, I have found this construction useful as a component of larger covariance structures.

Two changes are proposed:

  • the kernel only registers the covar_factor parameter (the low-rank component when rank > 0) when rank > 0
  • in the rank == 0 case, the covar_matrix() method returns a DiagLinearOpeator rather than a PsdSumLinearOperator(RootLinearOperator, DiagLinearOperator)

@rmgarnett rmgarnett changed the title slight improvements to IndexKernel for the rank = 0 (diagonal) case slight improvements to IndexKernel for the rank == 0 (diagonal) case Sep 19, 2022
Balandat
Balandat reviewed Sep 20, 2022
View reviewed changes
Copy link
Collaborator
@Balandat Balandat left a comment

Choose a reason for hiding this comment

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

In principle it seems reasonable enough to me to support this.

However, looking at forward and _eval_covar_matrix the code as is will cause some issues. Namely in _eval_covar_matrix (called from forward) we will get an AttributeError when we try this from self.covar_factor. Which means that we should also add a unit test case to cover the rank==0 behavior.

Taking a step back, if we handled that correctly, we probably shouldn't return an InterpolatedLinearOperator in forward - or at least we'd want to special case InterpolatedLinearOperator to handle a diagonal base_linear_op (it doesn't seem that this is currently the case).

edge

@rmgarnett
Copy link
Author

I've added a branch to _eval_covar_matrix() to handle the diagonal case to partially address @Balandat's comments.

gpleiss
gpleiss approved these changes Sep 21, 2022
View reviewed changes
Copy link
Member
@gpleiss gpleiss left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll let @Balandat make the final approval.

Balandat
Balandat approved these changes Sep 22, 2022
View reviewed changes
Copy link
Collaborator
@Balandat Balandat left a comment
8000

Choose a reason for hiding this comment

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

lgtm. I think at least some of the special casing of using a DiagLinearOperator is actually happening inside InterpolatedLinearOperator so we'll probably get some gains there already.

@Balandat
Copy link
Collaborator

I guess I would still like to see this covered by a unit test though :)

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

@gpleiss gpleiss gpleiss approved these changes

@Balandat Balandat Balandat 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.
3 participants
@rmgarnett @Balandat @gpleiss
0