Correction of the spectral_mixture_kernel

[theogf]

[Updated description] I reworked a bit more deeply the spectral_mixture_kernel and spectral_mixture_kernel_product functions:

• spectral_mixture_kernel is now SpectralMixtureKernel. The previou implementation was false as it was expecting that CosineKernel was computing cospi(y' x) when it is computing cospi.(abs.(y .* x)). Since the former is not a kernel I resolved to implement k(x,y) specifically. spectral_mixture_product_kenrel did not need any change and I deprecated spectral_mixture_kernel
• Rewrote the docstrings more accurately
• Matched the functions (and docstrings) with the cited papers
• Changed the names of the arguments (removed the s)
• Changed the canonical form of the arguments AbstractMatrix -> AbstractVector{<:AbstractVector} the original constructor is still valid though.

[old description] I was checking the docstrings and the one for the spectral_mixture_kernel is particularly wrong I think. I double checked with the first reference paper as well as the implementation and changed the formula/description of the arguments

[willtebbutt]

Just checking: the main technical change here is gammas being described as the covariance vs sqrt of covariance matrix?

[theogf]

Wait actually also this is wrong 😅, the gammas is the collection of the standard deviations (according to the implementation)

[devmotion]

Ah yeah, I didn't clean up this docstring when updating the documentation since I am not familiar with it. Thought I'd leave it for someone else 😛

[theogf]

I will take care of it more in depth (more is needed I think). I am just a bit puzzled by the Matrix representation of the variables arguments, we should allow vector of vectors and Co as well. Also I would like to remove the s suffix of all the Greek letters

[theogf]

@willtebbutt @devmotion @st-- I would like to move the sm.jl file into the kernels folder as it is a composite kernel and change its name to spectral_mixture.jl should I do this in this PR or do it in a future PR?

[st--]

Probably easier to do it separately (then the only change would be the include() and git mv, instant approval) and then we don't risk complicating the review of this pr if GitHub doesn't display the diff properly anymore.

On Tuesday, August 31, 2021, Théo Galy-Fajou @.***> wrote:

@willtebbutt @devmotion @st-- I would like to move the sm.jl file into the kernels folder as it is a composite kernel and change its name to spectral_mixture.jl should I do this in this PR or do it in a future PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. < https://ci3.googleusercontent.com/proxy/AcQ25A1ylDC0zqDJKu61QI_to28DRB56NJtELryfLp92xCwhrI7aBv2AI631Y7U40UfDxp8ADzt_jc2z2cN0S1SeqJk0CqjAq2CiGejInEXLppx8YPxfkcE-BSFuAuXpSOmfSaieKta21mymYBin6nHwcWHorc6CXUZjMwIGgPHilf01fmfNv0l8VXPY45h89etcRuVNboCWDOV482KuQeOdPgyeaess3dyUev_P1A=s0-d-e1-ft#https://github.com/notifications/beacon/ABL7FD4KOGQIQOEGOBO2NVTT7STBDA5CNFSM43BPENS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGYX3BGY.gif>

[theogf]

I had to make some substantial changes but now things should be all correct!

[theogf]

@devmotion Are you happy with the PR? Should I just bump the version?

[theogf]

@devmotion @st--bump :)

[codecov[bot]]

Codecov Report

Merging #279 (d3f2d92) into master (cbbf1d4) will decrease coverage by 0.19%. The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
- Coverage   93.14%   92.94%   -0.20%     
==========================================
  Files          52       52              
  Lines        1210     1219       +9     
==========================================
+ Hits         1127     1133       +6     
- Misses         83       86       +3     

Impacted Files	Coverage Δ
src/KernelFunctions.jl	100.00% <ø> (ø)
src/transform/lineartransform.jl	100.00% <ø> (ø)
src/basekernels/sm.jl	92.30% <91.66%> (-7.70%)	:arrow_down:
src/utils.jl	89.87% <0.00%> (-1.27%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cbbf1d4...d3f2d92. Read the comment docs.

[st--]

I did not go through the code/maths in detail so I can't comment on that, but I trust you in having fixed it.

Still needs a version bump.

Do you think it'd be worthwhile to add tests for the methods not yet covered ? (Not gonna complain if you say no not worth it)