-
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
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 f4e5f4bComputer Information
Benchmark Results
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #985 +/- ##
============================================
+ Coverage 81.20% 82.13% +0.92%
============================================
Files 39 39
Lines 3910 3811 -99
============================================
- Hits 3175 3130 -45
+ Misses 735 681 -54 ☔ 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
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.
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.
Actually just for posterity I did find a way to reproduce it when I tried to remove eltype
etc. in #1015. But without the changes from that PR I couldn't repro it
79a5f3c
to
7aae613
Compare
4c96020
to
7b4c5fa
Compare
331279c
to
7b0d5a9
Compare
8587eb7
to
a6a42bd
Compare
0bd4e02
to
8e3dcac
Compare
f6dd1d5
to
d9292ad
Compare
726d486
to
bc04355
Compare
8e3dcac
to
0b87d0d
Compare
0b87d0d
to
992569f
Compare
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.
sorry this file's diff is a bit of a mess, there are no real code changes, it's just:
- merged
tilde_assume
,tilde_assume!!
, andassume
into a single function (they all just called each other) - added types to the arguments
- added proper docstrings
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.
Enjoyed this one a lot.
## Model | ||
|
||
### Macros | ||
|
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.
Is it time to start a HISTORY.md entry? Might be easier to do it here were you can cross-check against what's being removed, rather than once everything is in breaking in a huge diff.
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.
Yeah, good idea to do it in this PR. I'll write one up later and ping you again
end | ||
function tilde_assume(rng::Random.AbstractRNG, ::DefaultContext, sampler, right, vn, vi) | ||
return assume(rng, sampler, right, vn, vi) | ||
function tilde_assume!!( |
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.
To confirm that I understand how this will play out in Turing.jl: The idea is that samplers don't need to modify the behaviour of the tilde pipeline any more, and thus SamplingContext
can go in its entirety, and we don't need things like tilde_assume
without !!
or assume
. And the few that do still need to do that (Gibbs, maybe particles, hopefully nothing else) need to define their own context. Is that right?
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 think so, although I'm not 100% sure. We'll probably have to do a similar process to what we did last time for accumulators: make a Turing PR that builds against the breaking branch of DPPL. Things might break. But I'm hoping it won't be too bad haha.
Mildly tangential: I kind of find it weird that we do |
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, too, have disliked the bolding taking precedence over headings. Happy for that to be changed.
|
||
- `InitContext(rng, InitFromPrior())`: New values are sampled from the prior distribution (on the right-hand side of the tilde). | ||
- `InitContext(rng, InitFromUniform(a, b))`: New values are sampled uniformly from the interval `[a, b]`, and then invlinked to the support of the distribution on the right-hand side of the tilde. | ||
- `InitContext(rng, InitFromParams(p, fallback))`: New values are obtained by indexing into the `p` object, which can be a `NamedTuple` or `Dict{<:VarName}`. If a variable is not found in `p`, then the `fallback` strategy is used, which is simply another of these strategies. In particular, `InitFromParams` enables the case where different variables are to be initialised from different sources. |
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.
enables the case where different variables are to be initialised from different sources.
Does this allow different sources in some broader sense than some having fixed values and others using fallback
? Like could I do InitFromPrior
for some and InitFromUniform
for others?
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.
That thought did pop into my mind when I was writing, which led me to mention the custom initialisation strategy just below this. You'd have to write your custom InitFromFoo
, but it would be possible in that function to check e.g. the varname and dispatch to prior / uniform accordingly.
Co-authored-by: Markus Hauru <[email protected]>
Day N of coveralls failing randomly... |
Part 1: Adding
hasvalue
andgetvalue
to AbstractPPLPart 2: Removing
hasvalue
andgetvalue
from DynamicPPLPart 3: Introducing
InitContext
andinit!!
Part 4: Using
InitFromParams
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