-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add support for missing NAs in estimate_infection() model #528
Conversation
e904fe4
to
9647232
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if cb5705c is merged into main:
|
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.
Very nice! A couple of minor questions/confusions on my side.
Co-authored-by: Sebastian Funk <[email protected]>
Co-authored-by: Sebastian Funk <[email protected]>
Co-authored-by: Sebastian Funk <[email protected]>
Co-authored-by: Sebastian Funk <[email protected]>
25c1404
to
7533201
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 876a533 is merged into main:
|
* add a lookup to estimate_infections * add R side support * don't internally impute missing as zero * update news * fix data preprocessing order * correction data ingestion * clean up filtering of leading zeros * error check create_clean_reported_cases and add unit tests to cover function * correct handling of missing data in data preprocessing: * refine data preprocessing * update news and tests * update global variables * Update NEWS.md Co-authored-by: Sebastian Funk <[email protected]> * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * fix line length linting * Document --------- Co-authored-by: Sebastian Funk <[email protected]> Co-authored-by: GitHub Actions <[email protected]>
* add a lookup to estimate_infections * add R side support * don't internally impute missing as zero * update news * fix data preprocessing order * correction data ingestion * clean up filtering of leading zeros * error check create_clean_reported_cases and add unit tests to cover function * correct handling of missing data in data preprocessing: * refine data preprocessing * update news and tests * update global variables * Update NEWS.md Co-authored-by: Sebastian Funk <[email protected]> * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * fix line length linting * Document --------- Co-authored-by: Sebastian Funk <[email protected]> Co-authored-by: GitHub Actions <[email protected]>
This PR adds support for missing data in the form of NAs but dropping them from the likelihood rather than imputing that they are zero and then optionally
Note that this is a breaking change and may be undesirable for some users where missing dates do indicate zeros vs being truly missing.
I have tested by knocking out data from the example and running the model.