Clarify API for GP approximations #361

st-- · 2023-04-25T14:44:22Z

Summary
AbstractGPs.jl should define the API we expect across the ecosystem. Currently, this is not the case for GP approximations, even though they're already introduced in this package (VFE for sparse GPR).

Proposed changes
Expose posterior and approx_log_evidence as part of the API with nice generic docstrings explaining their purpose.

Related issues
Resolves #221

Also see #223, #241, #318, maybe #319

What alternatives have you considered?
Status quo: some definitions in ApproximateGPs.jl, but people not realizing that you could just use a three-parameter form for posterior, leading to more reimplementation of base FiniteGPs than would be necessary (e.g. https://github.com/SebastianCallh/IterGP.jl/blob/AbstractGPs-interface/src/gp.jl)

…ence`

codecov · 2023-04-25T14:46:07Z

Codecov Report

Patch coverage: 87.50% and project coverage change: -0.74 ⚠️

Comparison is base (3e5f0a5) 97.63% compared to head (501b702) 96.89%.

❗ Current head 501b702 differs from pull request most recent head f260171. Consider uploading reports for the commit f260171 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   97.63%   96.89%   -0.74%     
==========================================
  Files          10       10              
  Lines         380      387       +7     
==========================================
+ Hits          371      375       +4     
- Misses          9       12       +3

Impacted Files	Coverage Δ
src/abstract_gp.jl	`100.00% <ø> (ø)`
src/exact_gpr_posterior.jl	`92.50% <0.00%> (-7.50%)`	⬇️
src/sparse_approximations.jl	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Crown421 · 2023-04-25T14:58:49Z

Should there be an AbstractGPApproximation type or so? That other packages can/ should use?

src/sparse_approximations.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

st-- · 2023-04-25T15:10:32Z

Should there be an AbstractGPApproximation type or so? That other packages can/ should use?

Hm... where would that be useful? Is there anything that you would be able to do with any approximation, without knowing what approximation it is (i.e. it'd be necessary as a common base type in the hierarchy)? Or just as a "type hint" (i.e. it might be better to just improve the docs)?

st-- · 2023-04-25T15:13:40Z

Open question from #221 (comment):

should we add a struct ExactGP end "approximation" type to support a consistent API regardless of whether one is using exact GPR or a sparse approximation such as VFE/DTC?

Crown421 · 2023-04-25T15:29:50Z

Open question from #221 (comment):

should we add a struct ExactGP end "approximation" type to support a consistent API regardless of whether one is using exact GPR or a sparse approximation such as VFE/DTC?

I think this makes sense to me, with appropriate aliases for the current two argument form.

Re the above, I was thinking about a type hint, need to think about whether such a type might also be useful elsewhere.

Crown421 · 2023-04-25T16:03:38Z

One way I can think of where a AbstractApproximation type might be useful in starting a more general hierarchy.
Next level down would be the methods mentioned here: JuliaGaussianProcesses/ApproximateGPs.jl#84, might be some supertype of VFE, DTC (possibly others in the future like FITC)

…tractGPs.jl into st/3posterior

theogf · 2023-04-25T16:25:11Z

src/exact_gpr_posterior.jl

@@ -3,6 +3,12 @@ struct PosteriorGP{Tprior,Tdata} <: AbstractGP
    data::Tdata
 end

+struct ExactGP end
+
+posterior(::ExactGP, fx::FiniteGP, y::AbstractVector{<:Real}) = posterior(fx, y)


I would have gone the other way around?

But maybe that's too complicated...

I find this order of arguments quite intuitive.

Sorry I meant that the implemented definition would be posterior(::ExactPosterior, gp, y) and that posterior(gp, y) would default to it.

Does it make any difference in practice? (I kinda like it as it is but that might just be status-quo bias too...)

In practice I think the answer is no.
Within the code, there might be something to be said for consistency. Every posterior is defined with the 3-argument form, but the exact one gets a special alias.

To be fair, there is only one posterior. Shouldn't VFE and DTC dispatched on approx_posterior?

@theogf what do you mean? there is no approx_posterior method..

haha somehow in my mind there was a approx_posterior method. So yeah then it's back to

Should we have a unique 3-args posterior API (where the 2-args default to ExactInference) ?

Should we just have 2-args methods and have the GP wrapped (like in ApproximateGPs) ?

ApproximateGPs has the 3-args posterior though, no wrapping?

src/sparse_approximations.jl

test/sparse_approximations.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

st-- · 2023-04-26T09:33:40Z

src/sparse_approximations.jl

+"""
+    DTC(fz::FiniteGP)
+
+Similar to `VFE`, but uses a different objective for `approx_log_evidence`.


Could maybe do with a better docstring but then it needs to be sorted out more thoroughly anyways (see #309) and I can't think of what it should be right now, so would leave that for some other PR/person/time...

st-- · 2023-04-26T09:34:42Z

test/sparse_approximations.jl

+        @testset "approx_log_evidence" begin
+            x = collect(Float64, range(-1.0, 1.0; length=N_cond))
+            f = GP(SqExponentialKernel())
+            fx = f(x, 0.1)
+            y = rand(rng, fx)
+
+            # Ensure that the elbo/dtc objective is close to the logpdf when appropriate.
+            @test approx_log_evidence(ApproxType(f(x)), fx, y) isa Real
+            @test approx_log_evidence(ApproxType(f(x)), fx, y) ≈ logpdf(fx, y)
+
+            if ApproxType === VFE
+                @test elbo(ApproxType(f(x)), fx, y) ==
+                    approx_log_evidence(ApproxType(f(x)), fx, y)
+                @test elbo(ApproxType(f(x .+ randn(rng, N_cond))), fx, y) < logpdf(fx, y)
+            end
+        end


this is the main change

…tractGPs.jl into st/3posterior

st-- · 2023-04-27T09:31:34Z

Hello! What do you think needs doing/addressing/... before merging this ?

st-- · 2023-04-27T09:32:33Z

src/sparse_approximations.jl

+        _update_approx(f_post_approx.approx, fz_new), f_post_approx.prior, cache
+    )
 end

+_update_approx(vfe::VFE, fz_new::FiniteGP) = VFE(fz_new)
+_update_approx(dtc::DTC, fz_new::FiniteGP) = DTC(fz_new)


is this the right way to handle this? if anyone can think of a better approach please say:)

src/exact_gpr_posterior.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

API docstrings for the base forms of posterior and `approx_log_evid…

12a0066

…ence`

st-- added 2 commits April 25, 2023 18:05

clarify docstring

401a3dd

DTC as separate type

6af01e2

github-actions bot reviewed Apr 25, 2023

View reviewed changes

src/sparse_approximations.jl Outdated Show resolved Hide resolved

src/sparse_approximations.jl Outdated Show resolved Hide resolved

src/sparse_approximations.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

73e70d9

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

ExactGP for fallback forms

3d30d0b

st-- requested review from theogf and willtebbutt April 25, 2023 16:03

Merge branch 'st/3posterior' of github.com:JuliaGaussianProcesses/Abs…

f0e4d10

…tractGPs.jl into st/3posterior

theogf reviewed Apr 25, 2023

View reviewed changes

st-- commented Apr 25, 2023

View reviewed changes

src/sparse_approximations.jl Outdated Show resolved Hide resolved

st-- added 3 commits April 25, 2023 22:53

deprecations

101e351

fix doctest

e50cd91

update tests

7d39319

github-actions bot reviewed Apr 26, 2023

View reviewed changes

test/sparse_approximations.jl Outdated Show resolved Hide resolved

Update test/sparse_approximations.jl

9de34ce

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

st-- commented Apr 26, 2023

View reviewed changes

st-- added 3 commits April 26, 2023 12:47

rename ExactGP -> ExactGPR

b3bd78d

Merge branch 'st/3posterior' of github.com:JuliaGaussianProcesses/Abs…

dfacf2a

…tractGPs.jl into st/3posterior

Merge branch 'master' into st/3posterior

6f14e02

st-- commented Apr 27, 2023

View reviewed changes

st-- requested a review from theogf April 28, 2023 07:17

Merge branch 'master' into st/3posterior

806f633

st-- commented May 2, 2023

View reviewed changes

src/exact_gpr_posterior.jl Outdated Show resolved Hide resolved

rename ExactGPR -> ExactInference

45c2d6e

github-actions bot reviewed May 2, 2023

View reviewed changes

src/exact_gpr_posterior.jl Outdated Show resolved Hide resolved

Update src/exact_gpr_posterior.jl

501b702

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

willtebbutt approved these changes May 8, 2023

View reviewed changes

bump version to 0.5.17

f260171

st-- merged commit 1b7c135 into master May 8, 2023

st-- deleted the st/3posterior branch May 8, 2023 13:43

st-- mentioned this pull request May 9, 2023

build on approx_log_evidence introduced in AbstractGPs 0.5.17 JuliaGaussianProcesses/ApproximateGPs.jl#145

Open

st-- mentioned this pull request Sep 23, 2023

fix deprecation of dtc #380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify API for GP approximations #361

Clarify API for GP approximations #361

st-- commented Apr 25, 2023 •

edited

Loading

codecov bot commented Apr 25, 2023 •

edited

Loading

Crown421 commented Apr 25, 2023

st-- commented Apr 25, 2023

st-- commented Apr 25, 2023

Crown421 commented Apr 25, 2023

Crown421 commented Apr 25, 2023 •

edited

Loading

theogf Apr 25, 2023

Crown421 Apr 25, 2023

theogf Apr 25, 2023

st-- Apr 26, 2023

Crown421 Apr 26, 2023

theogf Apr 26, 2023

st-- Apr 27, 2023

theogf Apr 27, 2023

st-- Apr 28, 2023

st-- Apr 26, 2023

st-- Apr 26, 2023

st-- commented Apr 27, 2023

st-- Apr 27, 2023

Clarify API for GP approximations #361

Clarify API for GP approximations #361

Conversation

st-- commented Apr 25, 2023 • edited Loading

codecov bot commented Apr 25, 2023 • edited Loading

Codecov Report

Crown421 commented Apr 25, 2023

st-- commented Apr 25, 2023

st-- commented Apr 25, 2023

Crown421 commented Apr 25, 2023

Crown421 commented Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st-- commented Apr 27, 2023

Choose a reason for hiding this comment

st-- commented Apr 25, 2023 •

edited

Loading

codecov bot commented Apr 25, 2023 •

edited

Loading

Crown421 commented Apr 25, 2023 •

edited

Loading