Skip to content
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

Closed
phipsgabler opened this issue Sep 20, 2021 · 6 comments
Closed

More aggressive concretization #35

phipsgabler opened this issue Sep 20, 2021 · 6 comments

Comments

@phipsgabler
Copy link
Member

I propose to make concretize turn every : into the value of axis(x, i).

I think this would make the set of VarNames 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:

concretize(@varname(x[[true, false]], x) == @varname(x[[1,2]])
@devmotion
Copy link
Member

Would this break variables of dynamically changing size, similar to https://turing.ml/dev/tutorials/06-infinite-mixture-model/ where the size of mu changes in the for loop?

@phipsgabler
Copy link
Member Author

Not more than concretization of begin and end would, I assume? All of these depend essentially only on the length/axes of the value at runtime.

But looking at this from the other side -- have we thoroughly tested the existing lens-based varname implementation for this case...?

@phipsgabler
Copy link
Member Author

I mean, even if you have the weirdest case I can think of,

x[end] ~ D
push!(x, ...)

in a loop, it should be save, since the end is concretized at the point where the assignment happens and thus becomes x[<current index in the loop>].

And : you can only use legally once in a tilda, because otherwise, you'd be sampling the same location twice.

Am I missing something?

@phipsgabler
Copy link
Member Author

@torfjelde what do you think about this?

@torfjelde
Copy link
Member

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.

bors bot pushed a commit that referenced this issue Feb 7, 2022
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!
bors bot pushed a commit that referenced this issue Nov 6, 2022
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 <[email protected]>
Co-authored-by: Hong Ge <[email protected]>
@yebai
Copy link
Member

yebai commented Oct 23, 2023

It should be fixed now.

@yebai yebai closed this as completed Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants