-
Notifications
You must be signed in to change notification settings - Fork 36
DebugAccumulator (plus tiny bits and pieces) #976
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
Changes from all commits
28e5ba4
88da7bd
8c7aff9
a85f28d
919cb25
0649972
0ebb56e
e534434
d73bb14
cd2f969
1f10b18
8a7fea8
0635607
40eddde
408b951
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 |
---|---|---|
|
@@ -2,7 +2,15 @@ | |
|
||
## 0.37.0 | ||
|
||
**Breaking changes** | ||
DynamicPPL 0.37 comes with a substantial reworking of its internals. | ||
Fundamentally, there is no change to the actual modelling syntax: if you are a Turing.jl user, for example, this release is unlikely to affect you much. | ||
However, if you are a package developer or someone who uses DynamicPPL's functionality directly, you will notice a number of changes. | ||
|
||
To avoid overwhelming the reader, we begin by listing the most important, user-facing changes, before explaining the changes to the internals in more detail. | ||
|
||
Note that virtually all changes listed here are breaking. | ||
|
||
**Public-facing changes** | ||
|
||
### Submodel macro | ||
|
||
|
@@ -19,6 +27,32 @@ There is now also an `rng` keyword argument to help seed parameter generation. | |
Finally, instead of specifying `value_atol` and `grad_atol`, you can now specify `atol` and `rtol` which are used for both value and gradient. | ||
Their semantics are the same as in Julia's `isapprox`; two values are equal if they satisfy either `atol` or `rtol`. | ||
|
||
### `DynamicPPL.TestUtils.check_model` | ||
|
||
You now need to explicitly pass a `VarInfo` argument to `check_model` and `check_model_and_trace`. | ||
Previously, these functions would generate a new VarInfo for you (using an optionally provided `rng`). | ||
Comment on lines
+32
to
+33
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'm confused, check_model(model::Model, varinfo::AbstractVarInfo=VarInfo(model); error_on_failure=false) making 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. Oh, that is true, I didn't realise that. Errrrr, I can't say I like having the VarInfo be optional. In 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 thought that it was semantically neat that to 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. Yeahh, agreed. That's the main reason why I was a bit hesitant in my last comment. The single-argument version did require being restrictive in that it would always use SamplingContext though (there was no way to change this). I guess I'll keep it this way now (with both model and varinfo compulsory) but also keep it in mind as one of the areas where we're not fully sure about the best API. |
||
|
||
### Removal of `PriorContext` and `LikelihoodContext` | ||
|
||
A number of DynamicPPL's contexts have been removed, most notably `PriorContext` and `LikelihoodContext`. | ||
Although these are not the only _exported_ contexts, we consider unlikely that anyone was using _other_ contexts manually: if you have a question about contexts _other_ than these, please continue reading the 'Internals' section below. | ||
|
||
Previously, during evaluation of a model, DynamicPPL only had the capability to store a _single_ log probability (`logp`) field. | ||
`DefaultContext`, `PriorContext`, and `LikelihoodContext` were used to control what this field represented: they would accumulate the log joint, log prior, or log likelihood, respectively. | ||
|
||
Now, we have reworked DynamicPPL's `VarInfo` object such that it can track multiple log probabilities at once (see the 'Accumulators' section below). | ||
If you were evaluating a model with `PriorContext`, you can now just evaluate it with `DefaultContext`, and instead of calling `getlogp(varinfo)`, you can call `getlogprior(varinfo)` (and similarly for the likelihood). | ||
|
||
If you were constructing a `LogDensityFunction` with `PriorContext`, you can now stick to `DefaultContext`. | ||
`LogDensityFunction` now has an extra field, called `getlogdensity`, which represents a function that takes a `VarInfo` and returns the log density you want. | ||
Thus, if you pass `getlogprior` as the value of this parameter, you will get the same behaviour as with `PriorContext`. | ||
|
||
The other case where one might use `PriorContext` was to use `@addlogprob!` to add to the log prior. | ||
Previously, this was accomplished by manually checking `__context__ isa DynamicPPL.PriorContext`. | ||
Now, you can write `@addlogprob (; logprior=x, loglikelihood=y)` to add `x` to the log-prior and `y` to the log-likelihood. | ||
|
||
**Internals** | ||
|
||
### Accumulators | ||
|
||
This release overhauls how VarInfo objects track variables such as the log joint probability. The new approach is to use what we call accumulators: Objects that the VarInfo carries on it that may change their state at each `tilde_assume!!` and `tilde_observe!!` call based on the value of the variable in question. They replace both variables that were previously hard-coded in the `VarInfo` object (`logp` and `num_produce`) and some contexts. This brings with it a number of breaking changes: | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
0.37 is turning into a bit of a monster. I'm personally quite happy with this, even a little bit excited! 😄
But I think we might need to start treating it a bit more seriously. I started by separating the changelog into more public vs more internal changes. (For example, most people really don't need to care about accs; even if you're using something like
values_as_in_model
, you don't need to care about whether it was implemented using a context or an acc.)Apart from this, maybe we should probably have a definition of done for 0.37 (ie which PRs/features do we want to get in for that release)? If you agree, then I'll start putting together a checklist on the
breaking
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.
For me the only real question is, do we want the simplifications afforded by accumulators to be in v0.37 or not. Either all of them or most of them. I would like VariableOrderAccumulator, because that's kinda where this whole thing started, getting rid of
num_produce
. Haven't thought carefully about what else might be on the fence of whether it's in or out.I have nothing that would be entirely unrelated to accumulators that I would like to put in v0.37.
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.
Very happy with the improvements to the changelist.
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.
In terms of accs, it's just VariableOrder left, isn't it? And we could keep it as a default acc in 0.37, and maybe later work out how to make it opt-in for PG only?
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 it might be just VariableOrder, though I haven't looked at this in a few weeks, so may forget something. We could keep it as default, but I would also consider leaving it out and immediately moving it Turing.jl once it's functional.
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 seems that we've actually converged to PG on two fronts, one with VariableOrderAcc and removing
num_produce
, the other with removingSamplingContext
and thedel
flag #982.If you asked me to be opinionated: I wonder if it may be easier for us to leave all the PG-related stuff to 0.38, partly because 0.37 is getting very big, and partly so that we can compartmentalise PG and non-PG stuff?
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 don't really mind v0.37 getting big, when we do PRs one by one, as long as it doesn't start to hold up releasing something of use. I guess it might make the integration work in Turing.jl more painful. Leaving things like removing code that is being added to Turing.jl for v0.38 would make sense, to have one release where it's all in place but not yet gone. Generally not bothered if you want to make an intermediate release. When you say "leave all the PG-related stuff to 0.38", would that include implementing VariableOrderAccumulator?
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.
Yup.