Skip to content

Fix epix_slide example referring to nonexistent column #137

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

Closed
brookslogan opened this issue Jul 7, 2022 · 2 comments · Fixed by #149
Closed

Fix epix_slide example referring to nonexistent column #137

brookslogan opened this issue Jul 7, 2022 · 2 comments · Fixed by #149
Assignees
Labels
documentation Improvements or additions to documentation P1 medium priority

Comments

@brookslogan
Copy link
Contributor

options(warn=1L)
devtools::run_examples()

produces (along with some other output and warnings)

> epix_slide(x = archive_cases_dv_subset,
+            f = ~ mean(.x$case_rate),
+            n = 3,
+            group_by = geo_value,
+            ref_time_values = time_values,
+            new_col_name = 'case_rate_3d_av')
Warning: Unknown or uninitialised column: `case_rate`.
Warning in mean.default(.x$case_rate) :
  argument is not numeric or logical: returning NA
Warning: Unknown or uninitialised column: `case_rate`.
Warning in mean.default(.x$case_rate) :
  argument is not numeric or logical: returning NA
Warning: Unknown or uninitialised column: `case_rate`.
Warning in mean.default(.x$case_rate) :
  argument is not numeric or logical: returning NA
[....................]

It looks like the column has been renamed or removed (maybe while addressing #83):

> archive_cases_dv_subset[["DT"]] %>% names()
[1] "geo_value"       "time_value"      "version"         "percent_cli"    
[5] "case_rate_7d_av"

So we should either change the column used for this computation, or reintroduce the old column and address #83 via selecting the interesting columns somewhere within those examples.

@brookslogan brookslogan added documentation Improvements or additions to documentation P2 low priority P1 medium priority and removed P2 low priority labels Jul 7, 2022
@mgyliu mgyliu self-assigned this Jul 11, 2022
@rachlobay
Copy link
Collaborator

rachlobay commented Jul 13, 2022

I could be wrong, but I think this relates back to changes made a couple months ago to the internal data (for ex. d8cf124) and so looking at those changes could possibly help w/this issue.

@mgyliu
Copy link
Contributor

mgyliu commented Jul 13, 2022

Yeah I traced it down to the same source. Thanks for commenting @rachlobay!

Summary of why this happened: When archive_cases_dv_subset was first created (this PR), it was called archive_cases_dv and the column was called case_rate. In a subsequent PR, the dataset was renamed to archive_cases_dv_subset and the column was changed to case_rate_7d_av. The column values themselves were the same across both PRs (they were always the 7-day average case rate).

To fix the warnings, I changed the column in the example to case_rate_7d_av.

Currently having trouble with devtools::check on my local machine but I will put up a PR for this soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation P1 medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants