Refactored hybrid inference #1863

dellaert · 2024-10-08T09:22:42Z

This is a big PR that gets rind of "hiding" the conditional constants into the Jacobian factors in HGF, in favor of storing a scalar alongside each factor in the HGFG, as is done in HNFG. It also adds extensive testing.

Details

GaussianFactorGraphTree is now its own class, called HybridGaussianProductFactor
asGaussianFactorGraphTree is renamed to collectProductFactor, and now only exists in HGF, no longer in HGC
scalars are initialized with each conditionals adjusted negLogConstants, i.e. scalars are zero if they are the same for each mode.
augment is GONE
added methods discretePosterior which yield much easier "ratio tests".

Testing

added many more tests in testHGC, testHBN, and testHGFG
moved all multi-frontal elimination tests to testHybridBayesTree

TODO:

replace all ratio tests
get rid of HGC::conditionals_ if improves performance

Revised asProductFactor methods collectProductFactor() method Move test better print Formatting Efficiency Fix several bugs Fix print methods Fix print methods More tests, BT tests in different file More product tests Disable printing tests Minimize diff Fix rebase issue

varunagrawal · 2024-10-08T14:24:19Z

Would it be possible to break this into a separate PR for the HybridGaussianProductFactor?

gtsam/hybrid/HybridGaussianConditional.cpp

gtsam/hybrid/HybridGaussianFactor.cpp

gtsam/hybrid/tests/testHybridNonlinearFactorGraph.cpp

gtsam/hybrid/HybridGaussianFactorGraph.cpp

varunagrawal · 2024-10-08T15:11:34Z

gtsam/hybrid/HybridGaussianFactorGraph.cpp

+        auto [factor, scalar] = pair;
+        // If factor is null, it has been pruned, hence return potential zero
+        if (!factor) return 0.0;
+        return exp(-scalar - factor->error(kEmpty));


This is a major bug!!!
when we don't normalize the values, if we have large factor errors (e.g. even 80), then exp(-80) ~= 0.0 and everything collapses. This was one of the major issues I fixed last December that got things working.

Unfortunately this is not revealed in the the GTSAM tests because the sigmas are too simple but show up in the hybrid estimation code (same reason why the pruning PR didn't show any issues in GTSAM but caused experiments to fail).

Confirming that I am running into a similar issue as before.

Ok, I'll fix this

I will fix it only here, when the exp is taken. I'd like the original errors preserved in the factors, not shifted before it's needed. I can explain in person if you vehemently object.

OK, for this case and case below, I added a function

/// Take negative log-values, shift them so that the minimum value is 0, and /// then exponentiate to create a DecisionTreeFactor (not normalized yet!). static DecisionTreeFactor::shared_ptr DiscreteFactorFromErrors( const DiscreteKeys &discreteKeys, const AlgebraicDecisionTree<Key> &errors) { double min_log = errors.min(); AlgebraicDecisionTree<Key> potentials = DecisionTree<Key, double>( errors, [&min_log](const double x) { return exp(-(x - min_log)); }); return std::make_shared<DecisionTreeFactor>(discreteKeys, potentials); }

Please note I realized it is not necessary to normalize to probabilities: it is still just a factor that we need, not a conditional. That was an error on my part before - although it only cost some computation, nothing else.

Also I would like to hear the explanation for my own curiosity. I have a suspicion we may need to normalize somewhere.

Actually, it might still need to happen in the discretePosterior methods. It seems what we need is a constructor in DecisionTreeFactor. factors are not normalized, so for discretePosterior it should be a constructor in DiscreteConditional (which is normalized).

I can do it and piggyback it off of #1867

Apparently we can't create a DiscreteConditional since we don't have the cardinality information of the Keys from an AlgebraicDecisionTree. Thoughts?

Also discretePosterior already normalizes due to the p / p.sum().

gtsam/hybrid/HybridGaussianFactorGraph.cpp

varunagrawal · 2024-10-08T15:16:41Z

Here are my clang-format settings:

"clang-format.executable": "/usr/local/bin/clang-format",
"clang-format.language.c.style": "Google",
"clang-format.language.cpp.style": "Google",
"clang-format.fallbackStyle": "Google",

varunagrawal · 2024-10-08T16:40:48Z

~~I re-added the normalization and tests that initially passed are now failing. :(~~
Nvm I was missing the scalar value.

Updates to `No Hiding` PR

dellaert · 2024-10-09T10:22:01Z

Would it be possible to break this into a separate PR for the HybridGaussianProductFactor?

It's possible to split the PR - but not with this version of HybridGaussianProductFactor

dellaert · 2024-10-09T10:27:33Z

Here are my clang-format settings:

"clang-format.executable": "/usr/local/bin/clang-format",
"clang-format.language.c.style": "Google",
"clang-format.language.cpp.style": "Google",
"clang-format.fallbackStyle": "Google",

This file is checked in in:

BasedOnStyle: Google

BinPackArguments: false
BinPackParameters: false
ColumnLimit: 100
DerivePointerAlignment: false
IncludeBlocks: Preserve
PointerAlignment: Left

Maybe we can just change that file to not have any modifications on top of Google style.

dellaert · 2024-10-09T11:04:28Z

Pushed new changes. I have no idea on how to fix windows. Because istream is involved it could be serialization?

This reverts commit 55dc3f5.

varunagrawal · 2024-10-09T19:17:44Z

Pushed new changes. I have no idea on how to fix windows. Because istream is involved it could be serialization?

I figured out a hack to fix the Windows CI. It is in #1866.

More Hybrid Fixes

dellaert · 2024-10-10T07:35:18Z

@varunagrawal Can we now merge this? Do your experiments work?

varunagrawal · 2024-10-10T13:31:27Z

Yes, but I wanted to fix all the pending TODOs. I'll do them in the other PR.

varunagrawal

LGTM

dellaert and others added 19 commits October 1, 2024 16:43

Store the values

3797996

implement errorTree for HybridNonlinearFactorGraph

14d1594

discretePosterior in HNFG

1bb5b95

New product factor class

8b3dfd8

Switch to using HybridGaussianProductFactor

ed9a216

Fix conditional==null bug

55ca557

Product now has scalars

584a71f

operator returns pairs, extensive switching test

e1c0d0e

Cherry-pick Varun's bugfix

04cfb06

Don't normalize probabilities for a mere DiscreteFactor

b3c6980

Extensive new API test

586b177

No more hiding!

518ea81

Less regression, more API tests for s(3).

f0770c2

Store negLogConstant[i] - negLogConstant_

bcd94e3

asProductFactor from base class!

0f48efb

Fixed discreteEimination

5241614

Merge branch 'develop' into feature/no_hiding

88b8dc9

Minimize formatting changes

9f7ccbb

dellaert requested a review from varunagrawal October 8, 2024 09:22

varunagrawal reviewed Oct 8, 2024

View reviewed changes

varunagrawal added 5 commits October 8, 2024 11:48

formatting fixes

8e85b68

add type info

f39f678

missing include and formatting

7603cd4

update comment

874ba67

improve HybridGaussianProductFactor

21b4c4c

formatting testHybridGaussianFactorGraph

8b8466e

varunagrawal and others added 6 commits October 8, 2024 23:20

include sstream in HybridGaussianProductFactor

cd3e0f3

add back PotentiallyPrunedComponentError as an inline function

4ae5596

formatting

26c9dcb

replace sstream with string

4df266a

use cout instead of stringstream

436524a

Merge pull request #1865 from borglab/feature/no_hiding-2

59f97d6

Updates to `No Hiding` PR

dellaert added 2 commits October 9, 2024 20:03

Pure Google style in clang-format

19fdb43

Shift error values before exponentiating

34bb1d0

varunagrawal added 12 commits October 9, 2024 10:51

fix printing tests

caddc73

add serialization to HybridGaussianProductFactor

205eb18

checking if print is the problem

4f74735

undo print and remove extra includes

8a650b6

comment out serialization

e87d1fb

undo test change

99a39e6

delete constructor from string to fix issue

752e10f

fix vector type

1ac8a6e

implement constructor

f4bf280

experiment with removing the constructor

55dc3f5

Revert "experiment with removing the constructor"

4d707e7

This reverts commit 55dc3f5.

implement dummy >> operator

93ec276

Merge pull request #1866 from borglab/more-fixes

95f053f

More Hybrid Fixes

varunagrawal approved these changes Oct 10, 2024

View reviewed changes

varunagrawal merged commit def8b25 into develop Oct 10, 2024
33 checks passed

varunagrawal deleted the feature/no_hiding branch October 10, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored hybrid inference #1863

Refactored hybrid inference #1863

dellaert commented Oct 8, 2024

varunagrawal commented Oct 8, 2024

varunagrawal Oct 8, 2024

varunagrawal Oct 8, 2024

dellaert Oct 9, 2024

dellaert Oct 9, 2024

dellaert Oct 9, 2024

varunagrawal Oct 9, 2024

dellaert Oct 9, 2024

varunagrawal Oct 9, 2024

varunagrawal Oct 9, 2024

varunagrawal Oct 9, 2024

varunagrawal commented Oct 8, 2024

varunagrawal commented Oct 8, 2024 •

edited

Loading

dellaert commented Oct 9, 2024

dellaert commented Oct 9, 2024

dellaert commented Oct 9, 2024

varunagrawal commented Oct 9, 2024

dellaert commented Oct 10, 2024

varunagrawal commented Oct 10, 2024

varunagrawal left a comment

Refactored hybrid inference #1863

Refactored hybrid inference #1863

Conversation

dellaert commented Oct 8, 2024

Details

Testing

TODO:

varunagrawal commented Oct 8, 2024

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 Oct 8, 2024

varunagrawal commented Oct 8, 2024 • edited Loading

dellaert commented Oct 9, 2024

dellaert commented Oct 9, 2024

dellaert commented Oct 9, 2024

varunagrawal commented Oct 9, 2024

dellaert commented Oct 10, 2024

varunagrawal commented Oct 10, 2024

varunagrawal left a comment

Choose a reason for hiding this comment

varunagrawal commented Oct 8, 2024 •

edited

Loading