Skip to content
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 notebook: Gaussian process priors #316

Merged
merged 23 commits into from
Jul 9, 2021

Conversation

st--
Copy link
Member

@st-- st-- commented Jun 30, 2021

No description provided.

@st-- st-- mentioned this pull request Jun 30, 2021
@devmotion
Copy link
Member

Some high-level comment/idea: Since this is not how we want users to create or work with Gaussian processes, I think it would be nice to have to both the "naive"/basic KernelFunctions approach and the AbstractGP-based implementation together in an example to avoid any incorrect impressions and so that users know how they should do it. However, if both AbstractGPs and KernelFunctions are part of the example, I think the example fits better into either the AbstractGPs repo or the JuliaGaussianProcesses webpage. Otherwise we use packages that depend on KernelFunctions and hence introduce a circular dependency that makes it difficult to update the example without delay if there are breaking changes in KernelFunctions.

@st--
Copy link
Member Author

st-- commented Jun 30, 2021

How about leaving this example in KernelFunctions but clarifying all you're saying in an intro paragraph and linking to the corresponding "proper" example in AbstractGPs docs?

@devmotion
Copy link
Member

This should definitely be done if the example is added to KernelFunctions. My worry is that people do not notice and read the paragraph and just copy whatever code there is on this page about Gaussian processes. I assume this is less of a problem with the "correct" code is on the same page as well.

@st--
Copy link
Member Author

st-- commented Jun 30, 2021

My worry is that people do not notice and read the paragraph and just copy whatever code there is on this page about Gaussian processes.

It might be helpful if you expand on what you're worried about so we can discuss likelihood, impact, and mitigations:)?

@devmotion
Copy link
Member

I am worried that they copy and use the code but are not aware of how one is supposed to work with Gaussian processes.

@willtebbutt
Copy link
Member

willtebbutt commented Jun 30, 2021

In my opinion it would be sufficient to add one of those hint things that you can add in documenter saying something along the lines of "this is an illustrative example, you should probably be using an AbstractGP of some kind or another if you actually want to do something with a GP"

@devmotion
Copy link
Member

On the other hand, we can always link to AbstractGPs or the JuliaGaussianProcesses webpage from the documentation, so the example would still be discoverable from the KernelFunctions docs even if it is part of AbstractGPs or the webpage.

@st-- st-- marked this pull request as ready for review July 2, 2021 07:50
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
st-- and others added 2 commits July 2, 2021 10:58
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
st-- and others added 2 commits July 2, 2021 11:00
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
st-- and others added 3 commits July 2, 2021 12:50
st-- and others added 2 commits July 8, 2021 11:57
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
examples/gaussian-process-priors/script.jl Outdated Show resolved Hide resolved
willtebbutt
willtebbutt previously approved these changes Jul 8, 2021
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. Just a quick clarification.

examples/gaussian-process-priors/script.jl Show resolved Hide resolved
theogf
theogf previously approved these changes Jul 9, 2021
@st-- st-- dismissed stale reviews from theogf and willtebbutt via 10d443d July 9, 2021 10:48
@st-- st-- merged commit 58cb069 into master Jul 9, 2021
@st-- st-- deleted the st/examples--gaussian-process-priors branch July 9, 2021 11:30
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.

4 participants