-
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
Load and incorporate observations in pclean main #120
Changes from 6 commits
1484d87
ced702a
81791f6
d94d7c3
bb58176
027b484
a016538
c25bd27
f0c4c0c
503a176
ecd74e5
e0ac578
b6e508e
116e406
fcfa603
ab02dbc
fb10d29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
// Copyright 2024 | ||
// Apache License, Version 2.0, refer to LICENSE.txt | ||
|
||
#include "irm.hh" | ||
#include "pclean/csv.hh" | ||
#include "pclean/pclean_lib.hh" | ||
|
||
T_observations translate_observations( | ||
const DataFrame& df, const T_schema &schema) { | ||
T_observations obs; | ||
|
||
for (const auto& col : df.data) { | ||
const std::string& col_name = col.first; | ||
if (!schema.contains(col_name)) { | ||
printf("Schema does not contain %s, skipping ...\n", col_name.c_str()); | ||
continue; | ||
} | ||
|
||
const T_relation& trel = schema.at(col_name); | ||
size_t num_domains = std::visit([&](const auto &r) { | ||
return r.domains.size();}, trel); | ||
|
||
for (size_t i = 0; i < col.second.size(); ++i) { | ||
const std::string& val = col.second[i]; | ||
if (val.empty()) { | ||
// Don't incorporate missing values. | ||
// TODO(thomaswc): Allow the user to specify other values that mean | ||
// missing data. ("missing", "NA", "nan", etc.). | ||
continue; | ||
} | ||
|
||
std::vector<std::string> entities; | ||
for (size_t j = 0; j < num_domains; ++j) { | ||
// Give every row it's own universe of unique id's. | ||
// TODO(thomaswc): Correctly handle the case when a row makes | ||
// references to two or more different entities of the same type. | ||
entities.push_back(std::to_string(i)); | ||
} | ||
obs[col_name].push_back(std::make_tuple(entities, val)); | ||
} | ||
} | ||
return obs; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// Copyright 2024 | ||
// Apache License, Version 2.0, refer to LICENSE.txt | ||
|
||
#pragma once | ||
|
||
#include "irm.hh" | ||
#include "util_io.hh" | ||
#include "pclean/csv.hh" | ||
#include "pclean/pclean_lib.hh" | ||
|
||
// For each non-missing value in the DataFrame df, create an | ||
// observation in the returned T_observations. The column name of the value | ||
// is used as the relation name, and each entity in each domain is given | ||
// its own unique value. | ||
T_observations translate_observations( | ||
const DataFrame& df, const T_schema &schema); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
#define BOOST_TEST_MODULE test pclean_csv | ||
|
||
#include "pclean/pclean_lib.hh" | ||
#include <sstream> | ||
#include <boost/test/included/unit_test.hpp> | ||
namespace tt = boost::test_tools; | ||
|
||
BOOST_AUTO_TEST_CASE(test_translate_observations) { | ||
std::stringstream ss(R"""(Column1,Room Type,Monthly Rent,County,State | ||
0,studio,,Mahoning County,OH | ||
1,4br,2152.0,,NV | ||
2,1br,1267.0,Gwinnett County, | ||
)"""); | ||
|
||
DataFrame df = DataFrame::from_csv(ss); | ||
|
||
std::map<std::string, std::string> state_params = {{"strings", "AL AK AZ AR CA CO CT DE DC FL GA HI ID IL IN IA KS KY LA ME MD MA MI MN MS MO MT NE NV NH NJ NM NY NC ND OH OK OR PA RI SC SD TN TX UT VT VA WA WV WI WY"}}; | ||
std::map<std::string, std::string> br_params = {{"strings", "1br 2br 3br 4br studio"}}; | ||
|
||
T_schema schema = { | ||
{"County:name", | ||
T_clean_relation{{"County"}, false, DistributionSpec("bigram")}}, | ||
{"County:state", | ||
T_clean_relation{{"County"}, false, DistributionSpec("stringcat", state_params)}}, | ||
{"Room Type", | ||
T_clean_relation{{"Obs"}, false, DistributionSpec("stringcat", br_params)}}, | ||
{"Monthly Rent", | ||
T_clean_relation{{"Obs"}, false, DistributionSpec("normal")}}, | ||
{"County", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity, could you give the "County" and "State" relations names that disambiguate them from the domains? These names would be output from the schema converter as "Obs:county:name" and "Obs:state:name", right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've prepended a "d" to all of the domain names for clarity. The schema converter would give these relation names of "County" and "State", actually -- for noisy relations coming from an "observe x as y" line, the relation name is always taken as the "y", so that it can match the csv column name. |
||
T_noisy_relation{{"County", "Obs"}, false, EmissionSpec("bigram"), "County:name"}}, | ||
{"State", | ||
T_noisy_relation{{"County", "Obs"}, false, EmissionSpec("bigram"), "County:state"}}}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. translate_observations doesn't use that field of the schema at all, so I don't think it matters at all for the purposes of this test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it matters a little for self-documentation, so I'd prefer to set it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks (sorry for the back and forth, there's a lot going on in this model that's subtle but I think we're getting towards a shared understanding). It looks to me like the HIRM schema in this test is implementing the following PClean-like schema, is that right?
Is this what
I think this was the source of a lot of my earlier confusion. According to my reading of Model 5, the Obs/Record class should be treated like all of the other latent classes in terms of defining the clean relations. What's special about it is that there's a one-to-one correspondence between its entities and rows of the observed data. The columns of the observed data are all represented by noisy relations, and all of them have "Obs" as an input domain. If you look at the paragraph on page 10 of the Overleaf that begins "Given all this information", I think this is what it implies (in particular, we need an If you agree, could you make that change in a follow-up PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, thinking about this more, I think my read is wrong and you're right, that the observations that come directly from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's hard to say for sure, since the schema you give doesn't parse. I think you might mean `class County class Obs observe But anyway, the schema used in pclean_lib_test is not quite what make_hirm_schema would output. Sorry if that caused confusion, but I was optimizing for something simple that would exercise translate_observations. From the above schema, make_hirm_schema would output four clean_relations named "County:name", "County:state", "Obs:rent" and "Obs:room", and four noisy_relations named "County", "State", "Room Type" and "Monthly Rent". Other than the slight name differences, they are basically the same as you have in your expected output. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for writing a non-parsable schema, I just wanted to verify the class definitions, and you're right, I was assuming there was an observation for each attribute. Thanks for clarifying what |
||
|
||
T_observations obs = translate_observations(df, schema); | ||
|
||
// Relations not corresponding to columns should be un-observed. | ||
BOOST_TEST(!obs.contains("County:name")); | ||
BOOST_TEST(!obs.contains("County:state")); | ||
|
||
BOOST_TEST(obs["Room Type"].size() == 3); | ||
BOOST_TEST(obs["Monthly Rent"].size() == 2); | ||
BOOST_TEST(obs["County"].size() == 2); | ||
BOOST_TEST(obs["State"].size() == 2); | ||
|
||
BOOST_TEST(std::get<0>(obs["Room Type"][0]).size() == 1); | ||
BOOST_TEST(std::get<1>(obs["Room Type"][0]) == "studio"); | ||
|
||
BOOST_TEST(std::get<0>(obs["Monthly Rent"][0]).size() == 1); | ||
BOOST_TEST(std::get<1>(obs["Monthly Rent"][0]) == "2152.0"); | ||
|
||
BOOST_TEST(std::get<0>(obs["County"][0]).size() == 2); | ||
BOOST_TEST(std::get<1>(obs["County"][0]) == "Mahoning County"); | ||
|
||
BOOST_TEST(std::get<0>(obs["State"][0]).size() == 2); | ||
BOOST_TEST(std::get<1>(obs["State"][0]) == "OH"); | ||
} | ||
|
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.
Now that I think about this more, I don't think we should initialize unobserved "observations" here. Either we should sample them from the generative model once we have an HIRM instance (which is what we'll do for Model 7 anyway), or for the purposes of Model 5/6 we should read them in from a file (depending on whether we decide that's necessary with the MIT folks -- sorry I haven't had a chance to post on Slack yet).
For the purposes of this PR, I think we can just omit unobserved values from the observations (i.e. assume only relations with
is_observed == true
have observations).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.
You are using the word "unobserved" in a different way than I would use it.
For me, the schema creates the relations, and then the data comes in, and after all the data is in, we can mark a relation as observed or unobserved depending on whether it had any observations.
If I had to guess, you are maybe using "unobserved" to mean something "a relation for which we will need to initialize and maintain a latent state"?
But just to be clear, every observation here is for a noisy relation generated in the second part of PCleanSchemaHelper::make_hirm_schema -- those are the only ones that correspond to CSV column names. None of those noisy relations will need to have a hidden latent state. Even if somehow all of their data turned out to be missing from the CSV file, there are no other relations that depend on them.
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.
Ok, I think I see. The observed relations should be exactly those defined by the
observe
statement, so we can declare a relation to be "observed" if it appears in that statement, right? I wouldn't expect there to be data for relations not defined byobserve
, and if a relation defined byobserve
doesn't have any observations in the data, I think that's a user error. Do you agree?Assuming the observed relations are those defined by the
observe
statement (and those which have observed data) I think these are equivalent -- everything that isn't observed (which should be the base relations of noisy relations), we have to infer.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, again, I think the best/safest/most intuitive thing to do is to call a relation observed iff we see data for it. So in line with that, I would mark relations as is_observed inside the incorporate_observations function.
But since we don't do that currently (is_observed is marked in load_observations), I've done the next best thing and declared the relations created from the "observe" clause in the schema as is_observed in this pull request.
Not quite. If I have a schema and a csv file, and then I drop a column from the csv file, I wouldn't call it user error to run pclean on the combination, even though there would be a
observe
relation without any observations. I think it would be quite easy to support that use case, so we probably should (while perhaps agreeing that supporting it isn't the most important priority for a research codebase).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 also add that this conversation now has almost nothing to do with this pull request, and should probably be moved to a different channel (like a meeting or github issue).
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 TODO to sample the non-index domains from a CRP prior instead of assuming each entry is a unique entity? (We might want to rethink this more substantially for Model 7, i.e. whether it makes sense to initialize the entities as part of the data ingestion process or elsewhere. Other inferred values, e.g. latent values of a noisy relation, are initialized during
incorporate
, which might make more sense).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.
Added a TODO to discuss and consider other options.
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.
For me, this conversation was very relevant, and essential in allowing me to verify that this PR does the right thing, so I appreciate you taking the time to clarify.
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 was going to file a Github issue, but I think we should leave this as-is. Downstream in HIRM
!is_observed
implies the value is latent and needs to be inferred, so settingis_observed
according to the "observe" clause gives the right semantics.That's fine, I was thinking it made sense to support a more limited notion of valid inputs before potentially supporting something more general, but I agree it doesn't matter much here.