Hybrid Factor Graph Implementation #1203

ProfFan · 2022-05-21T21:37:53Z

…id-tr

dellaert

Fantastic progress. Some more comments (and some important comments were added before I did a re-review) because I still don’t see eye to eye yet on some naming etc…. Also, I think we should go for HybraidGaussianXXX rather than GaussianHybridXXX because everything in this folder should mimick what is in linear and non-linear, except preceded by Hybrid.

dellaert · 2022-06-01T22:33:36Z

gtsam/hybrid/GaussianMixtureConditional.h

+#include <gtsam/linear/GaussianConditional.h>
+
+namespace gtsam {
+class GaussianMixtureConditional


The math in the comment is too generic. I think this class is a mixture of Gaussian, so expect to see that detail :-) Please check GaussianConditional.h for an example.

dellaert · 2022-06-01T22:34:03Z

gtsam/hybrid/GaussianMixtureConditional.h

+   * @param continuousFrontals the continuous frontals.
+   * @param continuousParents the continuous parents.
+   * @param discreteParents the discrete parents. Will be placed last.
+   * @param conditionals a decision tree of GaussianConditionals.


Not resolved

dellaert · 2022-06-01T22:38:51Z

gtsam/hybrid/HybridFactorGraph.cpp

+
+/* ************************************************************************ */
+std::pair<HybridConditional::shared_ptr, HybridFactor::shared_ptr>  //
+EliminateHybrid(const HybridFactorGraph &factors, const Ordering &frontalKeys) {


Why cant’t we do this in this PR? It will make the pruning PR also easier to review.

dellaert · 2022-06-01T22:40:24Z

gtsam/hybrid/HybridFactorGraph.cpp

+  // discrete variable, no continuous factor is allowed (escapes CG regime), so
+  // we panic, if discrete only we do the discrete elimination.
+
+  // However it is not that simple. During elimination it is possible that the


I’d love this important PR to be self-contained, so could the comment be adapted?

dellaert · 2022-06-01T22:41:16Z

gtsam/hybrid/HybridFactorGraph.h

+/**
+ * Hybrid Factor Graph
+ * -----------------------
+ * This is the linear version of a hybrid factor graph. Everything inside needs


I agree, let’s rename this, but since we have hybridGaussianFactor should this not be HybridGaussianFactorGraph?

dellaert · 2022-06-01T22:44:24Z

gtsam/hybrid/HybridISAM.h

+ * -------------------------------------------------------------------------- */
+
+/**
+ * @file HybridISAM.h


HybridGaissianISAM, to be consistent.

dellaert · 2022-06-01T22:45:06Z

gtsam/hybrid/HybridJunctionTree.cpp

+ * -------------------------------------------------------------------------- */
+
+/**
+ * @file HybridJunctionTree.cpp


HybridGaussianJunctionTree?

Break up EliminateHybrid into smaller functions

…torGraph

…Graph

dellaert

Awesome. Still some nits, and I am not sure all classes have the right name yet.
Some HybridXX need to become HybridGaussianXX?
Since this is still in alpha I think we can leave to next PR...

dellaert · 2022-06-03T00:03:18Z

gtsam/hybrid/GaussianMixture.h

+ * variable, M is the selection of discrete variables corresponding to a subset
+ * of the Gaussian variables and Z is parent of this node
+ *
+ * The negative log-probability is given by \f$ \sum_{m=1}^M \pi_m \frac{1}{2}


Nit: this is probably wrong. The mixture is \sum N(x; R,S,T,...d) = \sum exp -0.5 |...|^2 so the negative log would be -log \sum exp -0.5 |...|^2 which is (unfortunately) not \sum 0.5 |...|^2. But, in Conditionals we don't really care about the neg log, so we can just say

The probability P(x|y,z,...) is proportional to \f$ \sum_i k_i \exp - \frac{1}{2} |R_i x - (d_i - S_i y - T_i z - ...)|^2 \f$ where i indexes the components and k_i is a component-wise normalization constant.

I was thinking about this today morning. Let me redo it.

Also, github now supports latex:

$\sum_i k_i \exp - \frac{1}{2} |R_i x - (d_i - S_i y - T_i z - ...)|^2$

dellaert · 2022-06-03T00:14:31Z

gtsam/hybrid/HybridGaussianFactorGraph.cpp

+  }
+
+  auto result = EliminatePreferCholesky(gfg, frontalKeys);
+  return std::make_pair(


does just

return {result.first, result.second};

not work?

Not sure. Most of the comments in this round is from Fan's code that I didn't feel needed any updates. :)

No, CI will fail (I initially did {})

Main culprit is old GCC, we will be free of these problems once we move to C++14/17.

dellaert · 2022-06-03T00:14:42Z

gtsam/hybrid/HybridGaussianFactorGraph.cpp

+
+  auto result = EliminateDiscrete(dfg, frontalKeys);
+
+  return std::make_pair(


dellaert · 2022-06-03T00:16:06Z

gtsam/hybrid/HybridGaussianFactorGraph.cpp

+  GaussianFactorGraph gfg;
+  for (auto &fp : factors) {
+    auto ptr = boost::dynamic_pointer_cast<HybridGaussianFactor>(fp);
+    if (ptr) {


I think you can do (here and below)

if (auto hgf = boost::dynamic_pointer_cast<HybridGaussianFactor>(fp)) {...

dellaert · 2022-06-03T00:17:36Z

gtsam/hybrid/HybridGaussianFactorGraph.cpp

+  DiscreteKeys discreteSeparator(discreteSeparatorSet.begin(),
+                                 discreteSeparatorSet.end());
+
+  // sum out frontals, this is the factor on the separator


Can we make this a separate function as well?

dellaert · 2022-06-03T00:18:21Z

gtsam/hybrid/HybridGaussianFactorGraph.cpp

+
+  gttoc(sum);
+
+  using EliminationPair = GaussianFactorGraph::EliminationResult;


Comment on what's happening...

varunagrawal · 2022-06-03T02:52:49Z

Awesome. Still some nits, and I am not sure all classes have the right name yet. Some HybridXX need to become HybridGaussianXX? Since this is still in alpha I think we can leave to next PR...

I was wondering about this. We have GaussianEliminationTree, SymbolicEliminationTree and DiscreteEliminationTree, so should this be just HybridEliminationTree? Same question for HybridJunctionTree since these two only come into play after linearization so Gaussian is implied.

@dellaert once this question is resolved, I can merge.

ProfFan · 2022-06-03T03:15:53Z

Thanks @varunagrawal!

varunagrawal · 2022-06-03T16:45:16Z

gtsam/hybrid/GaussianMixture.h

+ *
+ */
+class GTSAM_EXPORT GaussianMixture
+    : public HybridFactor,


@ProfFan @dellaert why is this inheriting from HybridFactor and not HybridGaussianMixtureFactor like in the previous scheme? Do we not need to record the conditionals as factors in the base class anymore?

I lost track of this - @ProfFan might have the answer

I think this can be done that way, just what was on my mind is that I want to keep the two things separate, as they don't necessarily need to have inheritance relationship. We can revisit this later.

Sorry but this is an early design decision that needs to be resolved ASAP.

My understanding was that eliminating a HybridMixtureFactor should result in a GaussianMixture conditional, so there already exists an intrinsic relationship between the two.

My main concern is what happens when we only eliminate a partial set of the keys? If the factor is F(X1, X2, M1) and we only eliminate X1, shouldn't the gaussian factors for X1 still be captured in the conditional until even X2 is eliminated?

I actually agree with Varun: a conditional is a factor. No sense to have extra storage in this conditional for the Gaussian components, if that is going on.

However, I don't fully understand how the elimination example is relevant. We don't eliminate conditionals, we eliminate factors, so not sure how it bears on this discussion.

The GaussianMixtureFactor is a container for other factors via the decision tree with Gaussian FGs as the leaves. So if we eliminate one variable, I don't remember how that affects the other variables within the tree. What is the expected behavior?

My understanding is that it should be a conditional on the variable with the remaining variables as parents, so does this mean the conditional would be dependent on variables still a part of factors? In that case we should be recording the factors. However the final Bayes network will only have conditionals, so the GaussianMixtures will have to be retroactively updated to reflect the new conditionals for parents.

I'll go back and look at the behavior from the previous scheme.

dellaert · 2022-06-03T17:05:15Z

Awesome. Still some nits, and I am not sure all classes have the right name yet. Some HybridXX need to become HybridGaussianXX? Since this is still in alpha I think we can leave to next PR...

I was wondering about this. We have GaussianEliminationTree, SymbolicEliminationTree and DiscreteEliminationTree, so should this be just HybridEliminationTree? Same question for HybridJunctionTree since these two only come into play after linearization so Gaussian is implied.

@dellaert once this question is resolved, I can merge.

OK, that makes sense. Yes, HybridEliminationTree and HybridJunctionTree I guess.

ProfFan added 30 commits March 11, 2022 15:51

Fix typo

ea4ebb6

Prototype a type-erased hybrid machinery

0567303

Add better mocking for actual elimination

efa37ff

Fix compilation error

3aac52c

Add shorthand for inserting raw JacobianFactor

53551e0

Add full elimination

095f6ad

More mock-ups added

2bae286

Added mixture factor functionality

ee4f9d1

Added more mockups and color output of the elimination process

5ea614a

add more comments

0aeb596

even better printing and comments

fe5dde7

Address comments

f237438

Missing pragma onces

36ee4ce

Now we have real multifrontal!

7ad2031

Fully working Multifrontal

d42fc21

Add a test for EliminateSequential

1e8aae3

Address comments

b4f8eea

Remove warning

4f8dfeb

Address comments

293ef61

Display debug messages only when DEBUG = true

a36c86a

Add doxygen for GMM

8b4283b

Address compilation error on GCC

5f03e0f

Fix GCC error

8d88860

Add Python bindings

d2dc620

Added more Python examples

7f2fa61

Added plotting for nested dissection

0f69b4c

Working iSAM for Hybrid

a9c1d43

Merge remote-tracking branch 'origin/develop' into fan/prototype-hybr…

1fe7e74

…id-tr

Fix compile

2de3169

Merge remote-tracking branch 'origin/develop' into fan/prototype-hybr…

1bf9952

…id-tr

varunagrawal added 6 commits May 27, 2022 18:13

add doc strings to python unit test and add assertions

3bde044

add docs for HybridFactor

f443cf3

break up EliminateHybrid into smaller functions

6cd20fb

update the EliminateHybrid note to be more specific

c3a59cf

rename HybridFactorGraph to GaussianHybridFactorGraph

852a9b9

improved equality checks

841e6b0

dellaert reviewed Jun 1, 2022

View reviewed changes

varunagrawal added 8 commits June 1, 2022 23:05

Merge pull request #46 from varunagrawal/feature/eliminate-hybrid

1a8fa23

Break up EliminateHybrid into smaller functions

Rename files so that everything is HybridX

7c7b5dd

update description of GaussianMixtureConditional

31ab1a3

Add more information on conditionals requirement for GaussianMixture

d2029f3

rename GaussianMixtureConditional to GaussianMixture

c8bf9d3

Merge branch 'fan/prototype-hybrid-tr' into feature/GaussianHybridFac…

2cc0611

…torGraph

Merge pull request #47 from varunagrawal/feature/GaussianHybridFactor…

709bbb0

…Graph

Add GTSAM_EXPORT and Testable traits

932e65c

varunagrawal requested a review from dellaert June 2, 2022 04:40

fix python test

2afa678

dellaert approved these changes Jun 3, 2022

View reviewed changes

varunagrawal added 3 commits June 2, 2022 22:45

if checks for dynamic_cast

3e10514

separate out summing of frontals into separate function

dd87747

add comments

92176db

update GaussianMixture docstring

b47cd9d

varunagrawal reviewed Jun 3, 2022

View reviewed changes

rename back to HybridJunctionTree

eeecb27

varunagrawal merged commit 9a1eb02 into borglab:develop Jun 3, 2022

ProfFan mentioned this pull request Jul 14, 2022

Investigate the strange issues associated with implicit STL conversion #1244

Closed

3 tasks

varunagrawal deleted the fan/prototype-hybrid-tr branch October 2, 2022 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hybrid Factor Graph Implementation #1203

Hybrid Factor Graph Implementation #1203

ProfFan commented May 21, 2022

dellaert left a comment

dellaert Jun 1, 2022

dellaert Jun 1, 2022

dellaert Jun 1, 2022

dellaert Jun 1, 2022

dellaert Jun 1, 2022

dellaert Jun 1, 2022

dellaert Jun 1, 2022

dellaert left a comment

dellaert Jun 3, 2022

varunagrawal Jun 3, 2022

varunagrawal Jun 3, 2022

dellaert Jun 3, 2022

varunagrawal Jun 3, 2022

ProfFan Jun 3, 2022

ProfFan Jun 3, 2022

dellaert Jun 3, 2022

dellaert Jun 3, 2022

dellaert Jun 3, 2022

dellaert Jun 3, 2022

varunagrawal commented Jun 3, 2022 •

edited

Loading

ProfFan commented Jun 3, 2022

varunagrawal Jun 3, 2022

dellaert Jun 3, 2022

ProfFan Jun 5, 2022

varunagrawal Jun 5, 2022

dellaert Jun 5, 2022

varunagrawal Jun 7, 2022

varunagrawal Jun 7, 2022

dellaert commented Jun 3, 2022


		auto result = EliminateDiscrete(dfg, frontalKeys);

		return std::make_pair(


		gttoc(sum);

		using EliminationPair = GaussianFactorGraph::EliminationResult;

Hybrid Factor Graph Implementation #1203

Hybrid Factor Graph Implementation #1203

Conversation

ProfFan commented May 21, 2022

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varunagrawal commented Jun 3, 2022 • edited Loading

ProfFan commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert commented Jun 3, 2022

varunagrawal commented Jun 3, 2022 •

edited

Loading