-
Notifications
You must be signed in to change notification settings - Fork 21
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
reactivate AD tests: mean functions #313
Changes from 8 commits
1a99d74
caeffdc
9f6227f
f13c902
6ee4c76
9c85f6d
83ccb64
2989293
8043454
940777a
f3b736c
750ef77
bc0ee6a
a5365d6
75f95cd
1b09168
80b7813
37aeb0a
a7a4d8c
81841c1
77f6ebf
263a56c
fd69656
97c5284
9e93b93
01a7ac0
4b0a683
f1df8b5
145091d
41a01da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,4 +1,5 @@ | ||||
[deps] | ||||
AbstractGPs = "99985d1d-32ba-4be9-9821-2ec096f28918" | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's not an intended workflow and not officially supported. Tests are supposed to be run with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (who says what's intended, officially supported, and supposed to be done? where would I find out about that?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @st-- I use TestEnv.jl (loaded at startup). In the working repo:
and then all tests modules are loaded as well as the current repo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||||
ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4" | ||||
Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f" | ||||
Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4" | ||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,47 +1,53 @@ | ||||||
@testset "mean_functions" begin | ||||||
@testset "ZeroMean" begin | ||||||
P = 3 | ||||||
Q = 2 | ||||||
D = 4 | ||||||
# X = ColVecs(randn(rng, D, P)) | ||||||
x = randn(P) | ||||||
x̄ = randn(P) | ||||||
f = ZeroMean{Float64}() | ||||||
rng, N, D = MersenneTwister(123456), 5, 3 | ||||||
st-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
x1 = randn(rng, N) | ||||||
xD = ColVecs(randn(rng, D, N)) | ||||||
xD′ = RowVecs(randn(rng, N, D)) | ||||||
|
||||||
for x in [x] | ||||||
@test AbstractGPs._map_meanfunction(f, x) == zeros(size(x)) | ||||||
# differentiable_mean_function_tests(f, randn(rng, P), x) | ||||||
end | ||||||
m = ZeroMean{Float64}() | ||||||
|
||||||
for x in [x1, xD, xD′] | ||||||
@test AbstractGPs._map_meanfunction(m, x) == zeros(size(x)) | ||||||
differentiable_mean_function_tests(m, randn(rng, N), x) | ||||||
st-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
# Manually verify the ChainRule. Really, this should employ FiniteDifferences, but | ||||||
# currently ChainRulesTestUtils isn't up to handling this, so this will have to do | ||||||
# for now. | ||||||
y, pb = rrule(AbstractGPs._map_meanfunction, f, x) | ||||||
@test y == AbstractGPs._map_meanfunction(f, x) | ||||||
Δmap, Δf, Δx = pb(randn(P)) | ||||||
@test iszero(Δmap) | ||||||
@test iszero(Δf) | ||||||
@test iszero(Δx) | ||||||
# Manually verify the ChainRule. Really, this should employ FiniteDifferences, but | ||||||
# currently ChainRulesTestUtils isn't up to handling this, so this will have to do | ||||||
# for now. | ||||||
y, pb = rrule(AbstractGPs._map_meanfunction, m, x) | ||||||
@test y == AbstractGPs._map_meanfunction(m, x) | ||||||
Δmap, Δf, Δx = pb(randn(rng, N)) | ||||||
@test iszero(Δmap) | ||||||
@test iszero(Δf) | ||||||
@test iszero(Δx) | ||||||
st-- marked this conversation as resolved.
Show resolved
Hide resolved
st-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
end | ||||||
end | ||||||
@testset "ConstMean" begin | ||||||
rng, D, N = MersenneTwister(123456), 5, 3 | ||||||
# X = ColVecs(randn(rng, D, N)) | ||||||
x = randn(rng, N) | ||||||
rng, N, D = MersenneTwister(123456), 5, 3 | ||||||
st-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
x1 = randn(rng, N) | ||||||
xD = ColVecs(randn(rng, D, N)) | ||||||
xD′ = RowVecs(randn(rng, N, D)) | ||||||
|
||||||
c = randn(rng) | ||||||
m = ConstMean(c) | ||||||
|
||||||
for x in [x] | ||||||
for x in [x1, xD, xD′] | ||||||
@test AbstractGPs._map_meanfunction(m, x) == fill(c, N) | ||||||
# differentiable_mean_function_tests(m, randn(rng, N), x) | ||||||
differentiable_mean_function_tests(m, randn(rng, N), x) | ||||||
end | ||||||
end | ||||||
@testset "CustomMean" begin | ||||||
rng, N, D = MersenneTwister(123456), 11, 2 | ||||||
x = randn(rng, N) | ||||||
rng, N, D = MersenneTwister(123456), 5, 3 | ||||||
x1 = randn(rng, N) | ||||||
xD = ColVecs(randn(rng, D, N)) | ||||||
xD′ = RowVecs(randn(rng, N, D)) | ||||||
|
||||||
foo_mean = x -> sum(abs2, x) | ||||||
f = CustomMean(foo_mean) | ||||||
m = CustomMean(foo_mean) | ||||||
st-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
@test AbstractGPs._map_meanfunction(f, x) == map(foo_mean, x) | ||||||
# 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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just call
Suggested change
instead (and remove the y = ...), or otherwise remove that (currently unused) method definition of differentiable_mean_function_tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like it would simplify things a bit, so I'm in favour. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which one? remove the (rng, m, x) method, or apply this suggestion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be inclined to retain the method that just requires a |
||||||
end | ||||||
end | ||||||
end |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -27,6 +27,10 @@ end | |||||||
Base.zero(d::Dict) = Dict([(key, zero(val)) for (key, val) in d]) | ||||||||
Base.zero(x::Array) = zero.(x) | ||||||||
|
||||||||
# TODO should move into KernelFunctions.jl | ||||||||
Base.zero(x::ColVecs) = ColVecs(zero(x.X)) | ||||||||
Base.zero(x::RowVecs) = RowVecs(zero(x.X)) | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this should really not exist here.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I'll open a PR in KernelFunctions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll update it here once JuliaGaussianProcesses/KernelFunctions.jl#444 is merged There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||||||||
# My version of isapprox | ||||||||
function fd_isapprox(x_ad::Nothing, x_fd, rtol, atol) | ||||||||
return fd_isapprox(x_fd, zero(x_fd), rtol, atol) | ||||||||
|
@@ -73,8 +77,9 @@ end | |||||||
Test _very_ basic consistency properties of the mean function `m`. | ||||||||
""" | ||||||||
function mean_function_tests(m::MeanFunction, x::AbstractVector) | ||||||||
@test AbstractGPs._map_meanfunction(m, x) isa AbstractVector | ||||||||
@test length(ew(m, x)) == length(x) | ||||||||
mean = AbstractGPs._map_meanfunction(m, x) | ||||||||
@test mean isa AbstractVector | ||||||||
@test length(mean) == length(x) | ||||||||
end | ||||||||
|
||||||||
""" | ||||||||
|
@@ -87,34 +92,17 @@ end | |||||||
Ensure that the gradient w.r.t. the inputs of `MeanFunction` `m` are approximately correct. | ||||||||
""" | ||||||||
function differentiable_mean_function_tests( | ||||||||
m::MeanFunction, | ||||||||
ȳ::AbstractVector{<:Real}, | ||||||||
x::AbstractVector{<:Real}; | ||||||||
rtol=_rtol, | ||||||||
atol=_atol, | ||||||||
m::MeanFunction, ȳ::AbstractVector, x::AbstractVector; rtol=_rtol, atol=_atol | ||||||||
) | ||||||||
# Run forward tests. | ||||||||
mean_function_tests(m, x) | ||||||||
|
||||||||
# Check adjoint. | ||||||||
@assert length(ȳ) == length(x) | ||||||||
return adjoint_test(x -> ew(m, x), ȳ, x; rtol=rtol, atol=atol) | ||||||||
adjoint_test(x -> AbstractGPs._map_meanfunction(m, x), ȳ, x; rtol=rtol, atol=atol) | ||||||||
st-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
return nothing | ||||||||
end | ||||||||
|
||||||||
# function differentiable_mean_function_tests( | ||||||||
# m::MeanFunction, | ||||||||
# ȳ::AbstractVector{<:Real}, | ||||||||
# x::ColVecs{<:Real}; | ||||||||
# rtol=_rtol, | ||||||||
# atol=_atol, | ||||||||
# ) | ||||||||
# # Run forward tests. | ||||||||
# mean_function_tests(m, x) | ||||||||
|
||||||||
# @assert length(ȳ) == length(x) | ||||||||
# adjoint_test(X->ew(m, ColVecs(X)), ȳ, x.X; rtol=rtol, atol=atol) | ||||||||
# end | ||||||||
|
||||||||
function differentiable_mean_function_tests( | ||||||||
rng::AbstractRNG, m::MeanFunction, x::AbstractVector; rtol=_rtol, atol=_atol | ||||||||
) | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we define something like this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.