-
Notifications
You must be signed in to change notification settings - Fork 31
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
Remove Turing integration tests #733
base: master
Are you sure you want to change the base?
Conversation
ddf6ff9
to
27637ff
Compare
Pull Request Test Coverage Report for Build 12302036826Details
💛 - Coveralls |
556cd56
to
696bb3d
Compare
e06591b
to
1055a10
Compare
21889d7
to
0d7b2d1
Compare
0d7b2d1
to
675b40f
Compare
d023c04
to
543a97b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #733 +/- ##
==========================================
- Coverage 86.49% 86.02% -0.47%
==========================================
Files 36 36
Lines 4272 4272
==========================================
- Hits 3695 3675 -20
- Misses 577 597 +20 ☔ View full report in Codecov by Sentry. |
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.
Some specific notes on the tests that got removed.
I promise that apart from everything I highlighted here, everything did get moved into the DPPL test suite.
@@ -1,348 +0,0 @@ | |||
@testset "compiler.jl" begin |
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.
The tests in this file (test/turing/compiler.jl
) are the only ones that weren't moved somewhere inside the DynamicPPL test suite. That's because they all actually rely on sampling the posterior and performing numerical checks.
Consequently, I've just moved them to Turing, at least for the time being. This is being tracked at TuringLang/Turing.jl#2419
I don't entirely know the purpose of these tests, but I got quite tired of trawling through all of the tests and figuring out what they were doing / whether they were still needed, so I think this is a reasonable compromise for now.
@testset "replay" begin | ||
# Generate synthesised data | ||
xs = rand(Normal(0.5, 1), 100) | ||
|
||
# Define model | ||
@model function priorsinarray(xs, ::Type{T}=Float64) where {T} | ||
begin | ||
priors = Vector{T}(undef, 2) | ||
priors[1] ~ InverseGamma(2, 3) | ||
priors[2] ~ Normal(0, sqrt(priors[1])) | ||
for i in 1:length(xs) | ||
xs[i] ~ Normal(priors[2], sqrt(priors[1])) | ||
end | ||
priors | ||
end | ||
end | ||
|
||
# Sampling | ||
chain = sample(priorsinarray(xs), HMC(0.01, 10), 10) | ||
end |
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.
I removed this test without replacing it anywhere else. It seems to be checking that we can sample from a model that has its priors in an array. But I think this aspect is already covered in our DEMO_MODELS, specifically demo_dot_assume_observe_index
, so I made the call to remove this.
DynamicPPL.jl/src/test_utils/models.jl
Lines 272 to 282 in 2252a9b
@model function demo_dot_assume_observe_index( | |
x=[1.5, 2.0], ::Type{TV}=Vector{Float64} | |
) where {TV} | |
# `dot_assume` and `observe` with indexing | |
s = TV(undef, length(x)) | |
s .~ InverseGamma(2, 3) | |
m = TV(undef, length(x)) | |
m .~ Normal.(0, sqrt.(s)) | |
for i in eachindex(x) | |
x[i] ~ Normal(m[i], sqrt(s[i])) | |
end |
543a97b
to
7bfd233
Compare
…-> demo_assume_multivariate_observe_literal
7bfd233
to
7a93e4b
Compare
7a93e4b
to
eafcf3c
Compare
What do you think is the right order to merge this and TuringLang/Turing.jl#2419? |
Co-authored-by: Markus Hauru <[email protected]>
This one first, as it's not just tests being modified but also source code so the Turing PR will have to handle the breaking changes here. |
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.
This one first, as it's not just tests being modified but also source code so the Turing PR will have to handle the breaking changes here.
Sounds good. Given that this creates a moment when the tests are in neither package, we should make sure to readd them in Turing.jl soon.
Great stuff, thanks @penelopeysm!
Closes #703
General breaking changes
link!
andinvlink!
. They've been deprecated for about two years now: https://github.com/TuringLang/DynamicPPL.jl/blame/f0c31f045ccd9b69374a68ea24d3823d632a5234/src/varinfo.jl#L1323-L1329Breaking changes in
TestUtils.DEMO_MODELS
demo_assume_observe_literal
has been renamed todemo_assume_multivariate_observe_literal
to avoid name clash with new modeldemo_assume_literal_dot_observe
renamed todemo_assume_dot_observe_literal
to match the naming pattern of the other modelsdemo_assume_observe_literal
, which is a univariate assumeThis is a slightly annoying change, because anybody who uses
demo_assume_observe_literal
will have the behaviour changed silently. But the only code that uses it is DynamicPPL and Turing, so I think it's fine, as long as I fix it upstream too. (See GitHub-wide search fordemo_assume_observe_literal
)Changes to test-internal utils
make_chain_from_prior([rng,] model, n_iters) which constructs an MCMCChains.Chains object by sampling from the prior of
modelfor
n_iters` iterations.Note this is in
test/test_util.jl
rather thansrc/test_utils/foo.jl
, hence only accessible inside the test suite.Changes to test suite
make_chain_from_prior
function above), or some sampling algorithm with somespace
parameter (which has been replaced with a dummy algorithmMyAlg
intest/varinfo.jl
).Feedback needed:
make_chain_from_prior
seems too useful to languish intest/test_util.jl
. Should this be in DynamicPPLMCMCChainsExt?