SparseVariationalApproximation cleanup #86

st-- · 2021-12-22T22:56:31Z

This PR unifies the code for Centered and NonCentered versions of SparseVariationalApproximation. It might marginally change numeric behaviour. It might marginally make it slower. On the plus side, it significantly removes the amount of code, which should make it easier to actually improve it overall.

(Motivation: I actually tried out timing ApproximateGPs vs GPflow, and when I saw how much worse it is, I started looking more into the code ... this PR simplifies the code quite a bit, but it actually seems to make it slightly slower. I think the main issue is that there's huge amounts of allocations (hundreds of megabytes for N=5000, M=500 in each iteration). Not sure how best to proceed, but it is a rather unsatisfactory state of things :/)

src/sparse_variational.jl

st-- · 2021-12-22T22:58:50Z

src/sparse_variational.jl

-)
-    Cux = cov(f.prior, inducing_points(f), x)
-    D = f.data.Kuu.L \ Cux
-    μ = Cux' * f.data.α


This (and line 160 below in mean_and_var) were missing the prior mean.

st-- · 2021-12-22T22:59:42Z

src/sparse_variational.jl

    m_ε = mean(sva.q)
-    return (tr(cov(sva.q)) + m_ε'm_ε - length(m_ε) - logdet(post.data.C_ε)) / 2


This was unnecessarily making a copy (see JuliaStats/Distributions.jl#1373).

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

…proximateGPs.jl into st/sva_cleanup

willtebbutt · 2021-12-23T15:19:43Z

I'm not too surprised that this isn't hugely performant at the minute, given how little attention we've paid to the performance of this package so far. How would you like to proceed @st-- ? Ignore performance in this PR and look at it in some subsequent work?

st-- · 2021-12-23T18:24:14Z

I'm not too surprised that this isn't hugely performant at the minute, given how little attention we've paid to the performance of this package so far. How would you like to proceed @st-- ? Ignore performance in this PR and look at it in some subsequent work?

I'm open to discussion. I do think this PR significantly simplifies the code (and removes a lot of near duplicate code), which will make it easier to change "big things". We might want to split it up into separate methods for Centered/NonCentered again later. But I think for now, this PR makes the package more maintainable. (It also fixes a subtle bug for non-zero mean functions. We could probably do with more tests...).

st-- · 2021-12-23T18:35:30Z

The Laplace test failures are not related to this PR and fixed in #85.

If you have any ideas on the others, help welcome!

test/sparse_variational.jl

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

…proximateGPs.jl into st/sva_cleanup

…eGPs.jl into st/sva_cleanup

codecov · 2022-01-04T17:08:07Z

Codecov Report

Merging #86 (b6d9026) into master (df0010d) will decrease coverage by 0.53%.
The diff coverage is 93.18%.

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
- Coverage   90.13%   89.60%   -0.54%     
==========================================
  Files           4        4              
  Lines         294      279      -15     
==========================================
- Hits          265      250      -15     
  Misses         29       29

Impacted Files	Coverage Δ
src/sparse_variational.jl	`96.20% <93.02%> (-0.64%)`	⬇️
src/utils.jl	`42.85% <100.00%> (+9.52%)`	⬆️

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 df0010d...b6d9026. Read the comment docs.

willtebbutt

I'm happy for this to go in without further review on my part once

my comments have been addressed
the patch version has been bumped

willtebbutt · 2022-01-05T13:11:05Z

src/sparse_variational.jl

-    f::ApproxPosteriorGP{<:SparseVariationalApproximation{NonCentered}}, x::AbstractVector
-)
-    return mean(f.prior, x) + _A(f, x)' * mean(f.approx.q)
+function _A_and_Cux(f, x)


At the risk of being a massive pedant, could we please rename Cux to Czx, i.e. consistently referring to inputs rather than random variables?

willtebbutt · 2022-01-05T13:11:37Z

src/sparse_variational.jl

+)
+    return mean(f.prior, x) + cov(f.prior, x, inducing_points(f)) * f.data.α
+end
+
 # Produces a matrix that is consistently referred to as A in this file. A more descriptive


Maybe update this to reflect the new functionality?

rossviljoen

Very nice PR, thanks!

Just a couple of v minor things

rossviljoen · 2022-01-05T21:36:36Z

src/utils.jl

@@ -11,3 +11,5 @@ end

 _chol_cov(q::AbstractMvNormal) = cholesky(Symmetric(cov(q)))
 _chol_cov(q::MvNormal) = cholesky(q.Σ)
+
+_cov(q::MvNormal) = q.Σ


Suggested change

_cov(q::MvNormal) = q.Σ

_cov(q::AbstractMvNormal) = cov(q)

_cov(q::MvNormal) = q.Σ

Hm, it's kinda a hack in the first place - so I think I'd rather have _cov(q) error when called with anything else, and see why... no?

@st-- I don't get why it should error - shouldn't you just be able to use any AbstractMvNormal for sva.q?

src/sparse_variational.jl

st-- added 2 commits December 22, 2021 22:40

prior KL

c55ef11

unify Centered and NonCentered predictions

fa516c3

github-actions bot reviewed Dec 22, 2021

View reviewed changes

src/sparse_variational.jl Show resolved Hide resolved

src/sparse_variational.jl Outdated Show resolved Hide resolved

st-- commented Dec 22, 2021

View reviewed changes

st-- and others added 3 commits December 23, 2021 00:00

Apply suggestions from code review

4cc3dfa

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

bugfix

96ee796

Merge branch 'st/sva_cleanup' of github.com:JuliaGaussianProcesses/Ap…

a186e7a

…proximateGPs.jl into st/sva_cleanup

st-- added 2 commits December 23, 2021 19:26

fix rename

43ce052

testsets

8b49e75

github-actions bot reviewed Dec 23, 2021

View reviewed changes

test/sparse_variational.jl Outdated Show resolved Hide resolved

test/sparse_variational.jl Outdated Show resolved Hide resolved

st-- and others added 4 commits December 23, 2021 19:43

Apply suggestions from code review

cf0cece

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

bugfix

90704a0

Merge branch 'st/sva_cleanup' of github.com:JuliaGaussianProcesses/Ap…

4fb5ea3

…proximateGPs.jl into st/sva_cleanup

tomls

d075138

st-- marked this pull request as ready for review December 31, 2021 15:08

st-- requested review from willtebbutt and rossviljoen January 4, 2022 15:33

st-- added 2 commits January 4, 2022 17:33

Merge branch 'master' of github.com:JuliaGaussianProcesses/Approximat…

a05edf5

…eGPs.jl into st/sva_cleanup

bugfix

a4acb03

willtebbutt approved these changes Jan 5, 2022

View reviewed changes

st-- added 2 commits January 5, 2022 15:26

update comments

9250eb5

patch bump

a6b67fa

rossviljoen approved these changes Jan 5, 2022

View reviewed changes

st-- added 2 commits January 7, 2022 23:04

consistent naming

34a3b7a

minor cleanup

df308a3

st-- added 3 commits January 7, 2022 23:14

clean up imports

55ece79

clean up imports

69fc92f

formatting

b6d9026

st-- merged commit fee8c3d into master Jan 7, 2022

st-- deleted the st/sva_cleanup branch January 7, 2022 21:29

st-- mentioned this pull request Jan 11, 2022

Support for non-zero prior mean #89

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SparseVariationalApproximation cleanup #86

SparseVariationalApproximation cleanup #86

st-- commented Dec 22, 2021 •

edited

Loading

st-- Dec 22, 2021

st-- Dec 22, 2021

willtebbutt commented Dec 23, 2021

st-- commented Dec 23, 2021 •

edited

Loading

st-- commented Dec 23, 2021

codecov bot commented Jan 4, 2022 •

edited

Loading

willtebbutt left a comment

willtebbutt Jan 5, 2022

willtebbutt Jan 5, 2022

st-- Jan 5, 2022

rossviljoen left a comment

rossviljoen Jan 5, 2022

st-- Jan 7, 2022

rossviljoen Jan 8, 2022

		m_ε = mean(sva.q)
		return (tr(cov(sva.q)) + m_ε'm_ε - length(m_ε) - logdet(post.data.C_ε)) / 2

	_cov(q::MvNormal) = q.Σ
	_cov(q::AbstractMvNormal) = cov(q)
	_cov(q::MvNormal) = q.Σ

SparseVariationalApproximation cleanup #86

SparseVariationalApproximation cleanup #86

Conversation

st-- commented Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willtebbutt commented Dec 23, 2021

st-- commented Dec 23, 2021 • edited Loading

st-- commented Dec 23, 2021

codecov bot commented Jan 4, 2022 • edited Loading

Codecov Report

willtebbutt left a 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

rossviljoen left a 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 Dec 22, 2021 •

edited

Loading

st-- commented Dec 23, 2021 •

edited

Loading

codecov bot commented Jan 4, 2022 •

edited

Loading