Add SamplePSDD #79

RenatoGeh · 2021-06-23T21:08:35Z

Hi,

This PR proposes adding the SamplePSDD learning algorithm that is to appear at UAI 2021. If you guys think this is a good fit for Juice, I'd be happy to add the code to the library and make any adjustments to comply with Juice notation or anything like that.

I wrote some tests, but it seems tests were already failing for me before doing anything to the code. I'm not sure if it's because of my GPU (fails at the parameter estimation GPU tests) or if it's something I introduced in the PR. Even though it's in a completely different section of the code, the stack trace says it has something to do with the BDD dependency (which is weird, since it shouldn't have anything to do with it).

The CI also appears to give out an error when running the tests I added. Strangely, the CI log gives no clue whatsoever on what caused them, and running it on my local machine outputs no error on these particular test cases. Running the test files by themselves also gives no errors. I'm surely missing something here, but have no idea what exactly.

We tried adding a minimal number of dependencies, but ultimately we ended up having to import the following externals:

BinaryDecisionDiagrams.jl: for the BDDs;
ThreadPools.jl: for non-uniform threading;
Distributions.jl: for Dirichlet sampling.

To avoid namespace shenanigans, I imported the BDD dependency in its own namespace, since there are quite a few name conflicts with LogicCircuits.

As to notation/naming conventions, I somewhat tried mimicking your namings, but not sure if I'm 100% compliant.

The src/structurelearner/sample_psdd.jl file implements SamplePSDD and any functions it needs. src/ensembles/ensembles.jl adds a type Ensemble for mixtures of not-necessarily-same-vtree probabilistic circuits. It adds the weight learning methods we mention in the paper (namely EM, likelihood weighting, uniform weights and stacking of mixtures). File src/ensembles/bmc.jl adds a BayesModelComb type for Bayesian Model Combination of PCs.

Let me know if you want me to change anything in the code.

Thanks

khosravipasha · 2021-06-24T01:12:55Z

Thanks for the PR, this should be very useful. I will have some time to review in more details next week or so.

For the tests, the "slow tests" is fine and expected to fail, the "unit tests" portion is currently run on CPU, for the GPU tests we run manually using customized github runner.

Yesh the stack traces are not that informative, and that's why we have a way of running individual tests
https://github.com/Juice-jl/ProbabilisticCircuits.jl/actions/workflows/one_test.yml
You can probably replicated that on your clone and run that action to get better stack trace withing the same CI environment.

RenatoGeh · 2021-06-28T21:51:48Z

Thanks for the tips, Pasha. I added a few more tests, added I/O writing for Ensembles and added a function to compile a BDD into a PSDD. That last one's kind of out of place, but since we used that as a baseline for the paper, I added it to the PR as well. Feel free to ignore that commit if you think it's best.

On the issue of tests, I ran the individual tests and they passed with no issues:

In the unit tests workflow, these same tests do pass, but they end up crashing some place else. This same (or at least similar) error seems to be occurring on Juice-jl:master as well.

guyvdbroeck · 2021-06-29T01:23:24Z

You might want to merge the latest bug fix to master than gets rid of the failing MAP inference test.

RenatoGeh · 2021-06-29T10:25:56Z

Great, thanks Guy. Rebased master and now tests seem to pass fine.

khosravipasha

Thanks for the PR.
Overall, looks good to me, few comments inside.

What's the difference of BayesModelComb and Ensemble? In terms of storage/types they look the same. We also have SharedProbCircuit (but that's slightly different since the structures are shared). Here I assume each ProbCircuit can have different structure right?
Also might want to rename Ensemble slightly, since its for a specific usecase when we have bdd and logical constraints (unless I read it wrong)
I need to double check some interations between the threads and gpu later but if it works now then should be fine. One concern is for bigger circuits having lots of threads might lead to memory issues. Can worry about this later I guess.

It was a lot of code so did not take a full pass yet, but overall looks good to me. Will take another pass early next week, then can merge it.

khosravipasha · 2021-07-03T03:41:09Z

src/ensembles/ensembles.jl

+
+"""Split `X` into two partitions `A` and `B`, where `A` is a Bernoulli sample of each element in
+`X` with probability `p` and `B=X∖A`. Guarantees at least one element in `A` and `B`."""
+function bernoulli_partition(X::Vector{Int}, p::Float64)::Tuple{Vector{Int}, Vector{Int}}


This function seems general enough, maybe can move it to Utils or something (not a big deal).
https://github.com/Juice-jl/ProbabilisticCircuits.jl/tree/master/src/Utils

Alright. Will do.

khosravipasha · 2021-07-03T03:42:32Z

src/ensembles/ensembles.jl

+    2. `:uniform` for uniform weights;
+    3. `:em` for Expectation-Maximization;
+    4. `:stacking` for mixture model Stacking;
+"""


Awesome, thanks for the detailed comments. Also feel free to add link to your paper(s) on functions that are specific to that paper, thinnks this one would be one of them.

UAI is still on preliminaries. But I'll add them once UAI releases the final version. Thanks.

khosravipasha · 2021-07-03T03:43:24Z

src/ensembles/ensembles.jl

+end
+
+"Samples a Vtree with a right bias of `p`. If `p<0`, then uniformly sample vtrees."
+function sample_vtree(n::Int, p::Float64)::Vtree


similar comment to bernoulli_partition

khosravipasha · 2021-07-03T03:43:51Z

src/ensembles/ensembles.jl

+end
+
+"Returns a(n index) partitioning a la k-fold."
+function kfold(n::Int, p::Int)::Vector{Tuple{UnitRange, Vector{Int}}}


similar comment to bernoulli_partition (i.e. move to utils)

khosravipasha · 2021-07-03T03:45:17Z

src/structurelearner/bdd.jl

+    return passdown(U)
+end
+
+"Returns a structured probabilistic circuit compiled from a binary decision diagram."


nice, this is going to be very useful :)

this works with any BDD? any restriction to know about?

Should work with any BDD. I guess a restriction is that it's limited to the BDD.jl implementation: we assume a fixed (arbitrary) ordering, and so the PSDD will compile from a particularly ordered BDD.

khosravipasha · 2021-07-03T03:55:37Z

src/ensembles/ensembles.jl

+    return F
+end
+
+"Learns the weights of the Ensemble by Stacking, with `k` as the number of folds in k-fold."


If I recall correctly, tacking is using outcome of another model as extra features, but that was mostly for classifiers. Is that also the case here?

Yeah, that's the idea, but in this case we used a density estimation version. Here's the reference paper.

RenatoGeh · 2021-07-05T14:58:01Z

Thanks for the review, Pasha. Answering questions inline.

Thanks for the PR.
Overall, looks good to me, few comments inside.
1. What's the difference of `BayesModelComb` and `Ensemble`? In terms of storage/types they look the same. 

The difference is that BayesModelComb is an ensemble of Ensembles. Here's the reference paper we based the algorithm on.

We also have SharedProbCircuit (but that's slightly different since the structures are shared). Here I assume each ProbCircuit can have different structure right?

Yeah. Ensemble doesn't assume structure.

   Also might want to rename `Ensemble` slightly, since its for a specific usecase when we have bdd and logical constraints (unless I read it wrong)

I kept it Ensemble since the mixture's weights are learned independent of the components, and so thought it might be useful to keep it as generic as possible. The idea is that we provide a particular case for SamplePSDD through ensemble_sample_psdd, but we could very well bundle up some SPNs some other way and learn ensemble weights with the existing functions.

2. I need to double check some interations between the threads and gpu later but if it works now then should be fine. One concern is for bigger circuits having lots of threads might lead to memory issues. Can worry about this later I guess.
It was a lot of code so did not take a full pass yet, but overall looks good to me. Will take another pass early next week, then can merge it.

Awesome. :) As a side note, I forgot to add some documentation. I'll write some and push them. I'll mark this WIP until I do.

guyvdbroeck · 2021-07-07T06:11:46Z

Would it be realistic to move the BinaryDecisionDiagrams.jl dependency and related functionality to LogicCircuits.jl? It already has an SDD compiler (which of course can also compile BDDs by fixing a right-linear vtree). I would prefer to keep all purely logical features there.

RenatoGeh · 2021-07-08T12:16:09Z

Do you mean moving the BDD code into LogicCircuits.jl? Or re-implementing the code using LogicCircuits.jl syntax through the existing types? I see some problems with the latter, since SamplePSDD relies on efficiently reducing, restricting and applying BDDs, which could introduce some overhead if we try to generalize with logic circuits. In the case of the former, I'd personally see no problem in moving BinaryDecisionDiagrams.jl to LogicCircuits.jl. Although one issue would be that the code would be somewhat disconnected from the rest.

guyvdbroeck · 2021-07-08T15:29:43Z

I would prefer to move the BDD code into LogicCircuits.jl. Perhaps in future then we can have some interoperability between your BDD code and the other logic circuit types, some conversions, support for the LogicCircuits.jl compilation API, etc.

RenatoGeh · 2021-07-08T19:07:39Z

Yeah, I see no problem with that. I'll send the PR to LogicCircuits.jl.

khosravipasha · 2021-08-28T01:30:08Z

@RenatoGeh
Recently upgraded the DataFrames dependency to 1.x. In this new version, there is a breaking change that cannot do DataFrame(matrix) anymore, and need to replace it with DataFrame(matrix, :auto). In case you ran into any issues.

Also the LogicCircuit PR is now pushed and in master.

RenatoGeh · 2021-08-30T14:07:11Z

@RenatoGeh
Recently upgraded the DataFrames dependency to 1.x. In this new version, there is a breaking change that cannot do DataFrame(matrix) anymore, and need to replace it with DataFrame(matrix, :auto). In case you ran into any issues.

Also the LogicCircuit PR is now pushed and in master.

Thanks for heads up, Pasha! I did have one issue when writing the docs. I wanted to show how to build a PSDD from a CNF file and ran into #80. Would you rather we deal with this in another PR and try to merge this right now, or try to resolve this in this one?

khosravipasha · 2021-08-30T19:00:34Z

Thanks for heads up, Pasha! I did have one issue when writing the docs. I wanted to show how to build a PSDD from a CNF file and ran into #80. Would you rather we deal with this in another PR and try to merge this right now, or try to resolve this in this one?

@RenatoGeh Yes this seems to be a separate issue, we can merge this one first and do the fix and extra docs in future commits. Thanks.

RenatoGeh · 2021-09-03T16:27:17Z

Ok, thanks. Refactored everything to use LogicCircuits' BDDs now and added a very simple how-to in the manuals. From what I gather, tests are not passing because LogicCircuits' version set in Project.toml points to version 0.2.3, which doesn't have the recent required changes.

khosravipasha · 2021-09-03T21:36:34Z

Thanks Renato, will take a look at the tests, probably config issue cause the tests should also grab logic circuits from master in dev mode. I will just run them locally if not. We have been meaning to release a new version anyway.

khosravipasha · 2021-09-04T02:21:59Z

Fixed the tests errors, ran it locally and it passed, will let tests run here too, and will merge sometime next week if there is no other changes needed.

codecov-commenter · 2021-09-04T02:37:19Z

Codecov Report

Merging #79 (efcfaf6) into master (14fbf66) will increase coverage by 1.71%.
The diff coverage is 80.76%.

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

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   67.33%   69.05%   +1.71%     
==========================================
  Files          40       45       +5     
  Lines        3043     3490     +447     
==========================================
+ Hits         2049     2410     +361     
- Misses        994     1080      +86

Impacted Files	Coverage Δ
src/LoadSave/circuit_loaders.jl	`40.90% <0.00%> (-43.47%)`	⬇️
src/LoadSave/circuit_savers.jl	`78.35% <0.00%> (-21.65%)`	⬇️
src/ProbabilisticCircuits.jl	`100.00% <ø> (ø)`
src/Utils/Utils.jl	`100.00% <ø> (ø)`
src/structurelearner/sample_psdd.jl	`87.76% <87.76%> (ø)`
src/ensembles/bmc.jl	`92.68% <92.68%> (ø)`
src/structurelearner/bdd.jl	`94.44% <94.44%> (ø)`
src/ensembles/ensembles.jl	`97.14% <97.14%> (ø)`
src/Utils/misc.jl	`56.57% <100.00%> (+17.69%)`	⬆️
src/Utils/sample.jl	`100.00% <100.00%> (ø)`
... and 5 more

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 14fbf66...d6da646. Read the comment docs.

RenatoGeh · 2021-09-06T11:57:28Z

Alright, awesome. Thanks for that last fix, review and feedback. Just one last thing about the docs. I mention how to compile a PSDD from an SDD, but as we know that's broken for now. Just giving you a heads up in case we have anyone asking about it in the issues.

khosravipasha · 2021-09-07T05:57:46Z

Great, thanks. Yeah for the SDD => PSDD, is it that same as the issue #80?

Will merge this PR now, if there is anything else we can make extra issues.

RenatoGeh · 2021-09-07T11:27:33Z

Great, thanks. Yeah for the SDD => PSDD, is it that same as the issue #80?

It is.

Will merge this PR now, if there is anything else we can make extra issues.

Sure.

RenatoGeh force-pushed the samplepsdd branch 2 times, most recently from 0b56db0 to e6b04f8 Compare June 24, 2021 14:41

RenatoGeh force-pushed the samplepsdd branch from 7393375 to 505b4a0 Compare June 29, 2021 10:01

khosravipasha approved these changes Jul 3, 2021

View reviewed changes

RenatoGeh changed the title ~~Add SamplePSDD~~ [WIP] Add SamplePSDD Jul 5, 2021

RenatoGeh mentioned this pull request Aug 10, 2021

Add BinaryDecisionDiagrams.jl to Juice Tractables/LogicCircuits.jl#90

Merged

RenatoGeh added 11 commits September 3, 2021 12:27

Add SamplePSDD

55f9da6

Add dependencies

ce827c9

Add SamplePSDD tests

3fd30c7

Add Ensemble and BMC + tests

31bb41d

Shell BDD inside a namespace to avoid conflicts

7242a9d

Add Ensemble I/O

98496fc

Add structural tests to SamplePSDD

34ff73f

Add compilation of BDD to PC

427345c

Move general purpose functions to Utils

183d5e2

Refactor to reference LogicCircuit's BDDs

e8dfb56

Add simple howtos for SamplePSDD and BDD compilation

1c8b2e9

RenatoGeh force-pushed the samplepsdd branch from e90178a to 1c8b2e9 Compare September 3, 2021 15:34

RenatoGeh changed the title ~~[WIP] Add SamplePSDD~~ Add SamplePSDD Sep 3, 2021

fix import and dataframe issues

d6da646

khosravipasha merged commit 43523f1 into Tractables:master Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SamplePSDD #79

Add SamplePSDD #79

RenatoGeh commented Jun 23, 2021

khosravipasha commented Jun 24, 2021

RenatoGeh commented Jun 28, 2021

guyvdbroeck commented Jun 29, 2021

RenatoGeh commented Jun 29, 2021

khosravipasha left a comment

khosravipasha Jul 3, 2021

RenatoGeh Jul 5, 2021

khosravipasha Jul 3, 2021

RenatoGeh Jul 5, 2021

khosravipasha Jul 3, 2021

khosravipasha Jul 3, 2021

khosravipasha Jul 3, 2021

RenatoGeh Jul 5, 2021

khosravipasha Jul 3, 2021

RenatoGeh Jul 5, 2021

RenatoGeh commented Jul 5, 2021

guyvdbroeck commented Jul 7, 2021

RenatoGeh commented Jul 8, 2021

guyvdbroeck commented Jul 8, 2021

RenatoGeh commented Jul 8, 2021

khosravipasha commented Aug 28, 2021

RenatoGeh commented Aug 30, 2021

khosravipasha commented Aug 30, 2021

RenatoGeh commented Sep 3, 2021

khosravipasha commented Sep 3, 2021 via email

khosravipasha commented Sep 4, 2021

codecov-commenter commented Sep 4, 2021

RenatoGeh commented Sep 6, 2021

khosravipasha commented Sep 7, 2021

RenatoGeh commented Sep 7, 2021

Add SamplePSDD #79

Add SamplePSDD #79

Conversation

RenatoGeh commented Jun 23, 2021

khosravipasha commented Jun 24, 2021

RenatoGeh commented Jun 28, 2021

guyvdbroeck commented Jun 29, 2021

RenatoGeh commented Jun 29, 2021

khosravipasha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RenatoGeh commented Jul 5, 2021

guyvdbroeck commented Jul 7, 2021

RenatoGeh commented Jul 8, 2021

guyvdbroeck commented Jul 8, 2021

RenatoGeh commented Jul 8, 2021

khosravipasha commented Aug 28, 2021

RenatoGeh commented Aug 30, 2021

khosravipasha commented Aug 30, 2021

RenatoGeh commented Sep 3, 2021

khosravipasha commented Sep 3, 2021 via email

khosravipasha commented Sep 4, 2021

codecov-commenter commented Sep 4, 2021

Codecov Report

RenatoGeh commented Sep 6, 2021

khosravipasha commented Sep 7, 2021

RenatoGeh commented Sep 7, 2021