Speeding up adding factors + computing wiring #129

antoine-dedieu · 2022-03-29T00:51:55Z

Main goal of this PR is to speed up the process of adding factors to a FactorGraph and to compile wiring. In particular

we do not instantiate factors in a FactorGraph by default. Instead all the computations, including wiring, happen at the factor groups level.
all the factors-related operations are computed on demand, when the user requires them
we create a subfolder pgmax/groups/ to clarify the structure

Before:

adding factors for the RBM exp takes 25s, building run_bp takes 45s
adding factors for the convor exp takes 20s, building run_bp takes 25s

After:

adding factors for the RBM exp takes 4s, building run_bp takes 4s
adding factors for the convor exp takes 2s, building run_bp takes 12s

Our next step will accelerate the wiring compilation further by using numba

pgmax/fg/graph.py

pgmax/fg/groups.py

codecov-commenter · 2022-03-30T23:19:22Z

Codecov Report

Merging #129 (3d80958) into master (08137ce) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 3d80958 differs from pull request most recent head 46f2ab0. Consider uploading reports for the commit 46f2ab0 to get more accurate results

@@            Coverage Diff            @@
##            master      #129   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        13    +3     
  Lines          854       912   +58     
=========================================
+ Hits           854       912   +58

Impacted Files	Coverage Δ
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%> (ø)`
pgmax/groups/enumeration.py	`100.00% <100.00%> (ø)`
pgmax/groups/logical.py	`100.00% <100.00%> (ø)`
pgmax/groups/variables.py	`100.00% <100.00%> (ø)`
... and 1 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 08137ce...46f2ab0. Read the comment docs.

pgmax/fg/nodes.py

StannisZhou

Made some detailed comments, although nothing major.

It looks like there should be a way to use numba parallel and significantly speed up wiring compilation while unifying the compile wiring interface for all factor groups. So although the current factor group wiring compilation is still a bit annoying, we can go with what we have now after some additional cleanups (since we are likely going to rewrite many of these in the next PR).

examples/gmrf.py

examples/ising_model.py

examples/pmp_binary_deconvolution.py

pgmax/fg/graph.py

StannisZhou · 2022-04-01T06:45:13Z

pgmax/fg/graph.py

    def add_factor(
        self,
        variable_names: List,
        factor_configs: np.ndarray,
-        log_potentials: Optional[np.ndarray] = None,
+        log_potentials: Optional[np.ndarray] = np.empty((0,)),


Why change None to empty?

As I mentioned, for more consistency with the default FactorGroup parent

pgmax/fg/graph.py

antoine-dedieu · 2022-04-01T20:52:09Z

Thanks for your thorough review @StannisZhou
There are a few points we should discuss related to mypy behaviour
I totally agree that the next PR should be focused on speeding the factor group wiring using numba!

for more information, see https://pre-commit.ci

StannisZhou

Left a few more minor comments. LGTM after addressing the comments and making all tests pass!

pgmax/groups/enumeration.py

StannisZhou · 2022-04-04T07:02:06Z

tests/fg/test_graph.py

@@ -32,7 +35,7 @@ def test_factor_graph():
    with pytest.raises(
        ValueError,
        match=re.escape(
-            f"A factor involving variables {frozenset([0])} already exists."
+            f"A {enumeration_factor.EnumerationFactor} involving variables {tuple([0])} already exists."


I updated fg.graph for this part. Please update this test accordingly!

The fix right now is not entirely correct for LogicalFactors, but a proper fix seems unnecessarily complicated. I will create a issue to keep track of this but for now we can go with the not entirely correct solution using frozenset

antoine-dedieu changed the title ~~Speedup factor graph~~ Speedup factor graph creation + wiring Mar 29, 2022

antoine-dedieu changed the title ~~Speedup factor graph creation + wiring~~ Speedup adding factors + computing wiring Mar 29, 2022

antoine-dedieu changed the title ~~Speedup adding factors + computing wiring~~ Speeding up adding factors + computing wiring Mar 29, 2022

StannisZhou self-requested a review March 30, 2022 05:45