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

first go at extending the GibbsKernel to work with other base kernels #406

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

Conversation

Cyberface
Copy link
Contributor

Summary
@st-- in issue #372 suggested modifying the GibbsKernel to take as an argument a base kernel function.

This is a first cut at trying to implement this to get the conversation going. I'm sure I haven't implemented it correctly and I have changed any docstrings yet.

Proposed changes

  • adding a new function argument base_kernel which is by default the SqExponentialKernel.

What alternatives have you considered?

Breaking changes

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 66.89%. Comparing base (33d64d1) to head (7d8607c).
Report is 83 commits behind head on master.

Files with missing lines Patch % Lines
src/kernels/gibbskernel.jl 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (33d64d1) and HEAD (7d8607c). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (33d64d1) HEAD (7d8607c)
5 3
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #406       +/-   ##
===========================================
- Coverage   89.23%   66.89%   -22.35%     
===========================================
  Files          52       52               
  Lines        1198     1193        -5     
===========================================
- Hits         1069      798      -271     
- Misses        129      395      +266     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devmotion
Copy link
Member

We can't pass additional arguments to kernel(x, y), so implementation-wise the base kernel would have to be a field of kernel.

More generally, I wonder though if this generalization is mathematically. IIRC we had a discussion in the GibbsKernel PR in whixh @theogf and/or @willtebbutt were worried that allowing eg different metrics would already be problematic. I wasn't sure and didn't spend more thought on it but if this would be the case, wouldn't it be even more problematic to allow general kernels (which also allow you to pass a Gaussian kernel wirh a different metric)?

@st--
Copy link
Member

st-- commented Nov 11, 2021

As discussed in reference [4] of #372 and Paciorek's PhD thesis, I believe "stationary" is sufficient criterion (i.e. kernels that are implemented by a kappa of a scalar argument), assuming that the base kernel is positive definite in any dimension (I don't fully understand why the second part wouldn't just be trivial, but I'm assuming it's got something to do with the metric being valid in higher dimensions?) - in any case it seems that we'd have to somehow define "this kernel is stationary" - @willtebbutt suggested a trait, i.e. we need definitions isstationary(::Kernel) = nothing and isstationary(::SqExponentialKernel) = true etc. Then it should be possible to have this for arbitrary base kernels.

@st--
Copy link
Member

st-- commented Nov 11, 2021

We can't pass additional arguments to kernel(x, y), so implementation-wise the base kernel would have to be a field of kernel.

👍

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.

None yet

3 participants