Confirm clique sub-graphs only use priors once

[Affie]

The tree

fg_built, see l0f1

fg_beforeupsolve look like it is used

[dehann]

yes this is a bug, the :l0f1 should only occur once. have to double check if it should first occurrence or frontals only. The hard part is how do this for initialization.

[Affie]

By the same logic: Should all factors not only be used once? On frontals.
This assumes upinit,downinit,upsolve,downsolve.
see x3lm3f1
Note, I'm only looking at subgraphs fg_beforeupsolve (message priors included) so don't know what else the code might do.
Tree

Clique 1 - x3,lm0,lm3,x1

Clique 2 - x2: x1,x3,lm3

[Affie]

New function to only copy factors on frontals cc @dehann
see #472

[dehann]

Hi @Affie , thanks two things:

• trying to consolidate the now three different buildsubgraph functions has been bit of a lift, and we still have the three separate functions, although they are (almost all) together in DFG v0.5.2 now. For future, work I'd request to please help look at how we can drop the 3 build sub graph functions down to just one common function please. See:
  • buildSubgraphFromLabels! function in needsahome.jl DistributedFactorGraphs.jl#243
  • sub-graphs with only factors on frontals connected #472, Refactor #491 (Refactor of twig/test/priorusetest #490)
• The examples in this issue are great thanks, but we do not have access to the factor graphs you used to make them... Could you perhaps indicate what the original factor graphs look like please? I'd like to incorporate this as a permanent test in IIF.
  • Maybe even just a count of all factors in each of the two examples?
    • x0x1x2x3x4l0l1 has these priors and binary factors?
    • x0x1x2x3l0lm1lm2lm3 has those priors and binary factors?

[dehann]

attempt to recreate

First Example:

using IncrementalInference

fg = initfg()

addVariable!(fg, :x0, ContinuousScalar)
addVariable!(fg, :x1, ContinuousScalar)
addVariable!(fg, :x2, ContinuousScalar)
addVariable!(fg, :x3, ContinuousScalar)
addVariable!(fg, :x4, ContinuousScalar)

addVariable!(fg, :l0, ContinuousScalar)
addVariable!(fg, :l1, ContinuousScalar)

lc = LinearConditional(Normal())
lp = Prior(Normal())
addFactor!(fg, [:x0;:x1], lc, autoinit=false)
addFactor!(fg, [:x1;:x2], lc, autoinit=false)
addFactor!(fg, [:x2;:x3], lc, autoinit=false)
addFactor!(fg, [:x3;:x4], lc, autoinit=false)

addFactor!(fg, [:x0;:l0], lc, autoinit=false)
addFactor!(fg, [:x2;:l0], lc, autoinit=false)

addFactor!(fg, [:x0;:l1], lc, autoinit=false)
addFactor!(fg, [:x2;:l1], lc, autoinit=false)

addFactor!(fg, [:x0;], lp, autoinit=false)
addFactor!(fg, [:l0;], lp, autoinit=false)

vo = Symbol[:x2, :x0, :l0, :x3, :x1, :l1, :x4]
tree = resetBuildTreeFromOrder!(fg, vo)

drawTree(tree, show=true)

Right, current version of code, with dbg.fg_build graphs (i.e. no MsgPriors) and the prior on :l0 is double counted, specifically on the separator of the leaf clique:

[dehann]

As suggested by @Affie , priors on separators should not be added.

[dehann]

Ensuring that we properly resolve Ex2: PR #516 :

NOTE - the factor selection issues are likely already fixed in IIF v0.8.3, just adding more tests to ensure this is the case. This bug & update is part of the larger migration to CSM based solver.