-
Notifications
You must be signed in to change notification settings - Fork 2
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
Big factors plus irm_test plus integration test of transition_hyperparameters #65
Conversation
ThomasColthurst
commented
Jun 18, 2024
- :hirm is broken up into :irm, :hirm_lib and :hirm.
- :headers is removed
- :relation_variant is split off from :util_distribution_variant in order to avoid cyclic dependencies.
- A pretty small irm_test is added.
- test_irm_two_relations.cc is upgraded to use single_step_irm_inference, and because of that must set the distribution hyperparameters from the IRM that is loaded from disk.
- Fixed an unused variable warning in relation_test.cc
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.
Looks great, thank you!!
@@ -1,8 +1,9 @@ | |||
// Copyright 2024 | |||
// See LICENSE.txt | |||
|
|||
// This file collects classes/functions that depend on the set of distribution | |||
// subclasses and should be updated when a new subclass is added. | |||
// Classes and functions for dealing with Distributions and their values in a |
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.
Could you update the comment on lines 7-8 of the Distribution base class too?
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.
Done.
#include <boost/test/included/unit_test.hpp> | ||
namespace tt = boost::test_tools; | ||
|
||
BOOST_AUTO_TEST_CASE(test_irm) { |
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.
Could you add a test for logp? Maybe something like this (assuming I understand correctly):
double logp_score0 = irm.logp_score();
double logp1 = irm.logp(x);
irm.incorporate(x);
double logp_score1 = irm.logp_score();
BOOST_TEST(logp_score1 == logp_score0 + logp1);
(Or fine to do in a follow-up, if you or Srinivas are planning on expanding test coverage/adding HIRM tests later.)
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.
Well, I added that test, which I also expected to pass, but it didn't. Added a TODO for now.