Skip to content

Conversation

@molet
Copy link
Contributor

@molet molet commented Jul 8, 2024

Summary
Tiny change to preserve float type of inputs and parameters. For most of the kernels, if the inputs and parameters have Float32 type, then the output has also Float32:

julia> v1 = 0.9f0; v2 = 0.7f0;
julia> SqExponentialKernel()(v1, v2)
0.9801987f0
julia> GammaRationalKernel(alpha=2.0f0, gamma=1.0f0)(v1, v2)
0.82644624f0

However, currently, PeriodicKernel doesn't support this:

julia> PeriodicKernel(r=[1.0f0])(v1, v2)
0.8413515018155655

The simple change of exp(-0.5d) to exp(-d / 2) in its kappa function fixes it:

julia> PeriodicKernel(r=[1.0f0])(v1, v2)
0.8413515f0

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, can you add a test for the fix?

@molet
Copy link
Contributor Author

molet commented Jul 8, 2024

Certainly, it's been added.

@codecov
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 16.29%. Comparing base (c38b366) to head (8686dd1).

Files Patch % Lines
src/basekernels/periodic.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #560       +/-   ##
===========================================
+ Coverage    0.42%   16.29%   +15.87%     
===========================================
  Files          52       52               
  Lines        1423     1430        +7     
===========================================
+ Hits            6      233      +227     
+ Misses       1417     1197      -220     

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

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@willtebbutt
Copy link
Member

The changes that you've made do not appear to have anything to do with the test failures, so I'm going to merge.

@willtebbutt willtebbutt merged commit ab866b9 into JuliaGaussianProcesses:master Jul 22, 2024
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.

3 participants