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

Add SparseFiniteGP type and associated functionality #136

Merged
merged 15 commits into from
Oct 14, 2020

Conversation

ElOceanografo
Copy link
Contributor

This addresses #134, adding a SparseFiniteGP type that wraps two GPs for the observation and inducing points and can be plugged into Turing. The PR contains constructors and basic methods for the new type, mostly falling through to the underlying FiniteGP for the observation points. It provides a logpdf function that wraps the preexisting elbo method for sparse GPs, and also provides a convenience constructor for PseudoObs so that the following "just works":

x = 0:10
xu = 0:2:10
f = GP(Matern32(), GPC())
fx = SparseFiniteGP(f, x, xu)
y = rand(fx)
fpost = f | (fx  y)

There are a couple of remaining issues/questions:

  • I thought of defining a convenience constructor so you could define a SparseFiniteGP by doing fx = f(x, xu) (using the variables from the example above). However, this conflicts with this constructor for FiniteGP, where the second AbstractArray in the function signature is interpreted as the diagonal of the observation covariance matrix. I like that compact definition, but using it would likely mean a breaking change (i.e. requiring users write f(x, Diagonal(σ²)) instead of f(x, σ²)).
  • I decided to make cov(fx::SparseFiniteGP) throw an error instead of calculating the covariance of the observation GP. This follows behavior in base for e.g. inv(A::SparseArrays.AbstractSparseMatrixCSC). The use case for a sparse approximation is by default one where you don't want to or can't use the full covariance matrix, so I think this is the safe and fast option for this behavior. If the user really wants the dense covariance matrix, she can do cov(f.fobs).

Comments on code, tests, organization, or style welcome...

- SparseFiniteGP type, constructors, basic methods
- Convenience methods for inference using sparse GP
- Basic tests
@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.06%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   82.37%   82.43%   +0.06%     
==========================================
  Files          26       27       +1     
  Lines         749      763      +14     
==========================================
+ Hits          617      629      +12     
- Misses        132      134       +2     
Impacted Files Coverage Δ
src/Stheno.jl 100.00% <ø> (ø)
src/approximate_inference.jl 85.71% <85.71%> (ø)

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 80d3869...472f338. 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 looking really good, thanks very much for the contribution -- I'm happy with the overall design, except perhaps one of the constructors that I've left a specific note on.

All other comments are essentially style-related.

src/approximate_inference.jl Outdated Show resolved Hide resolved
src/approximate_inference.jl Outdated Show resolved Hide resolved
src/approximate_inference.jl Outdated Show resolved Hide resolved
test/gp/sparse_gp.jl Outdated Show resolved Hide resolved
test/gp/sparse_gp.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
src/approximate_inference.jl Outdated Show resolved Hide resolved
test/gp/sparse_gp.jl Outdated Show resolved Hide resolved
test/gp/sparse_gp.jl Outdated Show resolved Hide resolved
src/approximate_inference.jl 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.

This is looking great -- I think we're nearly there now.

Just some docs needed now, along with a couple of other very minor things.

src/approximate_inference.jl Outdated Show resolved Hide resolved
test/approximate_inference.jl Outdated Show resolved Hide resolved
src/approximate_inference.jl 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.

LGTM! Just needs a patch bump to 0.6.15 since this doesn't break any existing functionality. I'll make a release once that's done and tests have passed.

Thanks very much for this contribution!

@ElOceanografo
Copy link
Contributor Author

To clarify, do you mean I should update the version number in Project.toml?

@willtebbutt
Copy link
Member

To clarify, do you mean I should update the version number in Project.toml?

Yes please :)

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.

2 participants