Add columns to covidcast for issue and lag. #122
Conversation
krivard
commented
Jun 10, 2020
krivard
commented
- Database schema change in ddl (Brian please double-check)
- Supporting infra in acquisition
- Extend all data insertion test cases to include toy issue and lag
- Test that we can insert a new issue for an extant (source,signal,date,geo)
* Supporting infra in acquisition * Extend all data insertion test cases to include toy issue and lag * Test that we can insert a new issue for an extant (source,signal,date,geo)
undefx
left a comment
undefx
left a comment
There was a problem hiding this comment.
hey, just leaving a drive-by review. hope that's ok.
this looks good to me, and it's cool to see that you all are expanding on this dataset with issue and lag. exciting! also, very happy to see tests being updated. keep up the good work.
please consider all my comments to be optional/advisory.
| import glob | ||
| import os | ||
| import re | ||
| from datetime import date |
There was a problem hiding this comment.
recommend ordering imports alphabetically within each import group, per convention (not 100% followed in all files, but in the majority i think)
| # third party | ||
| import pandas | ||
|
|
||
| # delphi |
There was a problem hiding this comment.
for consistency, this section is usually titled "first party"
| sensible_day = 1 <= day <= 31 | ||
|
|
||
| return nearby_year and valid_month and sensible_day | ||
| if not (nearby_year and valid_month and sensible_day): return False |
There was a problem hiding this comment.
recommend moving the return False to the line below, indented, because:
- compound statements are discouraged by the python style guide
- coverage report for py3tester is misleading for compound statements
| sensible_week = 1 <= week <= 53 | ||
|
|
||
| return nearby_year and sensible_week | ||
| if not nearby_year and sensible_week: return False |
|
|
||
| @staticmethod | ||
| def find_csv_files(scan_dir, glob=glob): | ||
| def find_csv_files(scan_dir, issue=(date.today(),-1), glob=glob): |
There was a problem hiding this comment.
super nit, but i recommend using a linter like pycodestyle (but there are others ofc) to help spot style things. e.g. here it complains that there should be a space between , and -1. of course that's just someone's preference (pep8), but it helps to keep a consistent style for a large codebase.
| The return value is a tuple of (path, details), where, if the path was | ||
| valid, details is a tuple of (source, signal, time_type, geo_type, | ||
| time_value) (otherwise None). |
There was a problem hiding this comment.
suggest updating this to reflect the new issue_value and lag_value values returned
krivard
merged commit ab20e46
into
cmu-delphi:feature/extend-covidcast-to-store-issue-date
3 participants
