-
Notifications
You must be signed in to change notification settings - Fork 32
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
[WIP] Rework on kernelmatrix to work with Vectors and more complex kernels #83
Changes from 15 commits
1801fc7
3554faa
d8ec7b9
1cb4868
764fcd7
2b9d20a
9455aca
7cc1eb2
576a2f5
6e21244
d57ab6b
32a1e68
fd4cfd7
cbb024a
220b162
d117f25
7640550
64d0753
5ddf568
d6b88a1
1698e51
87eeae7
e536233
f1d0433
87be861
efa1479
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ end | |
|
||
kappa(k::ScaledKernel, x) = first(k.σ²) * kappa(k.kernel, x) | ||
|
||
kappa(k::ScaledKernel, x, y) = first(k.σ²) * kappa(k.kernel, x, y) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why has this been added? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I realised that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, okay. And this is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh no, it's in the sense of |
||
|
||
metric(k::ScaledKernel) = metric(k.kernel) | ||
|
||
Base.:*(w::Real, k::Kernel) = ScaledKernel(k, w) | ||
|
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.
Maybe
DistancesKernel
would highlight more clearly that for these kernels we make use of thepairwise
functionality in Distances? SimpleKernel and BaseKernel both sound very similar and are both not very descriptive names IMO.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 am not sure if it would be better but I was thinking we could change it as :
Kernel
->AbstractKernel
BaseKernel
->Kernel
SimpleKernel
->SimpleKernel
/BaseKernel
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
AbstractKernel
might be better and I guessKernel
would be fine instead ofBaseKernel
👍 However, I think it would be nice to use something better than SimpleKernel or BaseKernel. At least I personally don't have an intuition for the names SimpleKernel or BaseKernel - is it referring to some mathematical property or form? That's why I think something more telling would be nice.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.
Well the problem is that, to my knowledge, we make the separation only for computational reasons... So there is no clear name.
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, that was the motivation for DistancesKernel since it might highlight the point that these kernels use Distances for computing the kernel matrix more explicitly. As mentioned before, an alternative would be to not encode this in the kernel type but just use a traits based system for separating kernels for computational reasons.