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

Adding the poverty dataset #75

Merged
merged 15 commits into from
Nov 16, 2023
Merged

Adding the poverty dataset #75

merged 15 commits into from
Nov 16, 2023

Conversation

Niklewa
Copy link
Collaborator

@Niklewa Niklewa commented Nov 7, 2023

The dataset includes the following variables: GeoFIPS, total poverty, poverty under 18, median household income, year

@Niklewa Niklewa requested a review from emackev November 7, 2023 13:59
@Niklewa Niklewa requested a review from emackev November 10, 2023 13:00
@emackev
Copy link
Contributor

emackev commented Nov 13, 2023

This is great!! But not exactly in the format of other variables (e.g. GeoName is missing) -- can you please update it?

@Niklewa Niklewa changed the base branch from data_v2 to main November 14, 2023 12:03
@emackev
Copy link
Contributor

emackev commented Nov 14, 2023

@Niklewa, heads up some of the tests fail -- can you tag me in a comment when it's ready for me to review and merge? Thanks!

@Niklewa
Copy link
Collaborator Author

Niklewa commented Nov 15, 2023

@emackev All good now, minor linting issue

@rfl-urbaniak
Copy link
Contributor

@emackev everything looks good on my side. Unless there are further issues, can you make a new accepting review so that this can be merged?

@rfl-urbaniak rfl-urbaniak self-requested a review November 15, 2023 16:40
cities/utils/cleaning_poverty.py Outdated Show resolved Hide resolved
data/raw/poverty.csv Outdated Show resolved Hide resolved
docs/guides/data_sources.ipynb Outdated Show resolved Hide resolved
cities/utils/cleaning_pipeline.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@Niklewa Niklewa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have addressed all the issues. The clean_variable.py required modifications as exclusions are now stored in a .csv file instead of a .pkl file. However, for some reason, it is not functioning correctly now. I will address that later in the day.

@Niklewa
Copy link
Collaborator Author

Niklewa commented Nov 16, 2023

Now, everything seems to be fine

@Niklewa Niklewa requested a review from emackev November 16, 2023 14:33
Copy link
Contributor

@emackev emackev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I'll merge.

@emackev emackev merged commit f52793d into main Nov 16, 2023
1 check passed
@emackev emackev deleted the nl-add-poverty branch November 16, 2023 14:44
@emackev emackev restored the nl-add-poverty branch November 16, 2023 15:01
@emackev
Copy link
Contributor

emackev commented Nov 16, 2023

@Niklewa , I realize that when I ran the cleaning_pipeline, git says that the variables are new. Did you perhaps change something in the data cleaning pipeline after you last ran cleaning_pipeline?

image

@Niklewa
Copy link
Collaborator Author

Niklewa commented Nov 16, 2023

@emackev, I have investigated that, and I do not think I changed anything. The only difference between those files is the last digit of some standardized values (incredibly small difference). Perhaps it has something to do with randomness.

@emackev
Copy link
Contributor

emackev commented Nov 16, 2023

hmm, likely a numerical precision thing. Thanks for checking! It's a bit strange that it only shows up for the recent variables, not the older ones, after running clean_variables.py. Shall we push the updated variables just in case?

@Niklewa
Copy link
Collaborator Author

Niklewa commented Nov 16, 2023

It is strange. I think it's worth pushing them.

@rfl-urbaniak rfl-urbaniak deleted the nl-add-poverty branch November 27, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants