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

2085 add proportions nhsn #2111

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

2085 add proportions nhsn #2111

wants to merge 13 commits into from

Conversation

aysim319
Copy link
Contributor

Description

address #2085

Changelog

added new function that checks the metadata for last update
-> the function also checks for 503 error
-> added signals for total reporting hospitals

Associated Issue(s)

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.

Are you planning on adding the new RSV signals in this PR or in a separate one?

nhsn/delphi_nhsn/constants.py Outdated Show resolved Hide resolved
Comment on lines 29 to 31
updated_timestamp = datetime.utcfromtimestamp(int(response["rowsUpdatedAt"]))
now = datetime.utcnow()
recently_updated = (now - updated_timestamp) < timedelta(days=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I think this "recently-updated" logic is sufficient but not robust. For example, if we fail to pull data for multiple days, the next day we run we would not pull data we had never seen before if it was not posted in the last day.

The more robust solution would be to save last pull's updated_timestamp to a local file. We would then load that and compare updated_timestamp to that -- if exactly equal, skip update; if unequal, pull data.

Copy link
Contributor Author

@aysim319 aysim319 Feb 4, 2025

Choose a reason for hiding this comment

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

definitely makes sense and something I didn't think about! The only thing I did different was use the api instead of scanning the file since I imagine the file list is going to go and doesn't make much sense to scan the file list every day

nhsn/delphi_nhsn/pull.py Outdated Show resolved Hide resolved
@aysim319
Copy link
Contributor Author

aysim319 commented Feb 4, 2025

Are you planning on adding the new RSV signals in this PR or in a separate one?

I was originally planning for a seperate one. I was considering adding the new signal in this PR, but when I tried to add just the new columns and locally ran the tests and ran across issues and looks like it might be more involved, so i think it'd be better to create a seperate one. I also kinda shoved in other issues (retry and daily checking) and didn't want to add more things on top

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.

add retry for 50x error for scorata api and also throw error when that happens
2 participants