Skip to content
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 household_id, factor out col names #35

Merged
merged 2 commits into from
Aug 30, 2017
Merged

Add household_id, factor out col names #35

merged 2 commits into from
Aug 30, 2017

Conversation

katbusch
Copy link
Contributor

@katbusch katbusch commented Aug 29, 2017

This fixes #27, not in the requested way but in a cheap way that we can change later if we want. I also moved a few strings into the inputs file as constants to make the code easier to write/maintain


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 82.418% when pulling a752104 on unique_id into 94b5986 on master.

@katbusch
Copy link
Contributor Author

@nikisix
Copy link
Contributor

nikisix commented Aug 29, 2017

The actual code looks and runs well. However, in order for this to be a unique_id, we should use the full tract id, including its state and county components.
I.e.

STATEFP  COUNTYFP  TRACTCE  PUMA5CE
01       001       020100   02100
01       001       020200   02100
01       001       020300   02100
01       001       020400   02100
01       001       020500   02100
01       001       020600   02100
01       001       020700   02100
01       001       020801   02100
01       001       020802   02100

Ideally we'd have tract_ids that look more like: "01001020802" than "20802", which is what I'm ending up with right now after generating synth households.


Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nikisix
Copy link
Contributor

nikisix commented Aug 29, 2017

I also realize, this may very well be outside of the purview of this push


Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@katbusch
Copy link
Contributor Author

Ah I got it. Okay, I agree that is outside the purview of this. We should add state & county throughout our code and then we can tie it into the unique ID. I will file a separate issue for that


Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@katbusch
Copy link
Contributor Author

Made #36

@nikisix
Copy link
Contributor

nikisix commented Aug 30, 2017

:lgtm:


Comments from Reviewable

@katbusch katbusch merged commit 0d1d6d1 into master Aug 30, 2017
@katbusch katbusch deleted the unique_id branch August 30, 2017 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Household indexing is confusing
3 participants