-
Notifications
You must be signed in to change notification settings - Fork 779
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
Likelihoods and other improvements #1356
Conversation
dellaert
commented
Dec 29, 2022
- added likelihood to GaussianMixture
- made FG out of BN
- checked ratio
- fixes equals and print in GMF
Please approve and merge @varunagrawal , I'll fix comments in follow up PR. |
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.
LGTM with comments.
/* ************************************************************************ */ | ||
GaussianConditional GaussianConditional::FromMeanAndStddev( | ||
Key key, const Matrix& A, Key parent, const Vector& b, double sigma) { | ||
// |Rx + Sy - d| = |x-(Ay + b)|/sigma | ||
const Matrix R = Matrix::Identity(b.size(), b.size()); | ||
const Matrix S = -A; | ||
const Vector d = b; | ||
const Vector& d = b; |
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.
What is the reason for making this a reference?
"""Test a tiny two variable hybrid model.""" | ||
bayesNet = self.tiny() | ||
sample = bayesNet.sample() | ||
# print(sample) |
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.
Kill please?
|
||
self.assertEqual(fg.size(), 3) | ||
|
||
def test_tiny2(self): |
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.
Probably rename this to describe what the test is doing?
bayesNet = gtsam.HybridBayesNet() | ||
|
||
# Create mode key: 0 is low-noise, 1 is high-noise. | ||
modeKey = M(0) |
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.
Can be merged into the next line since modeKey isn't used anywhere else.
|
||
hv = hbn.optimize() | ||
self.assertEqual(hv.atDiscrete(C(0)), 1) | ||
|
||
@staticmethod | ||
def tiny(num_measurements: int = 1): | ||
"""Create a tiny two variable hybrid model.""" |
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 would like to see this say "Create a tiny two variable hybrid model which represents the generative probability P(z, x, n) = P(z | x, n)P(x)P(n)."
sample = bayesNet.sample() | ||
# print(sample) | ||
|
||
# Create a factor graph from the Bayes net with sampled measurements. |
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.
This comment should say
# Create a factor graph from the Bayes net with sampled measurements. The factor graph is `P(x)P(n) ϕ(x, n; z1) ϕ(x, n; z2)`
Please copy directly since I have added the relevant unicode.