Revised Belief Propagation #26
Conversation
Introduce `BeliefPropagationProblem` wrapper to hold the cache and the error `diff` field. Also simplifies some kwargs wrangling.
…be set from another cache
Also includes some fixes to the way `TensorNetwork` types are constructed based on index structure.
for more information, see https://pre-commit.ci
…instead of trying to operate on existing graphs The reason for this is: - One only cares about the edges of the input graph - A simple graph cannot be used as it "forgets" its edge names resulting in recursion - As shown with `TensorNetwork`, removing edges may not always be defined.
…s from an array.
This was caused by the change to the `cache` being backed by a directed graph.
|
@jack-dunham can you resolve comments that you've addressed so it is easier to track what remains? Also can you rebase on main and format to get tests running cleanly? |
…`BeliefPropagationCache`.
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
This method was just forwarding the underlying graph.
An `AbstractTensorNetwork` has edge type `Nothing`, which can be obtained from the `AbstractDataGraph` method.
mtfishman
dismissed
JoeyT1994’s stale review
Joey's requests mostly complete as far as I know.
|
|
||
| abstract type AbstractBeliefPropagationCache{V, VD, ED} <: AbstractDataGraph{V, VD, ED} end | ||
|
|
||
| factor_type(bpc::AbstractBeliefPropagationCache) = typeof(bpc) |
There was a problem hiding this comment.
| factor_type(bpc::AbstractBeliefPropagationCache) = typeof(bpc) | |
| factor_type(bpc::AbstractBeliefPropagationCache) = factor_type(typeof(bpc)) |
Can you add a regression test for that (and related functions that don't have tests)?
| bpc = new{V, VD, ED, E, typeof(digraph)}(digraph, factors, messages) | ||
|
|
||
| for edge in edges(bpc) | ||
| get!(() -> default_message(bpc, edge), messages, edge) |
There was a problem hiding this comment.
This seems bad, doesn't this modify the input messages in-place?
There was a problem hiding this comment.
I guess our plan is to get rid of default_message anyway so this constructor will change accordingly, hopefully we can just remove this entirely. Probably then we can then just use the default constructor and not define an inner constructor.
| factors::Dictionary{V, VD} | ||
| messages::Dictionary{E, ED} |
There was a problem hiding this comment.
Maybe for simplicity we could use Dict here?
There was a problem hiding this comment.
I'm fine either way, it should mostly be an implementation detail. Just curious to hear your thoughts on that.
3 participants
This PR express belief propagation in terms of the new interface based on AlgorithmsInterface.jl and the included AlgorithmsInterfaceExtensions.jl library.