Add metric field
#286
Add metric field
#286
Conversation
|
@devmotion this is nice. Could you elaborate on what aspect of it is breaking? Seems to me that users shouldn't see any obvious difference. |
|
The types change. E.g., currently |
|
Ah okay, makes sense. |
|
I have no objections to this, but I'll let @theogf review and approve. |
src/basekernels/exponential.jl
Outdated
| ```math | ||
| k(x, x') = \\exp\\bigg(- \\frac{\\|x - x'\\|_2^2}{2}\\bigg). | ||
| k(x, x') = \\exp\\bigg(- \\frac{\\|x - x'\\|^2}{2}\\bigg). |
There was a problem hiding this comment.
Should |x - x'| be replaced entirely by something more abstract like d(x, x')?
This comment applies generally to the other kernels in general
|
Good idea! I'll generalize the docstrings, I don't think it should be restricted to this form. |
|
I updated the docstrings. |
|
Nice! Maybe one last thing would be to write for each kernel explicitly "by default d is the Euclidean metric" for example |
|
In the section with the mathematical definition? I assumed it would be sufficient to mention it in the function signature in the docstring, similar to other parameters whose default values are also not mentioned in the definition. |
|
Yeah kind of below the mathematical definition so people who are not so familiar with kernels can have an idea how they "usually" look like. |
|
I added a note that explains that the default choice for |
|
Test failures with Julia nightly are unrelated. |
When I updated some of my calibration packages recently, it was quite annoying to see that I had different multiple exponential kernels just because I wanted to use different metrics. So in this PR I tried to fix #273 (at least to some extent) and added a
metricfield to some of the kernels.Since the change is breaking, I also removed the deprecations in a second commit.
@torfjelde might be interested in this PR 🙂