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

move to KernelFunctions style literate.jl #185

Merged
merged 9 commits into from
Jul 20, 2021
Merged

move to KernelFunctions style literate.jl #185

merged 9 commits into from
Jul 20, 2021

Conversation

st--
Copy link
Member

@st-- st-- commented Jul 19, 2021

Copying over the new notebooks setup from KernelFunctions.jl with independent environments for each example.

@st-- st-- requested review from devmotion, willtebbutt and theogf July 19, 2021 11:53
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
StatsFuns = "4c63d2b9-4356-54db-8cca-17b64c39e42c"

[compat]
Copy link
Member Author

Choose a reason for hiding this comment

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

NB- KernelFunctions.jl includes a line julia = "1.3", do we want to add a line like that here, or remove it there, and should it be 1.3 or just julia = "1", or julia = "1.6"?

Copy link
Member

Choose a reason for hiding this comment

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

It should be 1.3 for the main project.toml. It means we do not support lower versions of Julia.
For the docs we could add it, but it's not particularly necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I would personally prefer to not include a julia bound, and let the examples inherit it from whatever the bound the bound is on the current version of AbstractGPs.

# [Literate.jl](https://github.com/fredrikekre/Literate.jl) from the
# [Julia source file](@__REPO_ROOT_URL__/examples/@__NAME__/script.jl).
#md # The corresponding notebook can be viewed in [nbviewer](@__NBVIEWER_ROOT_URL__/examples/@__NAME__.ipynb).*
#nb # The rendered HTML can be viewed [in the docs](https://juliagaussianprocesses.github.io/AbstractGPs.jl/dev/examples/@__NAME__/).*
Copy link
Member Author

Choose a reason for hiding this comment

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

The one line changed from KernelFunctions.jl


# Print `@debug` statements (https://github.com/JuliaDocs/Documenter.jl/issues/955)
if haskey(ENV, "GITHUB_ACTIONS")
ENV["JULIA_DEBUG"] = "Documenter"
Copy link
Member Author

Choose a reason for hiding this comment

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

Now handled by .github/workflows/docs.yml

docs/make.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated
Comment on lines 72 to 77
strict=true,
checkdocs=:exports,
doctestfilters=[
r"{([a-zA-Z0-9]+,\s?)+[a-zA-Z0-9]+}",
r"(Array{[a-zA-Z0-9]+,\s?1}|Vector{[a-zA-Z0-9]+})",
r"(Array{[a-zA-Z0-9]+,\s?2}|Matrix{[a-zA-Z0-9]+})",
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's see if this works

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2021

Codecov Report

Merging #185 (9d0c5f9) into master (04e4c53) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #185   +/-   ##
=======================================
  Coverage   97.98%   97.98%           
=======================================
  Files          10       10           
  Lines         348      348           
=======================================
  Hits          341      341           
  Misses          7        7           

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 04e4c53...9d0c5f9. Read the comment docs.

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.

This is fantastic -- thanks for sorting it out.

The Manifest for the example needs updating -- presently it assumes that AbstractGPs is located at .. -- the fix is just to dev ../.. to update the Manifest.

Additionally, the optimisation example at the end of the script seems to be a bit unstable -- I was getting cholesky errors when I ran it locally. Would you mind adding some jitter to the pseudo-points while we're here? Specifically, making line 395 something like

        return -elbo(fx, y, f(logistic.(params[3:end]), 1e-6))

fixes the problem for me.

docs/make.jl Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
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.

I'm happy with this now. With our current settings I don't believe you'll be able to merge until we resolve the broken AD stuff though, which is annoying.

edit: I was getting confused and thought this was in KernelFunctions. Ignore my AD-related comments.

@st-- st-- merged commit 8852b75 into master Jul 20, 2021
@st-- st-- deleted the st/literate_jl branch July 20, 2021 21:44
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