Skip to content

Conversation

@devmotion
Copy link
Member

Constant propagation in the VarName does not work reliably as we noticed in TuringLang/DynamicPPL.jl#221. Applying the changes in this PR improves performance noticeably.

IMO there is no reason to keep the existing type-unstable constructor. I deprecated it, therefore it is a non-breaking change.

@codecov
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@b7a2e48). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #17   +/-   ##
=======================================
  Coverage        ?   82.45%           
=======================================
  Files           ?        1           
  Lines           ?       57           
  Branches        ?        0           
=======================================
  Hits            ?       47           
  Misses          ?       10           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7a2e48...b315b92. Read the comment docs.

@phipsgabler
Copy link
Member

I completely agree. This is basically the same effect as the recommendation to use Val{x} instead of Val(x), I believe.

@phipsgabler phipsgabler merged commit 0d31764 into main Apr 7, 2021
@phipsgabler phipsgabler deleted the dw/varname branch April 7, 2021 13:57
bors bot pushed a commit that referenced this pull request 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!
@marius311
Copy link

Following this deprecation, is there any succinct way to do VarName.((:x,:y,:z)) without the warning?

@phipsgabler
Copy link
Member

Nothing shorter than map(n -> VarName{n}(), (:x, :y, :z)), I think. It's an inherently dynamic operation.

@devmotion
Copy link
Member Author

I'm not a fan of eval usually but eval.(varname.((:x,:y,:z))) would work as well.

@phipsgabler
Copy link
Member

I mean, if this were a frequent practical problem, I can imagine a solution similar to @gensym where

@varnames x y z[1]

expands (unhygienically) to

x = VarName{:x}()
y = VarName{:y}()
var"z[1]" = @varname(z[1])

But unless there's a pressing need, I'll leave it as an exercise for the interested reader.

@marius311
Copy link

My use-case was within an inference library and I need to generate them programatically so the macro solution isn't what I want, but its an easy 1-line fix, I was just wondering if there did remain something builtin but its fine.

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

Successfully merging this pull request may close these issues.

4 participants