TuringLang / AbstractPPL.jl

Common types and interfaces for probabilistic programming
http://turinglang.org/AbstractPPL.jl/
MIT License
27 stars 7 forks source link

DAG representation using SimplePPL/Mamba #44

Closed PavanChaggar closed 2 years ago

PavanChaggar commented 2 years ago

The aims of this PR are to build a minimal interface for DAG representations of models. In the first instance, we'll try to do this using code from SimplePPL/Mamba.

A SimplePPL Model currently has the following structure:

mutable struct Model
    nodes::Dict{Symbol,Any}
    samplers::Vector{Sampler}
    VariableSet::Vector{VariableSet}
    states::Vector{ModelState}
    iter::Int
    burnin::Int
    hasinputs::Bool
    hasinits::Bool
end

This is very similar to the Mamba Model, with the exception of VariableSet which is used for defining graph partitions. Mamba models are tightly coupled with samplers. The first step is to completely decouple the DAG api from the samplers. The proposed Model struct is:

mutable struct Model
    nodes::Dict{Symbol,Any}
    hasinputs::Bool
    hasinits::Bool
end

We can also discuss removing hasinputs and hasinits in favour of using ConditionedModel. Also, would we like a separate field for the DAG? At present, we need to call ModelGraph on the Model to generate the graph. ModelGraph is called in the Model constructor to get dependent and target node information but the DAG itself is not saved.

After the Model and samplers have been decoupled, we'll simplify the Node data structures. At the moment, there is a lot of code repetition and specialised dispatch on node types. @yebai suggested this could be simplified using NamedTuples.

codecov[bot] commented 2 years ago

Codecov Report

Merging #44 (56c6f32) into main (04e3e0c) will decrease coverage by 6.75%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
- Coverage   77.06%   70.31%   -6.76%     
==========================================
  Files           2        4       +2     
  Lines         109      128      +19     
==========================================
+ Hits           84       90       +6     
- Misses         25       38      +13     
Impacted Files Coverage Δ
src/simpleppl-test.jl 0.00% <0.00%> (ø)
src/simpleppl.jl 0.00% <0.00%> (ø)
src/varname.jl 78.94% <0.00%> (+1.16%) :arrow_up:

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 04e3e0c...56c6f32. Read the comment docs.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1516249263

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/simpleppl-test.jl 0 4 0.0%
<!-- Total: 0 4 0.0% -->
Totals Coverage Status
Change from base Build 1513239102: -57.5%
Covered Lines: 84
Relevant Lines: 429

💛 - Coveralls
phipsgabler commented 2 years ago

So this is approximately what we're talking about:

m.value == (a = [0, 1], b = 0, c = 0)
m.input == (a = (), b = (), c = (:a, :b))
m.eval == (a = () -> D(), b = () -> 42, c = (a, b) -> Dist(a, 2sqrt(b)))
m.kind == (a = Stochastic, b = Logical, c = Stochastic)
m[@varname(a)] == (value = [0, 1], input = (), eval = ..., kind = Stochastic)
# @varname(a)::VarName{:a, IdentityLens}

# and perhaps
vn = @varname(a[1])
getvalue(m, vn) == get(m.value[getsym(vn)], getlens(vn)) == 0
yebai commented 2 years ago

In general, I wonder if AbstractPPL is the right place for this implementation. The package is supposed to be a lightweight interface package, as far as I understand, but the PR adds quite a lot of code and also not really lightweight dependencies such as Distributions and Graphs.

We plan to keep AbstractPPL lightweight (see my comments above). This PR is a WIP -- most code will be removed. We only plan to keep the core graphical model related implementation in AbstractPPL.

PavanChaggar commented 2 years ago

closing this because the diff has become a bit too messy -- my bad!