test utils revamp #159

willtebbutt · 2020-08-30T18:22:27Z

Introduces and applies a large number of consistency tests.

The advantage of running these tests is that we can be fairly sure that new kernels are at least self-consistent, and fully implement our interface.

The advantage of producing a new TestUtils module is that if someone implements a new kernel outside of KernelFunctions, they can straightforwardly test that the kernel obeys our interface.

This can't be merged until the following PRs are merged:

Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>

sharanry

I was going through this to understand the new testing strategy. Looks great! But, have a few doubts. :)

src/test_utils.jl

src/transform/lineartransform.jl

src/test_utils.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

willtebbutt · 2020-09-21T16:45:38Z

Moved some tests back to test/test_utils.jl because they introduce additional deps, and I would like to avoid this.

willtebbutt · 2020-09-21T20:03:50Z

@theogf could you comment on the GammaRationalQuadraticKernel implementation? It appears to suffer from the same issues that the GammaExponentialKernel had prior to #158 , and was recently mentioned in #170 .

What are your thoughts on how it should be modified? Currently the test produce covariance matrices with negative eigenvalues, so something needs to be modified. Would you mind taking a look?

I believe that it becomes correct if we just use the Euclidean metric, rather than SqEuclidean.

willtebbutt · 2020-09-24T09:24:28Z

@theogf what are your thoughts on this? Note that I've rejected your previous review because I've made changes to RationalQuadratic that need another round of review 🙂

src/test_utils.jl

theogf

I just put a few comments (sorry for the single comments, I messed up).

You really did an amazing titan job! That looks really good!

Also should we have tests on test_utils itself? Making sure that it goes wrong for a wrongly defined kernel?

src/test_utils.jl

src/transform/lineartransform.jl

theogf · 2020-09-24T10:28:23Z

src/test_utils.jl

+    atol=__ATOL,
+)
+    # TODO: uncomment the tests of ternary kerneldiagmatrix.
+


This might be too much but maybe adding some printing for each of the test would give some nice feedback to the user and avoid stalling in Travis?

Hmm I'm reluctant to do this in this PR. The user has plenty of feedback when the tests fail in the way that the test sets are printed, and I personally prefer to minimise output during the running of tests.

test/basekernels/fbm.jl

test/basekernels/gabor.jl

test/kernels/kernelsum.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>

willtebbutt · 2020-09-24T16:07:29Z

For some reason I can't reply to this directly:

Has this issue been solved? That would be great to avoid the allocation

No, unfortunately not. Would you be open to me opening a separate issue about it and pressing ahead with getting this PR merged?

willtebbutt · 2020-09-24T16:52:35Z

Also should we have tests on test_utils itself? Making sure that it goes wrong for a wrongly defined kernel?

Hmmm maybe. As with the above, I wonder whether we could postpone this to a future PR where we introduce some proper test fakes or something? This is already quite large :S

willtebbutt · 2020-09-25T10:47:20Z

Hmmm maybe. As with the above, I wonder whether we could postpone this to a future PR where we introduce some proper test fakes or something? This is already quite large :S

@theogf thoughts on this? (Sorry for being pushy -- just keen to get this merged so that I can press ahead with my Stheno.jl / TemporalGPs.jl integration plan)

theogf · 2020-09-25T10:49:53Z

Sorry yeah sure, we can do this later! Let's just keep an issue open for this

willtebbutt · 2020-09-25T10:55:35Z

Sorry yeah sure, we can do this later! Let's just keep an issue open for this

Great. I'll open an issue about those two things. Then we're good to go?

willtebbutt · 2020-09-25T17:21:32Z

If someone is happy to approve, I'll get this merged after a patch bump.

src/basekernels/rationalquad.jl

devmotion · 2020-09-25T18:07:14Z

src/basekernels/rationalquad.jl

@@ -1,48 +1,62 @@
 """
-    RationalQuadraticKernel(; α = 2.0)
+    RationalQuadraticKernel(; α=2.0)


Is it common to use α = 2? Something like α = 1 seems simpler, and is actually used as the default value by e.g. scikit-learn.

Hmmm good question. I really hadn't thought too much about this and wasn't overly fussed. I worry that 1 will yield a kernel with very long-range correlations (I'm just thinking about what a students-t with dof 1 looks like -- I think they coincide in this case).

I can't say that I'm overly fussed overall, so happy to change to 1 from 2 if you would prefer.

I don't have a strong opinion on this, so do whatever you think is more reasonable. Just noticed this when I compared it with the parameter choices in scikit-learn, and was wondering why we use 2 here.

I think I'm going to leave as-is for the sake of this PR. More than happy for this to be changed in a subsequent PR though.

devmotion · 2020-09-25T18:10:05Z

src/basekernels/rationalquad.jl

-        @check_args(GammaRationalQuadraticKernel, α, α > one(Tα), "α > 1")
-        @check_args(GammaRationalQuadraticKernel, γ, γ >= one(Tγ), "γ >= 1")
+    function GammaRationalQuadraticKernel(
+        ;alpha::Tα=2.0, gamma::Tγ=2.0, α::Tα=alpha, γ::Tγ=gamma,


I guess

Suggested change

;alpha::Tα=2.0, gamma::Tγ=2.0, α::Tα=alpha, γ::Tγ=gamma,

;alpha::Tα=2.0, gamma::Tγ=2, α::Tα=alpha, γ::Tγ=gamma,

would be more performant in the default case? The question here is also if we should use alpha = 1 as
default value.

Although probably, since we divide gamma by 2 anyway in the computation it doesn't matter.

Hmmm I wonder whether this is what you want. Generally speaking, if you're using this kernel you probably don't want the Integer parameter value, otherwise you would be using the RationalQuadraticKernel. My opinion is that making this parameter a float by default probably does make sense as a consequence of this.

Makes sense.

willtebbutt and others added 13 commits August 30, 2020 12:17

Fix style

e322d2c

Fix convention

81b2bb8

First pass over test set implementation

c795cdb

Add standardised tests to BaseKernels

24b0422

Test composite kernels

32877de

Fix some tests

a32eae6

Fix maha

5815b41

Fix sm

14178ce

Fix up maha

7ab9c52

Remove redundant file

180c934

Move existing test utils over to module

4954575

Add Gamma Exponential kernel reference

a79de46

Update src/matrix/kernelpdmat.jl

4ba9c35

Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>

sharanry reviewed Sep 2, 2020

View reviewed changes

src/test_utils.jl Outdated Show resolved Hide resolved

src/test_utils.jl Show resolved Hide resolved

src/test_utils.jl Outdated Show resolved Hide resolved

src/test_utils.jl Outdated Show resolved Hide resolved

willtebbutt added 4 commits September 2, 2020 14:39

Remove repeated code

ce11e1d

Warn about breaking change

1086241

Merge in master

1c8216d

Resolve merge conflict

e73b23d

devmotion reviewed Sep 7, 2020

View reviewed changes

src/transform/lineartransform.jl Show resolved Hide resolved

devmotion reviewed Sep 7, 2020

View reviewed changes

src/test_utils.jl Show resolved Hide resolved

Update src/test_utils.jl

22295f6

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

theogf previously approved these changes Sep 8, 2020

View reviewed changes

willtebbutt added 4 commits September 8, 2020 10:43

Merge in master

a945ab6

Merge in master

463d1ea

Resolve merge conflict

523313d

Bump patch

b58c649

willtebbutt changed the title ~~WIP: test utils revamp~~ test utils revamp Sep 21, 2020

Fix up tests

2e508e9

Fix weird printing issue

efb18be

theogf reviewed Sep 24, 2020

View reviewed changes

src/test_utils.jl Show resolved Hide resolved

theogf reviewed Sep 24, 2020

View reviewed changes

src/test_utils.jl Outdated Show resolved Hide resolved

devmotion reviewed Sep 24, 2020

View reviewed changes

src/test_utils.jl Outdated Show resolved Hide resolved

theogf reviewed Sep 24, 2020

View reviewed changes

willtebbutt and others added 5 commits September 24, 2020 12:53

Update src/test_utils.jl

93ec40d

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Update test/kernels/kernelsum.jl

c7f2490

Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>

Update src/test_utils.jl

bc345f6

Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>

Test FBM kernel

ab492c0

Fix up Gabor

e2bb5b5

This was referenced Sep 25, 2020

Unnecessary collect? #175

Open

Testing the tests #176

Open

devmotion requested changes Sep 25, 2020

View reviewed changes

src/basekernels/rationalquad.jl Outdated Show resolved Hide resolved

Loosen dof bound

ef044ea

devmotion reviewed Sep 25, 2020

View reviewed changes

devmotion approved these changes Sep 25, 2020

View reviewed changes

Perturb test

e63002e

willtebbutt merged commit 879cfac into master Sep 25, 2020

willtebbutt deleted the wct/test-utils branch September 25, 2020 19:40

sharanry mentioned this pull request Sep 28, 2020

Add unified interface test utils JuliaGaussianProcesses/GPLikelihoods.jl#19

Merged

theogf mentioned this pull request Jul 27, 2021

Exporting ColVecs, RowVecs and vec_of_vecs #163

Closed

devmotion mentioned this pull request Jun 20, 2023

Documenting caveats of SimpleKernel #520

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test utils revamp #159

test utils revamp #159

willtebbutt commented Aug 30, 2020 •

edited

sharanry left a comment

willtebbutt commented Sep 21, 2020

willtebbutt commented Sep 21, 2020 •

edited

willtebbutt commented Sep 24, 2020

theogf left a comment •

edited

theogf Sep 24, 2020

willtebbutt Sep 24, 2020

willtebbutt commented Sep 24, 2020

willtebbutt commented Sep 24, 2020

willtebbutt commented Sep 25, 2020

theogf commented Sep 25, 2020

willtebbutt commented Sep 25, 2020

willtebbutt commented Sep 25, 2020

devmotion Sep 25, 2020

willtebbutt Sep 25, 2020

devmotion Sep 25, 2020

willtebbutt Sep 25, 2020

devmotion Sep 25, 2020

devmotion Sep 25, 2020

willtebbutt Sep 25, 2020

devmotion Sep 25, 2020

	;alpha::Tα=2.0, gamma::Tγ=2.0, α::Tα=alpha, γ::Tγ=gamma,
	;alpha::Tα=2.0, gamma::Tγ=2, α::Tα=alpha, γ::Tγ=gamma,

test utils revamp #159

test utils revamp #159

Conversation

willtebbutt commented Aug 30, 2020 • edited

sharanry left a comment

Choose a reason for hiding this comment

willtebbutt commented Sep 21, 2020

willtebbutt commented Sep 21, 2020 • edited

willtebbutt commented Sep 24, 2020

theogf left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willtebbutt commented Sep 24, 2020

willtebbutt commented Sep 24, 2020

willtebbutt commented Sep 25, 2020

theogf commented Sep 25, 2020

willtebbutt commented Sep 25, 2020

willtebbutt commented Sep 25, 2020

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

willtebbutt commented Aug 30, 2020 •

edited

willtebbutt commented Sep 21, 2020 •

edited

theogf left a comment •

edited