-
Notifications
You must be signed in to change notification settings - Fork 8
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
More aggressive concretization #35
Comments
Would this break variables of dynamically changing size, similar to https://turing.ml/dev/tutorials/06-infinite-mixture-model/ where the size of |
Not more than concretization of But looking at this from the other side -- have we thoroughly tested the existing lens-based varname implementation for this case...? |
I mean, even if you have the weirdest case I can think of,
in a loop, it should be save, since the And Am I missing something? |
@torfjelde what do you think about this? |
On a first glance this looks like a good idea to me! As you said, this shouldn't cause any issues with examples such as mentioned above since it's all done at runtime anyways. |
This is a draft PR introducing a `Model` type that stores and makes use the model graph. The main type introduced here is the `Model` struct which stores the `ModelState` and `DAG`, each of which are their own types. `ModelState` contains information about the node values, dependencies and eval functions and `DAG` contains the graph and topologically ordered vertex list. A model can be constructed in the following way: ```julia julia> nt = ( s2 = (0.0, (), () -> InverseGamma(2.0,3.0), :Stochastic), μ = (1.0, (), () -> 1.0, :Logical), y = (0.0, (:μ, :s2), (μ, s2) -> MvNormal(μ, sqrt(s2)), :Stochastic) ) (s2 = (0.0, (), var"#33#36"(), :Stochastic), μ = (1.0, (), var"#34#37"(), :Logical), y = (0.0, (:μ, :s2), var"#35#38"(), :Stochastic)) julia> Model(nt) Nodes: μ = (value = 1.0, input = (), eval = var"#16#19"(), kind = :Logical) s2 = (value = 0.0, input = (), eval = var"#15#18"(), kind = :Stochastic) y = (value = 0.0, input = (:μ, :s2), eval = var"#17#20"(), kind = :Stochastic) DAG: 3×3 SparseMatrixCSC{Float64, Int64} with 2 stored entries: ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ 1.0 1.0 ⋅ ``` At present, only functions needed for the constructors are implemented, as well as indexing using `@varname`. I still need to complete the integration with the AbstractPPL api. TODO: ~~- [ ] `condition`/`decondition`,~~ ~~- [ ] `sample`~~ ~~- [ ] `logdensityof`~~ - [x] pure functions for ordered dictionary, as outlined in [AbstractPPL](https://github.com/TuringLang/AbstractPPL.jl#property-interface) Feedback on `Model` structure welcome whilst I implement the remaining features!
Should implement #35. The "problem" with this is that the resulting values are not exactly what you'd write by hand: ```julia julia> AbstractPPL.concretize(@varname(x.a[1:end, end][:]), x) x.a[1:2,2][Base.Slice(Base.OneTo(2))] ``` That wouldn't be so much of a problem besides printing, but unfortunately, lenses currenctly compare equality using strict types: ```julia julia> AbstractPPL.concretize(@varname(x[1:3, :]), rand(10, 10)).lens.indices == @varname(x[1:3, 1:10]).lens.indices true julia> AbstractPPL.concretize(@varname(x[1:3, :]), rand(10, 10)).lens == @varname(x[1:3, 1:10]).lens false julia> AbstractPPL.concretize(@varname(x[1:3, :]), rand(10, 10)) == @varname(x[1:3, 1:10]) false ``` Cf. jw3126/Setfield.jl#165; the equality comparison can hopefully be fixed there. The remaining thing is that subsumption must still be able to work with `Colon`, since a user might index a trace/VarInfo using a non-concretized varname containing a colon. But at least we can then be sure that one side is always concrete. Co-authored-by: Hong Ge <hg344@cam.ac.uk> Co-authored-by: Hong Ge <yebai@users.noreply.github.com>
It should be fixed now. |
I propose to make
concretize
turn every:
into the value ofaxis(x, i)
.I think this would make the set of
VarName
s we are working on a nicely defined poset, since then it's basically "sequences of tuples of sets" with the subsumption relation following inductively from sets of indices (leaving out linear indexing and trailing ones).That should also make this weird corner case obsolete.
And while we're at it: boolean indexing could be concretized to array indexing:
The text was updated successfully, but these errors were encountered: