Skip to content
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

Removing evaluate_kernel #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SebastianAment
Copy link
Collaborator

This PR aims at removing evaluate_kernel from the linear_operator package. This was already suggested in a comment in the code and makes sense since -- as far as I can tell -- it is a no-op for all operators in this package.

The main question at this point seems to be if additional changes are required in GPyTorch, which defines LazyEvaluatedKernelTensor and should therefore take care of any of its implementation specifics.

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.

There are a number of call sites in gpytorch that @gpleiss or @jacobrgardner have a much better understanding of, so I'll defer to their input on this.

@Balandat
Copy link
Collaborator

As to the changes here, they look good in isolation.

@jacobrgardner
Copy link
Member

I just want to be careful about making sure these don't get used. The problem these lines of code originally solved is that, with the current lazy kernel evaluation design, ALL covariance matrices are LazyEvaluatedKernelTensors until we evaluate the kernel, so if we need to type check the kind of linear operator we get out of a Kernel call we can't do that until the kernel has been evaluated

@gpleiss
Copy link
Member

gpleiss commented Sep 14, 2022

It would be great if we could remove this, but I agree with Jake that we want to be extra careful :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants