-
Notifications
You must be signed in to change notification settings - Fork 769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to EliminateDiscrete for max-product #1362
Conversation
dellaert
commented
Jan 1, 2023
- added tiny example in c++
- made SumFrontals a method
- added test for SumFrontals
- added test for elimination in python, and it works (now we need to move to the next simplest example):
- below shows estimate of marginals, which match the eliminated result! And rations of posterior resulting from elimination also match! Awesome job :-)
Not sure why it works yet, we should talk. |
@varunagrawal So, I thought the code would now be correct, but it's not working out. The ratios also stopped working. Maybe we can investigate together. In a nutshell, in the C++ code, because we only use measurement, the discretefactor should in the final posterior be exactly equal to the prior. That means that the factor computed in the first elimination step should be zero information: DiscreteFactor with two equal leaves. But the constants do not add up to the same value (which is what is required). |
I guess the issue with this was the assumption that one measurement wasn't informative enough to distinguish the modes? If yes, then glad we figured it out today. Making the note here for posterity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm making a PR to address some of these things and fix the tests.
@@ -172,6 +169,16 @@ class GTSAM_EXPORT GaussianMixture | |||
*/ | |||
double error(const HybridValues &values) const override; | |||
|
|||
// /// Calculate probability density for given values `x`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we keeping these for the future? My intuition says yes, but this would probably move to a base class.
}; | ||
DecisionTree<Key, double> fdt(separatorFactors, factorProb); | ||
// Normalize the values of decision tree to be valid probabilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this. Or will normalization be handled later?
@dellaert I am merging this. |