Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Load and incorporate observations in pclean main #120
Changes from 3 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
There are no files selected for viewing
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.
We generally haven't been using const on local variables I don't have a strong opinion either way, but I think we should be consistent in using it everywhere/nowhere/some places according to some rules we agree on (and up to now we've defaulted to "nowhere"). WDYT?
The Google style guide says "Using const on local variables is neither encouraged nor discouraged."
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 don't care much about this, but I do see some other uses of this in the code base, like on lines 267, 306 and 423 of util_io.cc.
And we use const all the time on for-loop variables, and those are local variables too!
I guess my overall opinion is that this might be an area where some inconsistency is fine.
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.
Inconsistency is fine by me too, I guess I'd just prefer that it feel less arbitrary. Are there any loose guidelines you'd propose?
Edit: I meant "random" not "arbitrary," some arbitrariness is probably inevitable.
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 guess my suggestion would be to lean towards using const when you can and when it would help.
So for example, if a local variable is only alive for a few lines and it is clear what is being done to it, the const doesn't help very much. But for a medium sized or longer function, annotating something as const can give the reader some valuable info about what is going on.
I guess the other guideline I would suggest is that consistency at the function level is more important than any sort of global consistency. That is: if my function has three variables and I mark two of them as const, then there is a slight presumption that the third non-const variable gets mutated somewhere. So don't do that unless it is.
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 don't understand this block of code -- it looks like we're assuming that each entity in each domain has the same (sequential) value for each observation, so the table looks like:
is that right? For Model 5, don't we want to read these in from the observations?
Also, related to the comment, I think we do want to assume that each row of the observation dataframe is its own entity (and is indexed by a primary key domain, like #138 describes), but we don't necessarily want to assume that all ancestor entities are distinct from those of any other entity (adding the index domain should let us avoid that).
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.
It is true that in Model 5 we need to assume which entities exist and what their relationships are. I guess we could create a file with that information, but given that we never know that information "in real life", I think it is better to generate it programatically. When we get to model 7, we will need to generate the entity assignments programatically anyway (and then allow them to make transitions).
Just to make sure I'm being clear, let me give a concrete example: the assets/rents_dirty.csv input file, which looks like
Column1,Room Type,Monthly Rent,County,State
0,studio,486.0,Mahoning County,OH
1,4br,2152.0,Clark County,NV
2,1br,1267.0,Gwinnett County,GA
...
There are two classes in the model: Obs and County. I agree with you that it is reasonable to assume that each row corresponds to its own Obs entity. (For now! For Model 7, we will want the ability to sometimes say that the model thinks that two rows are duplicates of the same underlying entity; that's one of the cleaning operations we want a PClean-type program to be able to do.) But we aren't given the County entity assignment for each row, and it's not trivial to guess it either, as sometimes either or both of the county name and state fields are missing. Given that, I think that it is reasonable to initialize the County entity assignment to be unique for each row (which is what my code currently does).
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 think for
C*
/the "Record" class, we always want to assume that each row is a separate entity and is in one-to-one correspondence with rows of the observed table. A clearer example might be the schema you've defined in other tests, which has asC*
The entities of
Record
are in one-to-one correspondence with the observations, but the physicians and practices they refer to are duplicated.For testing Model 5, I think (though I'm not sure) that we want to assume we know the entities and their relationships "in real life." This seems important to be able to test that entity clustering is as we expect (we could also hand-build HIRM-like schemas/observations that replicate PClean-style databases, but it would be nice if we could just define PClean-like ones directly). We should probably clarify this with the MIT folks -- I'll post a slack message.
For sampling, eventually we'll want to sample from a County CRP instead of assuming they're unique, using something like the HIRM sampling method introduced in the
080124-emilyaf-sample-hirm
branch (WIP).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've posted about it on slack.
My preference would be to check this code in as-is (or possibly with a TODO or issue to change it depending on the result of the slack conversation). What do you think?