-
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
Example notebooks #234
Example notebooks #234
Conversation
…tions.jl into st/examples
…tions.jl into st/examples
…tions.jl into st/examples
Regarding the general structure, I think we should copy the configuration in AbstractGPs. I assume it would also make sense to move examples that are based or require AbstractGPs to the AbstractGPs repo. |
Generally, I think it would be nice to use a separate environment and hence a separate Project.toml + Manifest.toml for probably each of the examples such that they are easily reproducible and the dependencies are separated clearly. One would have to change this in AbstractGPs as well. |
using Flux | ||
using Distributions, LinearAlgebra | ||
using Plots | ||
using ProgressMeter | ||
using AbstractGPs |
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.
IMO the example should be moved to AbstractGPs.
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.
Happy to move this one, I think it's a bit less about kernels themselves. @theogf as this was yours, would like to hear what you think before removing it here ?
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 replied more generally on the main comments.
@@ -0,0 +1,90 @@ | |||
# -*- coding: utf-8 -*- |
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.
# -*- coding: utf-8 -*- |
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.
It's from jupytext. I'd like to see if it's possible to get Literate.jl and jupytext to play along 100% instead of just 98%... fredrikekre/Literate.jl#131
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.
Even if it is supported, I would prefer the standard Literate syntax. Otherwise one either ends up with a mix of different styles or has to pay attention to use the non-standard jupytext formatting (at least non-standard in this environment). To me it seems this would add more confusion but not provide any benefits. If you want to play around and tune the code, I can recommend VSCode or the outdated Juno environment which support executing single blocks and lines of literate files automatically in the REPL.
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.
but not provide any benefits
We can discuss trade-offs, but I don't think this claim is fair; I've explained my motivation on the Literate.jl issue I linked. Having the example notebooks in a format that can directly be edited interactively in jupyter notebooks makes it easier to contribute notebooks for people who are comfortable with that workflow (like me, but it's a widely popular workflow), without requiring people to use VSCode (which is a lot harder to get into than firing up a jupyter notebook).
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.
My impression is that in the Julia community the common workflow for writing Literate or Weave documents is to work with Juno or VSCode, to copy and run code in a REPL on the side, or rebuild the output with Literate and Weave intermittently. I think it's fine to support additional workflows but I think
- it should be officially support by Literate and Weave
- it should not impact the other apparently more common scenarios.
From the issue I get the impression that there won't be an official support for jupytext in the near future. And even if it is added, the second point seems still problematic. It seems jupytext requires a different syntax that deviates from the official syntax for Literate and Weave - if someone edits the document with jupytext the syntax of the document would have to change and so we might end up with a mix of the regular and the jupytext syntax. This seems confusing.
# | ||
# The kernels defined in this package can also be used to specify the | ||
# covariance of a Gaussian process prior. | ||
# A Gaussian process (GP) is defined by its mean function $m(\cdot)$ and its covariance function or kernel $k(\cdot, \cdot')$: |
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 have never seen the notation \cdot'
, IMO k(\cdot, \cdot)
already indicates that k
is a function of two distinct arguments.
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.
# A Gaussian process (GP) is defined by its mean function $m(\cdot)$ and its covariance function or kernel $k(\cdot, \cdot')$: | |
# A Gaussian process (GP) is defined by its mean function $m(x)$ and its covariance function or kernel $k(x, x')$: |
About the GP example(s) here are my thoughts.
|
target_f(x::AbstractArray) = target_f(first(x)) | ||
y_train = target_f.(x_train) + randn(N) * noise | ||
x_test = collect(eachrow(range(xmin, xmax; length=200))) # Testing dataset | ||
spectral_mixture_kernel() |
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.
Should this be removed?
# | ||
# The kernels defined in this package can also be used to specify the | ||
# covariance of a Gaussian process prior. | ||
# A Gaussian process (GP) is defined by its mean function $m(\cdot)$ and its covariance function or kernel $k(\cdot, \cdot')$: |
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.
# A Gaussian process (GP) is defined by its mean function $m(\cdot)$ and its covariance function or kernel $k(\cdot, \cdot')$: | |
# A Gaussian process (GP) is defined by its mean function $m(x)$ and its covariance function or kernel $k(x, x')$: |
Regarding the AbstractGPs discussion, I still think that examples that use AbstractGPs should be added to the other repo or not use AbstractGPs if it is not the main focus. Mainly, because examples that use AbstractGPs would be interesting for users of AbstractGPs but would not show up when they go to the documentation of the package. While probably kernels are most commonly used with GPs, it seems more natural and easier to find for users if the examples in KernelFunctions are focused on, well, kernel functions and the ones in AbstractGPs on GPs. |
I get your point but it seems also absurd to restrict ourselves to only non-GP examples. If this is a problem this is where the general website we set up could be useful. We could have these more complex examples there and mention them in the docs of each package. |
Hi, it's been a while but I'd still like to get more & better examples into the KernelFunctions docs :) what do you think would be the best way forward ? |
Nice to see you back! The main problem is the discussion on which examples should belong here, or AbstractGPs. TBH I think it's a bit pointless to discuss this before the tutorials are even finished 😆 . We should first finish them and then decide where they should go I guess you can start by adding the review suggestions and try to get the Documentation action to pass so we can have previews :) |
Co-authored-by: David Widmann <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]>
* initial version of gaussian process priors example from st/examples (#234) * include Literate in dependencies * clean up GP prior script * update * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * add heading * cell break markers for VSCode * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * clean up plots, add more kernels * Update examples/gaussian-process-priors/script.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * minor fixes * two xrefs for kernel cut plots * fix plotting * more plotting changes * plotting cleanup Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* initial version of training kernel parameters example from st/examples (#234) * update script * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix out-of-domain initial value * initial version of training kernel parameters example from st/examples (#234) * update script * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix out-of-domain initial value * Add ParameterHandling * Extend example * Failed attempts with default Flux opt * Add Flux.destructure example * Add some nothing * Squashed commit of the following: commit f9bbd84 Author: st-- <[email protected]> Date: Fri Jan 28 09:11:50 2022 +0100 make nystrom work with AbstractVector (#427) * make nystrom work with AbstractVector * add test * Update test/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * patch bump * Update test/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review * deprecate * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: Théo Galy-Fajou <[email protected]> * Update src/approximations/nystrom.jl Co-authored-by: Théo Galy-Fajou <[email protected]> * Update src/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> commit d1c68a9 Author: st-- <[email protected]> Date: Thu Jan 13 22:33:43 2022 +0100 fix Distances compat (#423) * CompatHelper: bump compat for Distances to 0.10 for package test, (keep existing compat) * try out Theo's fix * fix test compat * use ForwardDiff for chain rule test of SqMahalanobis * test on 1.4 instead of 1.3 - see if the chainrules test passes there * revert version branch * revert to 1.3 * test_broken for older Julia versions Co-authored-by: CompatHelper Julia <[email protected]> commit 93d33c2 Author: st-- <[email protected]> Date: Wed Jan 12 14:11:14 2022 +0100 fix figure & cleanup (#422) * fix figure & cleanup * bump LIBSVM compat & Manifest * improve writing, replaces #321 commit 40cb59e Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed Jan 12 09:39:01 2022 +0200 CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) (#367) * CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) * ] up Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: st-- <[email protected]> commit 7204529 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue Jan 11 18:37:23 2022 +0200 CompatHelper: bump compat for Kronecker to 0.5 for package test, (keep existing compat) (#366) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: st-- <[email protected]> commit 924925d Author: st-- <[email protected]> Date: Tue Jan 11 16:26:02 2022 +0100 switch SVM example to half-moon dataset (#421) commit 992b665 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri Dec 24 12:18:56 2021 +0200 CompatHelper: bump compat for SpecialFunctions to 2 for package test, (keep existing compat) (#412) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> Co-authored-by: st-- <[email protected]> commit 04fa7f7 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu Dec 23 13:33:59 2021 +0200 CompatHelper: bump compat for SpecialFunctions to 2, (keep existing compat) (#411) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> Co-authored-by: st-- <[email protected]> commit c0fc3e1 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu Dec 23 01:10:40 2021 +0200 CompatHelper: add new compat entry for Compat at version 3 for package test, (keep existing compat) (#418) Co-authored-by: CompatHelper Julia <[email protected]> commit 05fe340 Author: st-- <[email protected]> Date: Tue Dec 21 00:49:37 2021 +0200 use only() instead of first() (#403) * use only() instead of first() for 1-"vectors" that were for the benefit of Flux * fix one test that should not have worked as it was * add missing scalar Sinus constructor commit 2d17212 Author: st-- <[email protected]> Date: Sat Dec 18 23:43:30 2021 +0200 Zygote AD failure workarounds & test cleanup (#414) Zygote AD failures: * revert #409 (test_utils workaround for broken Zygote - now working again) * disable broken Zygote AD test for ChainTransform Improved tests: * finer-grained testsets * add missing test cases to test_AD * replace test_FiniteDiff with test_AD(..., :FiniteDiff, ...) * remove code duplication commit 3c49949 Author: Théo Galy-Fajou <[email protected]> Date: Wed Nov 24 18:32:19 2021 +0100 Fix typo in valid_inputs error (#408) * Fix typo in valid_inputs error * Update src/utils.jl Co-authored-by: David Widmann <[email protected]> Co-authored-by: David Widmann <[email protected]> commit 9955044 Author: st-- <[email protected]> Date: Wed Nov 24 18:55:18 2021 +0200 Fix for Zygote 0.6.30 breaking our tests (#409) * restrict Zygote to <0.6.30 * revert Zygote test restriction and add finer-grained testset * Update test/utils.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * revert testset * mark test_broken * Use `@test_throws` instead of `@test_broken` Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> commit 33d64d1 Author: Théo Galy-Fajou <[email protected]> Date: Thu Nov 4 14:23:57 2021 +0100 Add benchmarking CI (#399) * Add benchmark file * delete old benchmarks * Add github action * Add Project * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Missing end of line Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit 360ce10 Author: David Widmann <[email protected]> Date: Tue Nov 2 11:09:58 2021 +0100 Update docstring of `GibbsKernel` (#395) * change title * Update manifest * Formatter * Some small updates * Add BenchmarkTools * Move Benchmarks to the correct manifest * Formatter * Fix missed comment * Add ParameterHandling too * Change gif embed * Forgot # * Formatter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Compat and gif kwargs * , for ; * different commit msg * Some more clarification * Change display * Final changes * Apply suggestions from code review Co-authored-by: st-- <[email protected]> * Apply suggestions from code review Co-authored-by: st-- <[email protected]> * Address comments * Missed one * Manifest issue * Apply suggestions from formatter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Local formatter * Delete loss description. Co-authored-by: st-- <[email protected]> * format Co-authored-by: ST John <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I've played around a bit with the example notebooks. This isn't intended for merging as-is (if anything, it probably makes sense to split it into one PR per notebook), just to see where I've gone so far - happy to hear your thoughts! Heavily builds on top of @theogf's examples branch.
Following #303 and #314, examples are now split into separate PRs: