FactorGraph supports any type of factors + runs specialized inference for ORFactors

[antoine-dedieu]

In this PR we

1. Redefine the factor graph abstraction by introducing factor types: factors in a graph are clustered in factor groups, which are grouped according to their factor types. See fg/graph.py
2. Specify two types of factors: EnumerationFactor (this class already existed) and ORFactor (this new class inherits from the new LogicalFactor). Each Factor class must have its own methods to compile and concatenate Wirings for inference. See factors/enumeration.py and factors/logical.py.
3. Make running inference in a graph agnostic to the current type of factors supported. New factors types can then be added without modifying graph.py.
4. Implement a specialized inference for ORFactors (see pass_OR_fac_to_var_messages in factors/logical.py) and compare it with the existing one for EnumerationFactors in the unit test tests/factors/test_or.py

[StannisZhou]

First round of comments. Some main issues:

1. Look into adding support for different temperatures
2. Improve naming. Might be better to stick to parents/children instead of top/bottom, and explicitly use positive/incoming instead of just pos/inc.
3. Move the function for computing maxes/argmaxes out as a reusable, standalone function.
4. Should not support single parent case
5. In general more comments would help

[antoine-dedieu]

@StannisZhou thanks for your comments. The main points to discuss are

1. can we support any temperature (ie derive simple expression)?
2. should we support the single parent case -- I think we should
3. what do we think about attaching the supplementary of the PMP Neurips paper for proper derivation of the equations?

[codecov-commenter]

Codecov Report

Merging #122 (58d6a6d) into master (716abd6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            master      #122    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            8        10     +2     
  Lines          671       857   +186     
==========================================
+ Hits           671       857   +186     

Impacted Files	Coverage Δ
pgmax/bp/bp_utils.py	100.00% <100.00%> (ø)
pgmax/bp/infer.py	100.00% <100.00%> (ø)
pgmax/factors/__init__.py	100.00% <100.00%> (ø)
pgmax/factors/enumeration.py	100.00% <100.00%> (ø)
pgmax/factors/logical.py	100.00% <100.00%> (ø)
pgmax/fg/graph.py	100.00% <100.00%> (ø)
pgmax/fg/groups.py	100.00% <100.00%> (ø)
pgmax/fg/nodes.py	100.00% <100.00%> (ø)

---

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 716abd6...58d6a6d. Read the comment docs.

[antoine-dedieu]

@StannisZhou I have started a draftr or the ORFactor and the ORFactorGroup. Let me know what you think and we can take it from there.
Note that we can implement this as follow:

parents_variables = groups.NDVariableArray(num_states=2, shape=(5,))
children_variable = groups.NDVariableArray(num_states=2, shape=(2,))
fg = graph.FactorGraph(
    variables=dict(parents=parents_variables, children=children_variable),
)
parents_names_for_factors = [
    [("parents", 0), ("parents", 1), ("parents", 2)],
    [("parents", 3), ("parents", 4)]
]
children_names_for_factors = [("children", 0), ("children", 1)]

fg.add_factor_group(
    factory=groups.ORFactorGroup,
    parents_names_for_factors=parents_names_for_factors,
    children_names_for_factors=children_names_for_factors,
)

[StannisZhou]

Left some comments. Will look at the message updates with different temperature part more carefully later.

One high-level comment is, one way to see how good our abstractions are is to think about how painful it would be in the future to add a new type of factor. This PR is definitely a bit painful. It would be helpful to make sure after this PR it would be much less painful. And important things to consider include minimizing the number of places we need to touch when adding new factors in the future (e.g. organizing wirings in a dictionary instead of separate enumerations), and isolate places that need updating to be non-disruptive to other parts of the codebase.

[antoine-dedieu]

@StannisZhou the unit test now builds 2 factors graphs with Enumeration vs OR factors and compares both general vs specialized factor to variables message updates.

[StannisZhou]

Mostly minor comments. We can merge this after these comments are addressed!

Before merging, could you also check to make sure all the current examples (e.g. RBM, RCN, GMRF) still run fine? In the unit test only the ising model example is checked.

[antoine-dedieu]

@StannisZhou with the recent changes:

• all the old + new unit tests pass
• coverage is at 100%
• the RCN, RBM, Ising, GMRF examples run fine

[StannisZhou]

LGTM after fixing the docstrings. Excited to have this merged!