-
Notifications
You must be signed in to change notification settings - Fork 7
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
Pre-processing #62
Pre-processing #62
Conversation
Hello @fedderw! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-05-12 18:50:27 UTC |
Also please format the code with |
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
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.
Some nits and minor suggestions. Could you check the TODO comments and either remove them if unnecessary, or move them into issues?
"New child poverty rate: " | ||
+ child_poverty_rate_string | ||
+ "%", | ||
"New child poverty rate: " + child_poverty_rate_string + "%", |
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.
doesn't need to be in this pr but these string pieces could be simplified with a function
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
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.
last minor things
subsetting thing Co-authored-by: Max Ghenis <mghenis@gmail.com>
Co-authored-by: Max Ghenis <mghenis@gmail.com>
Having numpy issues, the pre-processed csvs are not yet in there.
"Importing the numpy C-extensions failed. This error can happen for
many reasons, often due to issues with your setup or how NumPy was
installed."
fixes #30, fixes #34, fixes #35