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

[Merged by Bors] - DAG Model interface #47

Closed
wants to merge 36 commits into from
Closed

Conversation

PavanChaggar
Copy link
Collaborator

@PavanChaggar PavanChaggar commented Jan 10, 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> 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

  • pure functions for ordered dictionary, as outlined in AbstractPPL

Feedback on Model structure welcome whilst I implement the remaining features!

@PavanChaggar PavanChaggar marked this pull request as draft January 10, 2022 20:22
@coveralls
Copy link

coveralls commented Jan 10, 2022

Pull Request Test Coverage Report for Build 1679328117

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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+4.0%) to 81.111%

Totals Coverage Status
Change from base Build 1513239102: 4.0%
Covered Lines: 146
Relevant Lines: 180

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #47 (d51a91d) into main (04e3e0c) will increase coverage by 4.37%.
The diff coverage is 86.07%.

❗ Current head d51a91d differs from pull request most recent head f2e86da. Consider uploading reports for the commit f2e86da to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   77.06%   81.44%   +4.37%     
==========================================
  Files           2        3       +1     
  Lines         109      194      +85     
==========================================
+ Hits           84      158      +74     
- Misses         25       36      +11     
Impacted Files Coverage Δ
src/simpleppl.jl 86.07% <86.07%> (ø)
src/varname.jl 78.94% <0.00%> (+1.16%) ⬆️

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...f2e86da. Read the comment docs.

@cpfiffer
Copy link
Member

No substantive comments other than that this is cool! May do a review later when I get more time.

src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
src/simpleppl.jl Outdated Show resolved Hide resolved
@PavanChaggar
Copy link
Collaborator Author

I think all the necessary functions needed for dictionary-like iterations are now supported.

There was a problem with the adjacency matrix function in that it didn't support nodes that had only one input. I've added a fix for this but it's not totally type stable so still working on it.

@PavanChaggar
Copy link
Collaborator Author

@yebai I've merged ModelState and DAG together into one GraphInfo type. I've also just replaced the Model type with GraphInfo. Now when you call the Model constructor it returns a GraphInfo type. Is that ok or do you think it is better to have a dedicated Model type with field g::GraphInfo?

@yebai
Copy link
Member

yebai commented Feb 6, 2022

@PavanChaggar thanks, let's still keep a dedicated Model type. We might add more fields to Model later. In addition, it is more consistent with the Model type in DynamicPPL, which we could bring closer in the future. In my mind, model = CodeInfo + GraphInfo/VarInfo, where CodeInfo is not yet introduced but could represent the model AST/IR.

@yebai yebai changed the title [WIP] DAG Model interface DAG Model interface Feb 6, 2022
@yebai
Copy link
Member

yebai commented Feb 6, 2022

Let's also rename simpleppl.jl to graphinfo.jl.

@PavanChaggar PavanChaggar marked this pull request as ready for review February 7, 2022 10:24
@phipsgabler
Copy link
Member

I'd still like it to go into a separate submodule, so that it's clear that this is the part for one, concrete implementation.

src/AbstractPPL.jl Outdated Show resolved Hide resolved
@@ -17,4 +16,10 @@ include("abstractmodeltrace.jl")
include("abstractprobprog.jl")
include("deprecations.jl")

# GraphInfo
module GraphPPL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
module GraphPPL
module GraphInfo

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'm allowed to call it GraphInfo. Otherwise it prompts the following error:

ERROR: LoadError: LoadError: invalid redefinition of constant GraphInfo

src/AbstractPPL.jl Outdated Show resolved Hide resolved
src/graphinfo.jl Outdated Show resolved Hide resolved
src/graphinfo.jl Show resolved Hide resolved
src/graphinfo.jl Outdated Show resolved Hide resolved
src/graphinfo.jl Show resolved Hide resolved
Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - many thanks @PavanChaggar! I left some minor comments above.

@yebai
Copy link
Member

yebai commented Feb 7, 2022

bors r+

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!
@bors bors bot changed the title DAG Model interface [Merged by Bors] - DAG Model interface Feb 7, 2022
@bors bors bot closed this Feb 7, 2022
@yebai yebai deleted the pc/simpleppl branch February 7, 2022 17:21
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.

None yet

6 participants