Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding Gibbskernel #374
Adding Gibbskernel #374
Changes from 14 commits
d50f2b4
2ca62f4
2b83286
e4d1414
f9f066a
d6b53b9
698c453
3a98028
0f757a3
2e1df2d
b5c6faa
19e6665
21de7d4
4ddafbe
9fd1d14
5531851
78faeac
ae8b3d4
6b91a23
a914957
6eb4099
79aa4f5
0942447
de3e535
0caf2fd
cfbca3d
1fb7903
f577700
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should more correspond to the exact formula
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without promotion to Float64:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct though because
l
is passed intowith_lengthscale
- doesn't theSqExponentialKernel
already have the1/2
in the exponential looking hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but in the latex formula the factor 2 is not there, so this way we cancel it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the 2 disappears because we already have l(x) + l(y) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the 2 would be there when l(x) = l(y) = constant.
I think I finally understand most of the things here but I am confused by
with_lengthscale
. Can you explain why we shouldn't square the argument to with_lengthscale? Looking at the with_lengthscale code it seems to just multiply by1/l
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what 2 you refer to (and there is no l(x) + l(y) term anywhere 🤔). The 2 in the square root outside of the squared exponential kernel disappears because we already scale the square root of the denominator by 1/sqrt(2).
It will be squared by the squared exponential kernel. A squared exponential kernel with lengthscale l is of the form
k(x, y) = exp(- (x - y)^2/(2*l^2))
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the
l(x) + l(y)
term I was refering to is the denominator in the exponential i.e.exp(-(x - y)^2 / (l(x)^2 + l(y)^2)
And when
l(x) = l(y) = l = constant
you getexp(-(x - y)^2 / (2l^2)
?Also thanks for explaining to me about the
with_lengthscale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, if they are constant you get
exp(-(x-y)^2 / (2 * l^2))
. Very explicitly, in general the implementation yieldswhich is exactly the formula in the docstring (and the other issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's more what I meant to use instead of a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the test it would be great if you could be as faithful as possible to the latex formula, it makes it easier to see if it's doing the right thing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what you mean, could you elaborate or sketch out an idea of what you would like to see?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n.b. probably better to use
≈
here rather than==
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see what you mean. I've added your suggestion but actually this test is failing and I'm not sure why!
I don't think I fully understand the implementation in https://github.com/Cyberface/KernelFunctions.jl/blob/gibbskernel/src/kernels/gibbskernel.jl#L37 so will take a look a bit later again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure the using
norm(x-y)
is correct here? Calling this onThis returns a single number - shouldn't this be a 2x2 matrix?
Assuming the
norm
is fromLinearAlgebra
?Sorry for my confusion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, this the L2 norm! So it's
sqrt(sum((x_i - y_i)^2))
, this is what we would expect I thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I get you now - I thought this was supposed to give the covariance matrix but that is the
kernelmatrix
function!This is just the covariance between two points in n-dimensions.
Hopefully I got this correct :)