Support of message passing for Boolean functions #170

albertpod · 2022-07-20T14:25:24Z

This PR addresses issue #120.

Thanks to @Chengfeng-Jia and @Sepideh-Adamiat for deriving rules.

albertpod · 2022-07-20T14:27:45Z

This PR misses a nice demo demonstrating these functions' applicability.

Once the demo is finished, we can promote this PR.

bartvanerp

First draft of the PR looks good! There are some style issues that need to be tackled and some technical parts, such as:

It would be cool if all these operations could have type aliases, such as AND -> & according to the normal Julia syntax. Not sure how elaborate the required changes will be, but in my opinion required before merging.
The NOT node now has interfaces out and in1. This latter one implies that there is another input. I would recommend renaming it to in (and also in the tests/rules).
In some cases I can image users wanting to use these rules with data variables, I would recommend adding additional rules for Pointmass distributions.
I think we need to extend the Contingency object to include normalization and labels.
I would like to double check the derivations towards the inputs of the AND and OR node on the whiteboard as these are quite tricky and as I ended up with different results.
I did not check the Implication node as I am not sure what it does.

bartvanerp · 2022-07-21T06:54:48Z

src/nodes/implication.jl

+
+struct IMPL end
+
+@node IMPL Deterministic [out, in1, in2]


What is an implication node? As a user I would not know what it does so it either requires: 1) docstrings/documentation or 2) a name change.

I think @bartvanerp's comment is critical and should be addressed. A possible fix would include the truth tables for all logical operations.

Perhaps good to add docstrings for every node. The implication is as "famous" as OR or AND operators.

Ah I see that is quite common apparently. Could we use the abbreviation IMPLY instead according to https://en.wikipedia.org/wiki/Logical_connective?

@Chengfeng-Jia, what is your opinion on renaming the node?

I think IMPLY may be a proper name. Besides, if we decide to use & instead of AND, can we use $\rightarrow$ to represent Implication.

For the Pointmass part, thanks for @bartvanerp explain. I wonder if Pointmass can be regarded as a special case of Bernoulli distribution. For example, $x=1$ means $\mathcal{B}er\left(x \bigg| 1\right)$.

src/nodes/and.jl

src/ReactiveMP.jl

src/nodes/not.jl

src/nodes/or.jl

test/runtests.jl

src/nodes/not.jl

src/rules/or/out.jl

src/rules/and/marginals.jl

ThijsvdLaar

Amazing effort, I agree with the comments by @bartvanerp Most importantly, the Contingency should indeed be normalized, since it represents a joint distribution over connected variables. It agree that it would be nice to write out the derivations in full, for checking, later reference, and most importantly, it can make for an impressive appendix in your thesis.

bvdmitri · 2022-07-25T10:41:04Z

We need to add some sort of example to the documentation Examples section (and demo/ folder). It not only shows how to use these new nodes, but also serves as an extra test because CI fails if some example from the documentation does not run properly.

review-notebook-app · 2022-07-29T12:04:48Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

albertpod · 2022-08-01T11:15:11Z

@bartvanerp @bvdmitri @Chengfeng-Jia I've added the demo demonstrating the usage of these nodes.

codecov-commenter · 2022-08-01T11:30:47Z

Codecov Report

Merging #170 (08d406d) into master (0e1efd8) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 08d406d differs from pull request most recent head 1ef5b94. Consider uploading reports for the commit 1ef5b94 to get more accurate results

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
+ Coverage   76.67%   76.76%   +0.08%     
==========================================
  Files         183      196      +13     
  Lines        6569     6594      +25     
==========================================
+ Hits         5037     5062      +25     
  Misses       1532     1532

Impacted Files	Coverage Δ
src/ReactiveMP.jl	`66.66% <ø> (ø)`
src/distributions/contingency.jl	`100.00% <100.00%> (ø)`
src/rules/and/in1.jl	`100.00% <100.00%> (ø)`
src/rules/and/in2.jl	`100.00% <100.00%> (ø)`
src/rules/and/marginals.jl	`100.00% <100.00%> (ø)`
src/rules/and/out.jl	`100.00% <100.00%> (ø)`
src/rules/implication/in1.jl	`100.00% <100.00%> (ø)`
src/rules/implication/in2.jl	`100.00% <100.00%> (ø)`
src/rules/implication/marginals.jl	`100.00% <100.00%> (ø)`
src/rules/implication/out.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 0e1efd8...1ef5b94. Read the comment docs.

bvdmitri · 2022-08-02T10:04:54Z

Thanks @albertpod , @Chengfeng-Jia and @Sepideh-Adamiat . I'm fine with the current state of the PR. Once tests are passed we can merge.

…_logic

bartvanerp · 2022-08-03T08:10:34Z

After adding docstrings with truth tables I think this PR should be ready for merge

bvdmitri

Great work @albertpod , @Chengfeng-Jia and @Sepideh-Adamiat ! That is very good addition to our framework.

src/rules/and/marginals.jl

bvdmitri · 2022-07-25T10:43:26Z

test/nodes/test_implication.jl

@@ -0,0 +1,20 @@
+module OrNodeTest


Suggested change

module OrNodeTest

module ImplicationNodeTest

bvdmitri · 2022-07-25T10:46:13Z

src/nodes/not.jl

+
+struct NOT end
+
+@node NOT Deterministic [out, in1]


Tests fail because they expect to see :in :)

Suggested change

@node NOT Deterministic [out, in1]

@node NOT Deterministic [out, in]

This change also requires to fix all rules from :in1 to :in

src/rules/and/marginals.jl

Sepideh-Adamiat and others added 15 commits July 14, 2022 14:49

Add OR Node

8b4b755

Add rules for in1 and in2

082065c

add And and Implication node

74ddf64

add Implication node

b7548ec

Update in2

37e4706

Add Not node

6446033

Fix the test for the nodes

7b01a64

add test for AND_IMPL

082a5a8

add test for AND_IMPL

eec109f

Add marginal rules

a3ed124

Make format

ed56cba

add more test exampls

fca65ef

Update rules

547df59

Merge branches

b14b302

Make format

03d9233

albertpod requested review from ThijsvdLaar, bartvanerp, bvdmitri and ismailsenoz July 20, 2022 14:27

bartvanerp requested changes Jul 21, 2022

View reviewed changes

ThijsvdLaar reviewed Jul 21, 2022

View reviewed changes

Add demo

52025ae

albertpod added 4 commits July 29, 2022 14:06

Merge branch 'master' into dev_logic

b07e976

Update

5d58184

Fix tests

6f68b7e

Rename IMPL to IMPLY

b142440

bvdmitri added 2 commits August 1, 2022 17:27

test: fix contingency matrix eltype conversion

d91613d

docs: add new notebook to the documentation examples

19449ff

update ordering

08d406d

bartvanerp mentioned this pull request Aug 2, 2022

Add name aliasing for boolean operations ReactiveBayes/GraphPPL.jl#17

Closed

bvdmitri added 2 commits August 2, 2022 14:31

update notebook output

6d07229

Merge branch 'dev_logic' of github.com:biaslab/ReactiveMP.jl into dev…

1ef5b94

…_logic

bartvanerp approved these changes Aug 2, 2022

View reviewed changes

Update runtests.jl

581dd8b

Add descriptions

afa9870

albertpod marked this pull request as ready for review August 4, 2022 16:15

albertpod added 4 commits August 4, 2022 18:18

Make format

8a435ad

Fix comments

8ec8000

Update docs

4bfdba3

Update examples

9cda713

bvdmitri approved these changes Aug 5, 2022

View reviewed changes

bvdmitri merged commit 93f555e into master Aug 5, 2022

bvdmitri deleted the dev_logic branch August 5, 2022 11:53

bvdmitri mentioned this pull request Aug 5, 2022

Support of message passing for Boolean functions #120

Closed

	@node NOT Deterministic [out, in1]
	@node NOT Deterministic [out, in]

Support of message passing for Boolean functions #170

Support of message passing for Boolean functions #170

Uh oh!

Conversation

albertpod commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertpod commented Jul 20, 2022

Uh oh!

bartvanerp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ThijsvdLaar left a comment

Choose a reason for hiding this comment

Uh oh!

bvdmitri commented Jul 25, 2022

Uh oh!

review-notebook-app bot commented Jul 29, 2022

Uh oh!

albertpod commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bvdmitri commented Aug 2, 2022

Uh oh!

bartvanerp commented Aug 3, 2022

Uh oh!

bvdmitri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

albertpod commented Jul 20, 2022 •

edited

Loading

albertpod commented Aug 1, 2022 •

edited

Loading

codecov-commenter commented Aug 1, 2022 •

edited

Loading