-
Notifications
You must be signed in to change notification settings - Fork 10
VarName rework #150
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
VarName rework #150
Changes from 60 commits
54b3435
1957e5d
b4aee1c
9a8a236
41430d3
f0ac406
e7dd774
bb93f3d
5e9401a
6d73a5a
ed3ba78
07a7e53
8464ea2
5aa3997
8826e60
0fe06bb
e9b1f98
ea89272
6a72e8b
04231f0
6bccab0
0bbaf31
c103602
8555095
b2b23b9
1ab00dd
7efa65d
57d50dc
9bc9f3d
839eaa1
f31e37d
c759a2e
c4a2a15
43c969a
ecb6590
add9d34
58636a8
2eb7e7f
7083a69
ac95c23
456c322
63b04b9
e9eab0a
4ef515e
e11e23e
5a28760
0317af7
38cbf74
3b0c376
7ee1d04
84b3865
95a69e1
0282e15
9733bfe
e5277af
a2a7c1f
b77ed52
15c6968
bc36cdc
2571d7d
ff3f297
acebcf9
c29fab0
bfad289
6179919
b1ecce8
7022297
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,3 +1,56 @@ | ||
| ## 0.14.0 | ||
|
|
||
| This release overhauls the `VarName` type. | ||
| Much of the external API for traversing and manipulating `VarName`s (once they have been constructed) has been preserved, but if you use the `VarName` type directly, there are significant changes. | ||
|
|
||
| **Internal representation** | ||
|
|
||
| The `optic` field of VarName now uses our hand-rolled optic types, which are subtypes of `AbstractPPL.AbstractOptic`. | ||
| Previously these were optics from Accessors.jl. | ||
|
|
||
| This change was made for two reasons: firstly, it is easier to provide custom behaviour for VarNames as we avoid running into possible type piracy issues, and secondly, the linked-list data structure used in `AbstractOptic` is easier to work with than Accessors.jl, which used `Base.ComposedFunction` to represent optic compositions and required a lot of care to avoid a litany of issues with associativity and identity optics (see e.g. https://github.com/JuliaLang/julia/pull/54877). | ||
|
|
||
| To construct an optic, the easiest way is to use the `@opticof` macro, which superficially behaves similarly to `Accessors.@optic` (for example, you can write `@opticof _[1].y.z`), but also supports automatic concretization by passing a second parameter (just like `@varname`). | ||
|
|
||
| **Concretization** | ||
|
|
||
| VarNames using 'dynamic' indices, i.e., `begin` and `end`, are now instantiated in a 'dynamic' form, meaning that these indices are unresolved. | ||
| These indices need to be resolved, or concretized, against the actual container. | ||
| For example, `@varname(x[end])` is dynamic, but when concretized against `x = randn(3)`, this becomes `@varname(x[3])`. | ||
| This can be done using `concretize(varname, x)`. | ||
|
|
||
| The idea of concretization is not new to AbstractPPL. | ||
| However, there are some differences: | ||
|
|
||
| - Colons are no longer concretized: they *always* remain as Colons, even after calling `concretize`. | ||
| - Previously, AbstractPPL would refuse to allow you to construct unconcretized versions of `begin` and `end`. This is no longer the case; you can now create such VarNames in their unconcretized forms. | ||
| This is useful, for example, when indexing into a chain that contains `x` as a variable-length vector. This change allows you to write `chain[@varname(x[end])]` without having AbstractPPL throw an error. | ||
|
|
||
| **Keyword arguments to `getindex`** | ||
|
|
||
| VarNames can now be constructed with keyword arguments in `Index` optics, for example `@varname(x[i=1])`. | ||
| This is specifically implemented to support DimensionalData.jl's DimArrays. | ||
|
|
||
| **Other interface functions** | ||
|
|
||
| The `vsym` function (and `@vsym`) has been removed; you should use `getsym(vn)` instead. | ||
|
|
||
| The `Base.get` and `Accessors.set` methods for VarNames have been removed (these were responsible for method ambiguities). | ||
| Instead of using these methods you can first convert the `VarName` to an optic using `varname_to_optic(vn)`, and then use the getter and setter methods on the optics. | ||
|
|
||
| VarNames cannot be composed with optics now (compose the optics yourself). | ||
|
Member
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. Can I still
Member
Author
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. Fair.
Member
Author
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. Not a fan of unnecessary identifier reuse, so I've called it |
||
|
|
||
| The `inspace` function has been removed. | ||
| It used to be relevant for Turing's old Gibbs sampler; but now it no longer serves any use. | ||
|
|
||
| `ConcretizedSlice` has been removed (since colons are no longer concretized). | ||
|
|
||
| The subsumption interface has been pared down to just a single function, `subsumes`. | ||
| All other functions, such as `subsumedby`, `uncomparable`, and the Unicode operators, have been removed. | ||
|
|
||
| Serialization still works exactly as before. | ||
| However, you will see differences in the serialization output compared to previous versions, due to the changes in the internal structure. | ||
|
|
||
| ## 0.13.6 | ||
|
|
||
| Fix a missing qualifier in AbstractPPLDistributionsExt. | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| # Probabilistic programming API | ||
|
|
||
| ## Abstract model functions | ||
|
|
||
| ```@docs | ||
| AbstractProbabilisticProgram | ||
| condition | ||
| decondition | ||
| fix | ||
| unfix | ||
| logdensityof | ||
| AbstractContext | ||
| evaluate!! | ||
| ``` | ||
|
|
||
| ## Abstract traces | ||
|
|
||
| ```@docs | ||
| AbstractModelTrace | ||
| ``` |
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.
Why? Doesn't this undermine the purpose of concretisation? I understand the comment in the DPPL PR of eventually getting rid of concretisation in DPPL, but this doesn't seem like the solution to that.
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 have thought about this a lot, and I haven't actually found any reason why concretisation is needed, as long as shadow arrays in VNT are implemented. (I know you don't need the link, just leaving it there for future readers.) I would rather get rid of the entire
concretisefunction, but I think it's better to err on the safe side for now.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.
I mean, let's put it this way, colons are already quite broken in DPPL, so I can't be making anything worse.
But also I'm quite sure I'm making it better.
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.
But I think if
concretizeexists, then it should really concretize, and I think that would involve getting rid of colons.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.
I see. I think maybe we have different notions of concretise, or maybe the old and new ones don't line up. A Colon is still something you can pass to
getindexjust fine -- just like any other index:Only
beginandendare special because they are lowered, somewhere in Base Julia, so you can't callgetindexwithbeginand instead
I think concretisation really means to resolve these lowered expressions, which is the same approach that is used in Accessors itself (Accessors doesn't have any special Colon-handling code), but is not the same as what old AbstractPPL does. I think the point is, you don't need to know what
xis to index intox[:], because you can just passColontogetindexand let Julia handle that for you. On the other hand, you can't do the equivalent ofgetindex(x, end)without knowing whatxis.If we took the old notion of
AbstractPPL.concretize, then one could argue that linear indices should also be concretised to Cartesian indices.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 makes sense, I hadn't realised our notion of concrete was changing.