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.

Sign up for GitHub

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

Jump to bottom

1996 patch google symptoms #1999

Merged
merged 37 commits into from
Jul 30, 2024
Merged

1996 patch google symptoms #1999

merged 37 commits into from
Jul 30, 2024

Conversation

aysim319
Copy link
Contributor

@aysim319 aysim319 commented Jul 22, 2024 •
edited
Loading

Description

Added new patching script along with refactoring code/ changing business logic

  • refactored date parsing into seperate module
    • help with readability for the main module
  • changed date logic to only grab the parameter set date range since of grabbing all dates after backfill
    • simplifies business logic and allowing the code to be more easily wrapped to run patches
  • added patching script for backfill with intended issue dates

Associated Issue(s)

@aysim319 aysim319 requested review from dshemetov and nmdefries July 22, 2024 14:22
@aysim319 aysim319 force-pushed the 1996-patch-google-symptoms branch 4 times, most recently from 764f581 to d46933b Compare July 22, 2024 20:04
@nmdefries
Copy link
Contributor

There are a lot of changes here, so I'd recommend doing a comparison of output with this code vs with the old code to make sure they're the same (or vary in expected ways).

@aysim319
Copy link
Contributor Author

aysim319 commented Jul 22, 2024 •
edited
Loading

There are a lot of changes here, so I'd recommend doing a comparison of output with this code vs with the old code to make sure they're the same (or vary in expected ways).

at least for a daily run:
main_receiving.zip
params_used.json
patch_receiving.zip

there seems to be no change in the content of the output
diff -rq main_receiving patch_receiving
Files main_receiving/google_symptoms.log and patch_receiving/google_symptoms.log differ

the log differ due to timestamps

@aysim319 aysim319 force-pushed the 1996-patch-google-symptoms branch from 9dcc0f1 to 689d869 Compare July 22, 2024 23:18
nmdefries
nmdefries reviewed Jul 23, 2024
View reviewed changes
Copy link
Contributor

@nmdefries nmdefries left a comment

Choose a reason for hiding this comment

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

Initial comments and questions. Still looking at logic.

google_symptoms/delphi_google_symptoms/patch.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/patch.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/pull.py Show resolved Hide resolved
@aysim319 aysim319 force-pushed the 1996-patch-google-symptoms branch from 2059ce6 to 45a2252 Compare July 23, 2024 21:15
nmdefries
nmdefries requested changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@nmdefries nmdefries left a comment

Choose a reason for hiding this comment

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

Some date padding questions, and a couple processing steps that seem too different between patch and normal production modes.

google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/date_utils.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/pull.py Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/patch.py Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/run.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/run.py Outdated Show resolved Hide resolved
google_symptoms/delphi_google_symptoms/run.py Outdated Show resolved Hide resolved
@aysim319 aysim319 force-pushed the 1996-patch-google-symptoms branch 3 times, most recently from 9727acf to f601a1d Compare July 24, 2024 22:45
@aysim319 aysim319 force-pushed the 1996-patch-google-symptoms branch 2 times, most recently from 1938ce7 to f59b925 Compare July 24, 2024 23:23
aysim319 and others added 19 commits July 30, 2024 12:24
@aysim319
changes based on suggestion
1cb791b
@aysim319
standardizing output dir in test
0e14bf1
@aysim319
changing back to session scope
e181f54
@aysim319
more logging and update params
ff22993
@aysim319
adding back conditional for num_export_days
cbebcc3
@aysim319
test_run does not like fixtures
240fe5d
@nmdefries @aysim319
make deep copies of params fixtures
dd819a7
@nmdefries @aysim319
linting
20c14d9
@nmdefries @aysim319
test patch check if num_export_days is set or not
a3a55ae
@nmdefries @aysim319
patch fn takes params as an arg, like run_module
eb6a9be
@nmdefries @aysim319
list values added using actual day as an example
46b6317
@nmdefries @aysim319
move pull date creation into pull_gs_data
5532aac
@nmdefries @aysim319
don't modify num_export_days; define start/end date in terms of it an…
d209a9c 
…d lag
@nmdefries @aysim319
refer to params by name; patch_flag -> custom_run flag
66aa2cd
@nmdefries @aysim319
use test data based on recent actual indicator run
0708a22
@nmdefries @aysim319
say where gold test data came from
d0d3a7a
@nmdefries @aysim319
not all date_utils tests need metadata
766f49e
@nmdefries @aysim319
formatting
a04bdea
@nmdefries @aysim319
fn docs
6fce6d8
@aysim319 aysim319 force-pushed the 1996-patch-google-symptoms branch from 1215753 to f4981ea Compare July 30, 2024 16:24
@aysim319 aysim319 force-pushed the 1996-patch-google-symptoms branch from f4981ea to 528a1e3 Compare July 30, 2024 16:30
@aysim319 aysim319 merged commit 4168db5 into main Jul 30, 2024
16 checks passed
@nmdefries
Copy link
Contributor

@aysim319 I think the last set of comments didn't make it into this PR (or maybe I missed the commit?). Could you add those in on a separate PR?

@nmdefries nmdefries mentioned this pull request Jul 31, 2024
Closed
@nmdefries nmdefries deleted the 1996-patch-google-symptoms branch July 31, 2024 15:46
This was referenced Sep 11, 2024
Closed
Merged
@melange396 melange396 mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmdefries nmdefries nmdefries approved these changes

@dshemetov dshemetov dshemetov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aysim319 @nmdefries @dshemetov