Add ZeroMeanNormal distribution and change GaussianEmission to use it. #44
Conversation
| std::mt19937 prng; | ||
| ZeroMeanNormal nd(&prng); | ||
|
|
||
| BOOST_TEST(nd.logp(6.0) == -5.4563792395895785, tt::tolerance(1e-6)); |
There was a problem hiding this comment.
I'd replace these tests with marginalization tests (see normal_test.cc to test against a boost computed logp and logp_score via approximate integration).
I find the testing exact value tests brittle and have the possibility of hiding bugs, and strongly prefer marginalization tests.
There was a problem hiding this comment.
I agree that marginalization tests are great. They are also a lot more work. My preference would be to have these minimal tests now, and then add the marginalization tests in a later pull request.
There was a problem hiding this comment.
The recommendation here is to copy the tests from normal_test.cc. I've added marginalization tests for logp and logp_score. Those should work the exact same here.
There was a problem hiding this comment.
There was a problem hiding this comment.
So I tried your recommendation and it didn't work. The latest commit of this pull request has the test_log_prob and it fails.
This shouldn't be surprising, because as previously noted, this distribution has different marginal likelihood and predictive posterior functions than the Normal. For example, when we do add a marginalization test for this distribution, it will only need to do a single integral over variances rather than the double integral done in normal_test.cc.
| public: | ||
| // We use an Inverse gamma conjugate prior, so our hyperparameters are | ||
| double alpha = 1.0; | ||
| double beta = 1.0; |
There was a problem hiding this comment.
Could these be renamed the same was as normal? Admittedly I don't love the single letter naming scheme or alpha and beta since I find these opaque, but would prefer the naming for both to be the same.
There was a problem hiding this comment.
They could, but I think the most important thing is that they match the math. Since https://www.cs.ubc.ca/~murphyk/Papers/bayesGauss.pdf and https://j-zin.github.io/files/Conjugate_Bayesian_analysis_of_common_distributions.pdf both use alpha and beta, I would strongly prefer that we keep those as the variable names.
No description provided.