-
Notifications
You must be signed in to change notification settings - Fork 36
InitContext
, part 5 - Remove SamplingContext
, SampleFrom{Prior,Uniform}
, {tilde_,}assume
#985
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
base: py/actually-use-init
Are you sure you want to change the base?
Conversation
c32d112
to
b7221cc
Compare
InitContext
, part 5 - Remove SamplingContext
, SampleFromPrior
, SampleFromUniform
, and associated codeInitContext
, part 5 - Remove SamplingContext
, SampleFrom{Prior,Uniform}
, {tilde_,}assume
b7221cc
to
3835d01
Compare
d55d378
to
a392451
Compare
3835d01
to
713034f
Compare
a392451
to
12d93e5
Compare
713034f
to
45a97ee
Compare
12d93e5
to
7a8e7e3
Compare
45a97ee
to
e817d9c
Compare
7e38bbe
to
1d8bceb
Compare
92772b5
to
1ffc409
Compare
1d8bceb
to
2edcd10
Compare
1957b06
to
4fc60dc
Compare
Benchmark Report for Commit 7aae613Computer Information
Benchmark Results
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## py/actually-use-init #985 +/- ##
=======================================================
Coverage ? 79.89%
=======================================================
Files ? 39
Lines ? 3914
Branches ? 0
=======================================================
Hits ? 3127
Misses ? 787
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DynamicPPL.jl documentation for PR #985 is available at: |
3a16f9c
to
4c96020
Compare
3951d1d
to
79a5f3c
Compare
test/lkj.jl
Outdated
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.
It's maybe a bit opinionated to delete this file. I personally don't see the point of it. It's essentially testing that rand(::LKJ)
or rand(::LKJCholesky)
works (which is a Distributions problem) and/or testing that LKJ
/ LKJCholesky
linking works correctly (which is a Bijectors problem).
The direct replacement for these tests would be something like this:
for init_strategy in [PriorInit(), UniformInit()]
lkjchol_samples = [DynamicPPL.init(Random.default_rng, @varname(x), LKJCholesky(...), init_strategy) for _ in 1:1000]
@test isapprox(mean(lkjchol_samples), expected_mean)
end
However, I don't quite see why we should perform this test for LKJ/LKJCholesky and not for any other distributions. DynamicPPL.init
only uses Bijectors functionality, and the only reason why we would test LKJ specifically is if we were concerned about Bijectors' reliability on LKJ. But that should be tested in Bijectors, not here.
@@ -77,48 +77,6 @@ using DynamicPPL.TestUtils.AD: run_ad, WithExpectedResult, NoTest | |||
end | |||
end | |||
|
|||
@testset "Turing#2151: ReverseDiff compilation & eltype(vi, spl)" 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.
I couldn't find a way to reproduce this test without SamplingContext and the whole tilde-pipeline machinery that this PR removes, so I think that this is no longer relevant.
79a5f3c
to
7aae613
Compare
Part 1: Adding
hasvalue
andgetvalue
to AbstractPPLPart 2: Removing
hasvalue
andgetvalue
from DynamicPPLPart 3: Introducing
InitContext
andinit!!
Part 4: Using
ParamsInit
to implementpredict
,returned
, andinitialize_values
This is part 5/N of #967.
This PR removes everything that is no longer needed.
SamplingContext
,SampleFromPrior
,SampleFromUniform
, now have direct one-to-one replacements (albeit with slightly different behaviour since they now always overwrite variables in the varinfo).It also removes
assume
andtilde_assume
.Prior to this PR we had two different kinds of
assume
, one with a sampler and one without. Now we only have the one without, so we can just move that definition intotilde_assume!!(::DefaultContext, ...)
.Finally,
tilde_assume
has been subsumed intotilde_assume!!
as we can just dispatch on the type ofright
. (Previously this wasn't possible because there was a lot of stuff aboutis_rhs_model
, etc. etc. which was removed in #960.)Closes #859
Closes #955