-
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
Add a test for a more complex schema + bug fixes. #229
Conversation
// Samples and incorporates a value into all relations belonging to class_name | ||
// (including class attributes and noisy observations of ancestor class | ||
// attributes). Recursively calls itself on base relations. | ||
void GenDB::sample_and_incorporate_for_class(std::mt19937* prng, |
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.
Just curious -- will this work when class_name is the name of the observation class? Because if so, we could use it to slightly simplify the implementation of make_pclean_sample in pclean_lib.
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.
Good point, this should work for the observation class too.
cxx/gendb.cc
Outdated
// equals class_item. | ||
// equals class_item. For singleton references, recursively unincorporates | ||
// from the reference class as well. | ||
// TODO there is a known issue where an entity with a value that is perfectly |
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 you create a GitHub issue for this with more details? In particular, I'm not sure what perfect correlation means here.
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 rethought this and I don't think it's actually an issue (if the entities don't share a DAG path, the "correlated" one won't change after the Gibbs step, even if it technically should have been unincorporated in intermediate states -- by "correlated" I just mean that Reference A takes value X iff Reference B takes value Y. I deleted the comment.
cxx/gendb.cc
Outdated
@@ -304,7 +356,7 @@ double GenDB::unincorporate_reference_relation( | |||
std::unordered_map<T_items, ObservationVariant, H_items>>& | |||
stored_value_map) { | |||
// We can stop unincorporating from base relations if the index of | |||
// the domain of interest is greater than the number of domains. | |||
// the domain of interest is greater than the max index. |
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.
max index of what?
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 meant the max index of the vector of items, so the length minus one. I rephrased the comment.
567151d
to
17af275
Compare
Also renames domain_crps -> entity_crps to disambiguate from domain.crp in the IRMs.