Add sample_new parameter to gendb::incorporate #215
Conversation
| @@ -53,7 +54,8 @@ void GenDB::incorporate( | |||
| schema.query.fields.at(query_rel).class_path; | |||
| T_items items = | |||
| sample_entities_relation(prng, schema.query.record_class, | |||
| class_path.cbegin(), class_path.cend(), id); | |||
| class_path.cbegin(), class_path.cend(), id, | |||
There was a problem hiding this comment.
A lot of the machinery in sample_class_ancestors etc is for the purpose of walking the DAG of references to make sure the reference values are coherent (i.e. if Physician 5 went to School 3, that's correct in all samples). Am I correct that when sample_new is false, we never see the same Physician e.g. more than once, so we never actually have to walk the DAG? If so, then I think we should implement this by simply iterating over a relation's domains and pulling sequential values from the domain CRPs rather than adding complexity to sample_class_ancestors and the other methods that are primarily for inference.
There was a problem hiding this comment.
Added TODO. I don't think the sample_new = false case is as simple as you make out, if only because it's what pclean_lib::translate_observations did before and that was far from trivial (and it relied on the annotated_domains_for_relations datastructure, which is being eliminated).
There was a problem hiding this comment.
Looking again at translate_observations, it looks like it used annotated_domains_for_relations to get unique string names for all of the entities, which we don't need to do here since we're working with entity IDs directly, so I think it is pretty simple (and I'd prefer to do it in this PR though a TODO is ok too).
cxx/gendb.cc
Outdated
| @@ -40,7 +40,8 @@ double GenDB::logp_score() const { | |||
|
|
|||
| void GenDB::incorporate( | |||
| std::mt19937* prng, | |||
| const std::pair<int, std::map<std::string, ObservationVariant>>& row) { | |||
| const std::pair<int, std::map<std::string, ObservationVariant>>& row, | |||
| bool sample_new) { | |||
There was a problem hiding this comment.
The name sample_new is confusing to me, since we're guaranteed to get "new" unseen values when it's false, and we might get previously-seen values when it's true. Could you use maybe use_sequential_entities or use_unique_entities (with the semantics flipped), or another name that's more explanatory?
There was a problem hiding this comment.
How do you like new_entities_have_new_parts? (It has reversed semantics.)
There was a problem hiding this comment.
I don't understand what "parts" means or what a "new entity" is (is it an entity corresponding to a newly observed row or is it an entity that hasn't been seen before)? I think use_unique_entities or initialize_with_unique_entities does the job (or use your proposal, with a clarifying comment)
There was a problem hiding this comment.
Just saw the comment re. "parts" -- I think we should emphasize somewhere that new entities are also used for new rows (and it's still unclear to me what it means for an entity to have a part, which I understand as an attribute, so I'd lean towards one of the other names I proposed with "unique").
There was a problem hiding this comment.
How about new_rows_have_unique_entities?
There was a problem hiding this comment.
SGTM (though please add a comment that two entities that are added to the same domain in the course of adding a single row are also unique -- this is why I prefer a more general name like use_unique_entities).
cxx/gendb_test.cc
Outdated
| BOOST_AUTO_TEST_CASE(test_unincorporate_reference_new_entities_have_new_parts) { | ||
| std::mt19937 prng; | ||
| GenDB gendb(&prng, schema); | ||
| setup_gendb(&prng, gendb, true); |
There was a problem hiding this comment.
I don't think we'll use this with unincorporate_reference, it should just be for initialization of the state. Could you add a test sanity-checking that the state of the gendb object is as expected after incorporating with the flag=true? (e.g. all domain CRP tables should be of size 1, all domain CRPs should have the same number of items incorporated (assuming each domain only appears once in the schema, which it does here)).
There was a problem hiding this comment.
Added the test you requested, but I believe that all the domain CRP tables should be of size 32, and not 1 (since they are all incorporated into the CRPs with new_id's.)
There was a problem hiding this comment.
You assert that tables.size() == 32, which means that there are 32 tables, each of size 1 (for completeness you could test that each of the tables is indeed size 1, though testing that there are 32 tables and N=32, as you do, is adequate IMO).
There was a problem hiding this comment.
Added tests that the first tables are of size 1.
|
If you want, I can push another commit to this branch that avoids walking the DAG and plumbing the bool arg through. If it's ok with you I'd like to merge #217 before this (and #212) -- I'm happy to help with merge conflicts, just I'm scared of breaking the Gibbs sampler and that seems like an easier order to do it in. |
My preference would be for you to replace the bool arg plumbing in a separate pull request. #217 seems like it is still rather far from being checked-in. My preference would be to check this and #212 in first. |
|
The comments on #217 were all pretty minor and are now addressed, so I think it should be close. I'll resolve the merge conflicts by cloning this branch and rebasing the commits onto #217. Then if you want you can update your PR by overwriting your branch with the cloned branch (I'll send you the commands). That ordering will be easier for me than merging this and trying to rebase 217 on top of it. |
|
Actually the merge conflicts weren't as bad as I thought so we can check in either first. |
This is required for GenDB to replicate existing pclean behavior (via sample_new=False) and unblock #212.