Updated Train- kernel-parameters example

[Crown421]

Summary
Picking up #317, and adding more to it. The commits might a bit of a mess, due to some unfortunate experiments with the github desktop app.

Proposed changes

What alternatives have you considered?

[codecov]

Codecov Report

Merging #430 (04a5d98) into master (aa9ac9f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #430   +/-   ##
=======================================
  Coverage   93.13%   93.13%           
=======================================
  Files          52       52           
  Lines        1252     1252           
=======================================
  Hits         1166     1166           
  Misses         86       86           

---

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 aa9ac9f...04a5d98. Read the comment docs.

[Crown421]

I didn't build the documentation locally, is there a way to preview from here?

[devmotion]

There's a preview for PRs that are not from forks, the link to the deployed docs will show up among the Github actions (see e.g. #429). However, currently there's no link since the documentation is not built successfully.

[devmotion]

The problem is that BenchmarkTools is not installed: https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/runs/4983109277?check_suite_focus=true#step:5:31 Is it actually necessary to perform benchmarks?

[Crown421]

The problem is that BenchmarkTools is not installed: https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/runs/4983109277?check_suite_focus=true#step:5:31 Is it actually necessary to perform benchmarks?

Ah, that makes alot of sense. Not strictly necessary, but there are interesting things to see.

[st--]

Two questions:
a) why not

Suggested change

	return sum(abs2, y_train - ŷ) + exp(θ[4]) * norm(ŷ)
	return norm(y_train - ŷ) + exp(θ[4]) * norm(ŷ)

b) can you explain where the second term comes from? Just thinking about "parameter regularisation" might make one think it ought to be norm(\theta).

[Crown421]

This part is from your original example, I am honestly not sure.

[st--]

which in turn was part of a previously existing example 😅 @theogf @willtebbutt how would you address this (comment to add / equation to fix / ...)?

[st--]

why not

Suggested change

	θ = log.([1.1, 0.1, 0.01, 0.001]) # Initial vector
	θ = log.(ones(4)) # Initial vector

as you had before?
(alternatively, use the values from here above as well?)

[st--]

NB- what do you think of commenting explicitly on this being a mutable vector that will be changed in-place by the optimiser ?

[Crown421]

I think this also comes from the original example, can experiment with setting it to ones.

[st--]

it's probably a reasonable setting to end up in the right local optimum - maybe simpler to just change the very first ones(4) to also be [1.1, 0.1, 0.01, 0.001] instead?:)

[st--]

why do you introduce a new optt? (I'm assuming it's due to the internal optimizer state that you don't want to affect for the actual optimisation loop, but this might not be clear to the casual reader!)

As it's within a let block, you could just call it opt and it wouldn't get outside the scope of the let block.
It might then also be cleaner if you move the opt = ... definition from line 80 to the section below (line 94 following) where it's actually used.

[Crown421]

I tried a few things, but there seemed to be some leaking. This was my way of fixing that leakage.
I can take another look.

[st--]

You definitely need to copy the \theta so you don't change the initial values. But

@benchmark let
    θ = log.(ones(4))
    opt = Optimise.ADAGrad(0.5)
    ...
end

should work

[st--]

what's the benefit of this vs. having the output from gif() inline ?

[Crown421]

Locally it didn't display correctly.

[st--]

Huh! alright. Well, as long as it works in the deployed docs it doesn't matter too much one way or another:)

[st--]

might be nice to also comment on it allowing arbitrary nested namedtuples for easily accessing parameters without worrying about where in the flat param vector they are, their constraints, etc...

[st--]

Do you actually need these? I think Literate.jl only splits up code cells at non-code markdown (comments), so all of this would run as a single cell and its return values wouldn't get printed anyways?

[Crown421]

You are probably right, and this is some artifact from me moving things around.

[st--]

not clear to me, how would you combine the two? I had thought it was either-or..

[Crown421]

My impression was that one could do a kernelc \circ flatten (or unflatten) construction.

[st--]

if you think it's something one would actually want to do in practice, it'd be great to expand on it a bit more, but otherwise I would prefer removing the reference so as to not confuse people (like myself reading it now 😅 )

[st--]

Suggested change

	# This returns the `trainable` parameters of the kernel and a function to reconstruct the kernel.
	# This returns the "trainable" parameters of the kernel and a function to reconstruct the kernel.

or did you mean for trainable to be typeset as code ?

[Crown421]

as typeset in code, for Flux.trainable (which I understand from another PR might be going away, but still).

[st--]

Flux.trainable isn't exported (and barely documented - the only reference I could find was in https://github.com/FluxML/Flux.jl/blob/4a3483efd8e13437b3d86371723c977fb61c2793/docs/src/models/advanced.md, but that doesn't seem relevant here), so as it is it seems to me to be rather confusing. Maybe

Suggested change

	# This returns the `trainable` parameters of the kernel and a function to reconstruct the kernel.
	# This returns the trainable `params` of the kernel and a function to reconstruct the kernel.

?
or just

Suggested change

	# This returns the `trainable` parameters of the kernel and a function to reconstruct the kernel.
	# This returns the trainable parameters of the kernel and a function to reconstruct the kernel from these parameters.

[st--]

what's the p? what is it needed for? what does it do?

[Crown421]

Probably not needed anymore. Initially I wanted to show that kernelc(p) == kernel, but due to a mutability issue, this does not evaluate as true, despite them being == in almost all respects.

[st--]

Thanks, I think this is a useful addition to make clear that you can train kernel parameters (and how to do so); I've left minor comments as github suggestions, there's a few other comments that would be good if you could address before it's ready to merge. Let me know if anything doesn't make sense!

[st--]

@Crown421 let me know once you've addressed my remaining comments (either through changes or by replying what you doesn't make sense to you / you disagree with:)), and I'll finish off the review!

[Crown421]

@st-- I think I addressed all comments now. Apologies for the delay, I also managed to catch covid since we spoke.

[st--]

This looks like a very helpful addition, thanks for contributing!

there's just one remaining comment on the loss comment:) let me know what you want to do with that, then we can merge it