reactivate AD tests: mean functions #313

st-- · 2022-04-06T06:41:18Z

Turns out there were a bunch of AD tests in the code. They were just commented out (and a bit broken). This is the work-in-progress attempt to reactivate them, and thereby resolve #311. Help welcome!

test/test_util.jl

codecov · 2022-04-06T07:04:37Z

Codecov Report

Merging #313 (41a01da) into master (d99311e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
- Coverage   97.64%   97.62%   -0.02%     
==========================================
  Files          10       10              
  Lines         382      379       -3     
==========================================
- Hits          373      370       -3     
  Misses          9        9

Impacted Files	Coverage Δ
src/mean_function.jl	`100.00% <100.00%> (ø)`

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 d99311e...41a01da. Read the comment docs.

src/mean_function.jl

st-- · 2022-04-06T07:11:35Z

src/mean_function.jl

@@ -1,5 +1,7 @@
 abstract type MeanFunction end

+# (m::MeanFunction)(x::AbstractVector) = _map_meanfunction(m, x)


should we define something like this ?

What would be the motivation? Generally, I don't like implicit mapping or broadcasting. IIRC the function only exists to work around Zygote AD issues.

In any case, IMO it does not belong kn this PR.

no, it doesn't belong in here, I just thought while people are thinking about mean functions we can consider it. will remove it before merging.

test/mean_function.jl

st-- · 2022-04-06T07:19:14Z

test/mean_function.jl

-        # differentiable_mean_function_tests(f, randn(rng, N), x)
+        for x in [x1, xD, xD′]
+            @test AbstractGPs._map_meanfunction(m, x) == map(foo_mean, x)
+            differentiable_mean_function_tests(m, randn(rng, N), x)


Should we just call

Suggested change

differentiable_mean_function_tests(m, randn(rng, N), x)

differentiable_mean_function_tests(rng, m, x)

instead (and remove the y = ...), or otherwise remove that (currently unused) method definition of differentiable_mean_function_tests?

Seems like it would simplify things a bit, so I'm in favour.

which one? remove the (rng, m, x) method, or apply this suggestion?

I would be inclined to retain the method that just requires a rng, since we can now be confident that it will generate an appropriate tangent due to the call to collect.

devmotion · 2022-04-06T07:40:32Z

test/Project.toml

@@ -1,4 +1,5 @@
 [deps]
+AbstractGPs = "99985d1d-32ba-4be9-9821-2ec096f28918"


The tested package is usually not part of test/Project.toml as Pkg adds it automatically (https://pkgdocs.julialang.org/v1/creating-packages/#Test-specific-dependencies-in-Julia-1.2-and-above) and otherwise one has to add and update compat entries:

Suggested change

AbstractGPs = "99985d1d-32ba-4be9-9821-2ec096f28918"

Ok, I added it because I wanted to be able to run some of the tests locally, and when I used julia --project=test it complained about AbstractGPs not being in the project

Yeah, that's not an intended workflow and not officially supported. Tests are supposed to be run with Pkg.test.

so what's the intended workflow for "I don't want to run all the tests because that takes a really long time, I just want to run the tests that I'm currently working on in this one file"?

(who says what's intended, officially supported, and supposed to be done? where would I find out about that?)

With the test setup in eg KernelFunctions (and AbstractGPs?) with all imports and utilities in runtests.jl one has to run runtests.jl anyway, it seems, or load the packages manually. In general, also the first two options might be a bit misleading if eg other tests mutate the RNG.

@st-- I use TestEnv.jl (loaded at startup). In the working repo:

TestEnv.activate()

and then all tests modules are loaded as well as the current repo.
But I agree it's a mess

Ok, TestEnv seems to work, so I've reverted this change. How would I have been able to find this out by myself? Is this something we should/could document somewhere ?

I like a lot of things about Julia (e.g. the Pkg manager is so much better than in python land). But I have to say I miss pytest. It makes it so easy to select which subset of tests you want to run. E.g. just the slow ones, or just the fast ones, or the ones in these files only...

I think it'd help if instead of stuffing all imports into runtests.jl, having them at the top of each individual test file. Then it'd be easy to just include() one of them.

I guess one learns about such things by eg asking, googling, or attending JuliaCon. IIRC a while ago TestEnv was also discussed in the Turing slack (probably during or after JuliaCon).

I don't think we should document anything here. It's not a common task, it's not JuliaGP specific, and there's not a single preferred approach in the Julia ecosystem. In particular we don't care about how people test their PRs locally, if they run all tests or only parts of it (this can be tricky even with separate files +TestEnv, see below) and if they use TestEnv or not.

One nice thing about tests is that it is very flexible since it's just one long script, possibly with includes. One can split it up or just use a single file, one can use multiple test sets or not, one can run some tests conditionally on environment variables... But of course the flexibility comes at a cost, eg. when trying to run a subset of tests only.

In general, running subsets of tests seems most convenient if they are 1) separated in different files and/or with different switches such as environment variables (used eg in SciML but also the Turing ecosystem and even AbstractGPs) and 2) put into separate modules, eg with SafeTestsets, with explicit imports, test utilities, and generally avoiding any other leakage eg from mutating the RNG. Without 2) running tests separately may fail and, if all are passing, does not guarantee that running all tests together is successful.

src/mean_function.jl

test/mean_function.jl

devmotion · 2022-04-07T08:47:35Z

test/test_util.jl

+# TODO should move into KernelFunctions.jl
+Base.zero(x::ColVecs) = ColVecs(zero(x.X))
+Base.zero(x::RowVecs) = RowVecs(zero(x.X))
+


Yeah this should really not exist here.

Suggested change

# TODO should move into KernelFunctions.jl

Base.zero(x::ColVecs) = ColVecs(zero(x.X))

Base.zero(x::RowVecs) = RowVecs(zero(x.X))

👍 I'll open a PR in KernelFunctions

I'll update it here once JuliaGaussianProcesses/KernelFunctions.jl#444 is merged

Though note that this is just for the tests. Above, it also defines

Base.zero(d::Dict) = Dict([(key, zero(val)) for (key, val) in d]) Base.zero(x::Array) = zero.(x)

(The zero(::Array) seems to actually be implemented in Base, so not sure why that's here; I'm assuming only the Dict() one is actually needed)

st-- · 2022-04-07T10:07:11Z

@devmotion you requested changes, but it's not clear to me what changes you're actually requesting, could you please clarify?
regarding the definition of zero(::ColVecs), see my reply #313 (comment) though I also opened JuliaGaussianProcesses/KernelFunctions.jl#444

devmotion · 2022-04-07T11:12:31Z

I would like my comments to be addressed. Ie, no new commented out methods, no unrelated and unneeded changes, no new type piracy (doesn't matter if it's for tests or in the package), and understanding what causes the AD issues before reverting the FillArray changes (since possibly one can fix them in a better way).

…s.jl into st/ad_tests

…actGPs.jl into st/ad_tests

devmotion · 2022-04-08T09:10:09Z

I unsubscribed, please find another reviewer.

unsubscribed

Co-authored-by: David Widmann <[email protected]>

willtebbutt

I think this broadly looks good -- I've given some thoughts on how we might sort out the AD testing related problems. I would like to re-review once we've sorted out the AD testing issues, but I think it's basically there.

test/test_util.jl

src/mean_function.jl

willtebbutt · 2022-04-09T17:08:16Z

test/mean_function.jl

-        # differentiable_mean_function_tests(f, randn(rng, N), x)
+        for x in [x1, xD, xD′]
+            @test AbstractGPs._map_meanfunction(m, x) == map(foo_mean, x)
+            differentiable_mean_function_tests(m, randn(rng, N), x)


Seems like it would simplify things a bit, so I'm in favour.

src/mean_function.jl

willtebbutt · 2022-04-09T17:15:17Z

src/mean_function.jl

+function ChainRulesCore.rrule(::typeof(_map_meanfunction), m::ConstMean, x::AbstractVector)
+    map_ConstMean_pullback(Δ) = (NoTangent(), Tangent{ConstMean}(; c=sum(Δ)), ZeroTangent())
+    return _map_meanfunction(m, x), map_ConstMean_pullback
+end


Also, was this introduced to attempt to deal with the AD testing problems?

yes, that was my attempt at working around the lack of FillArray rrules/projectors by doing it directly on the mean function call... do you think it should get removed again ?

src/mean_function.jl

& revert FillArray changes Co-authored-by: willtebbutt <[email protected]>

src/mean_function.jl

test/test_util.jl

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

st-- · 2022-04-09T20:09:10Z

@willtebbutt thanks, looks like your collect() actually fixed the issues, and now it's all happy even with Zero and Fill. Also, I checked and neither of the two rrules defined in mean_function.jl for ZeroMean and ConstMean, respectively, are actually needed for the AD test to pass. Should we keep (/add) or remove them ?

Re #313 (comment) please let me know which way around you prefer.

willtebbutt · 2022-04-09T20:13:37Z

@willtebbutt thanks, looks like your collect() actually fixed the issues, and now it's all happy even with Zero and Fill. Also, I checked and neither of the two rrules defined in mean_function.jl for ZeroMean and ConstMean, respectively, are actually needed for the AD test to pass. Should we keep (/add) or remove them ?

Excellent. Please remove the rules since they're now not needed.

st-- · 2022-04-09T20:17:42Z

Will do.

What do you think of f1df8b5 ?

st-- · 2022-04-09T20:18:27Z

Please remove the rules since they're now not needed.

I'm wondering if they might make it a bit faster though (as they're shortcutting some backprop)?

willtebbutt · 2022-04-09T20:23:47Z

I'm wondering if they might make it a bit faster though (as they're shortcutting some backprop)?

They really shouldn't make stuff faster in this case -- it's simple enough that Zygote ought to be able to infer this properly. In my view, the additional code complexity probably isn't worth it.

willtebbutt · 2022-04-09T20:25:04Z

What do you think of f1df8b5 ?

I like it. Nice to remove some code duplication.

willtebbutt

LGTM

reactivate mean function AD tests

1a99d74

st-- added the help wanted Extra attention is needed label Apr 6, 2022

github-actions bot reviewed Apr 6, 2022

View reviewed changes

test/test_util.jl Outdated Show resolved Hide resolved

st-- added 3 commits April 6, 2022 09:43

format

caeffdc

fix test

9f6227f

revert FillArray

f13c902

st-- added 2 commits April 6, 2022 10:07

extend mean function tests to ColVecs/RowVecs

6ee4c76

extend ZeroMean tests too

9c85f6d

st-- commented Apr 6, 2022

View reviewed changes

src/mean_function.jl Show resolved Hide resolved

st-- commented Apr 6, 2022

View reviewed changes

st-- added 2 commits April 6, 2022 10:17

bugfix

83ccb64

add missing zero() definition for ColVecs/RowVecs

2989293

st-- commented Apr 6, 2022

View reviewed changes

test/mean_function.jl Outdated Show resolved Hide resolved

st-- commented Apr 6, 2022

View reviewed changes

st-- changed the title ~~reactivate AD tests~~ reactivate AD tests: mean functions Apr 6, 2022

st-- mentioned this pull request Apr 6, 2022

reactivate AD tests: FiniteGP #314

Merged

patch bump

8043454

devmotion reviewed Apr 6, 2022

View reviewed changes

revert Project.toml

940777a

devmotion previously requested changes Apr 7, 2022

View reviewed changes

st-- mentioned this pull request Apr 7, 2022

add zero(::ColVecs) / zero(::RowVecs) definitions JuliaGaussianProcesses/KernelFunctions.jl#444

Merged

st-- requested a review from devmotion April 7, 2022 10:05

mean function rrules

f3b736c

st-- added 4 commits April 7, 2022 15:34

revert runtests.jl

750ef77

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

bc0ee6a

…s.jl into st/ad_tests

clean up mean_function tests without reactivating AD

a5365d6

Merge branch 'st/meanfunctiontest' into st/ad_tests

75f95cd

st-- added 3 commits April 7, 2022 17:39

Merge branch 'st/ad_tests' of github.com:JuliaGaussianProcesses/Abstr…

81841c1

…actGPs.jl into st/ad_tests

Merge branch 'st/meanfunctiontest' into st/ad_tests

77f6ebf

remove no longer needed test

263a56c

st-- requested review from willtebbutt and theogf April 8, 2022 13:28

Base automatically changed from st/meanfunctiontest to master April 8, 2022 16:23

Merge branch 'master' into st/ad_tests

fd69656

st-- and others added 2 commits April 8, 2022 23:04

Update src/mean_function.jl

97c5284

Co-authored-by: David Widmann <[email protected]>

Merge branch 'master' into st/ad_tests

9e93b93

willtebbutt reviewed Apr 9, 2022

View reviewed changes

st-- commented Apr 9, 2022

View reviewed changes

src/mean_function.jl Outdated Show resolved Hide resolved

st-- commented Apr 9, 2022

View reviewed changes

src/mean_function.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

01a7ac0

& revert FillArray changes Co-authored-by: willtebbutt <[email protected]>

github-actions bot reviewed Apr 9, 2022

View reviewed changes

src/mean_function.jl Outdated Show resolved Hide resolved

test/test_util.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

4b0a683

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

unify testcases

f1df8b5

remove rrules and ChainRulesCore

145091d

willtebbutt approved these changes Apr 9, 2022

View reviewed changes

pass rng

41a01da

st-- merged commit cd8f069 into master Apr 10, 2022

st-- deleted the st/ad_tests branch April 10, 2022 09:09

		@@ -1,5 +1,7 @@
		abstract type MeanFunction end

		# (m::MeanFunction)(x::AbstractVector) = _map_meanfunction(m, x)

	differentiable_mean_function_tests(m, randn(rng, N), x)
	differentiable_mean_function_tests(rng, m, x)

		@@ -1,4 +1,5 @@
		[deps]
		AbstractGPs = "99985d1d-32ba-4be9-9821-2ec096f28918"

	# TODO should move into KernelFunctions.jl
	Base.zero(x::ColVecs) = ColVecs(zero(x.X))
	Base.zero(x::RowVecs) = RowVecs(zero(x.X))

reactivate AD tests: mean functions #313

reactivate AD tests: mean functions #313

Conversation

st-- commented Apr 6, 2022

codecov bot commented Apr 6, 2022 • edited Loading

Codecov Report

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

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-- Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

st-- commented Apr 7, 2022

devmotion commented Apr 7, 2022 • edited Loading

devmotion commented Apr 8, 2022

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

st-- commented Apr 9, 2022

willtebbutt commented Apr 9, 2022

st-- commented Apr 9, 2022

st-- commented Apr 9, 2022

willtebbutt commented Apr 9, 2022

willtebbutt commented Apr 9, 2022

willtebbutt left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2022 •

edited

Loading

st-- Apr 7, 2022 •

edited

Loading

devmotion commented Apr 7, 2022 •

edited

Loading