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

Why does krr depend on kernel? #9

Open
JeffFessler opened this issue Jan 29, 2022 · 2 comments
Open

Why does krr depend on kernel? #9

JeffFessler opened this issue Jan 29, 2022 · 2 comments

Comments

@JeffFessler
Copy link
Member

Shouldn't the kernel argument of krr always be the same as that used for krr_train?
Seems like it should at least be the default choice.
Would there ever be any reason to over-ride it?

@StevenWhitaker
Copy link
Collaborator

You're right, krr should use the same kernel as krr_train. This really only affects ExactKernels (you'll notice that krr doesn't use kernel for the RFFKernel case, so in that case users are already constrained to use the same kernel as used in krr_train).

So I agree that for consistency and practicality krr should always use the same kernel as krr_train. I think I originally wrote it the way it is currently because I wasn't sure of a good way to enforce using the same kernel. But now that I think about it more, I guess a way to enforce it would be to include the kernel in the ExactTrainingData object (would I then need to change the name, because the kernel isn't really data?) and then not have the kernel argument to krr.

Let me know what you think, or if you had your own idea of how to enforce consistency (or not enforce it but give a consistent default).

@JeffFessler
Copy link
Member Author

Yes I was thinking of the training stage as analogous to a "Plan" stage
(cf https://juliamath.github.io/AbstractFFTs.jl/stable/api/#AbstractFFTs.plan_fft)
where the struct would store all the relevant stuff that went into the training,
none of which is allowed to change at testing time (AFAIK), including \rho, \Lambda, and the kernel.
The name with Data is OK to me because parameters are a kind of data :)
BTW, this is a lower priority than my other PRs.

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

No branches or pull requests

2 participants