-
Notifications
You must be signed in to change notification settings - Fork 8
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
Slide improvements pass #477
Conversation
from breaking changes or deprecations
(Random implementation notes: I also investigated an |
@dshemetov I've updated |
- Correct check for whether data masking was used - Update checks and error messages for currently-accepted kinds of objects - Make some variable naming more consistent between files
f881586
to
ccbba20
Compare
to match `mutate` behavior
f0d2d62
to
a1c7020
Compare
Perf has 3-4x slowdown due to validate_tibble and as_environment in as_slide_computation. To address later. Perf comparisons of dev against this branch (branch a and branch b, respectively): profvis_slide_changes.zip, run with # Profile
# Branch A
# epi_slide
p <- profvis::profvis({
jhu_csse_county_level_subset %>%
group_by(geo_value) %>%
epi_slide(cases_sum5 = sum(.x$cases), before = 5)
})
now <- format(Sys.time(), "%Y-%m-%d %H_%M_%S %Z")
htmlwidgets::saveWidget(p, glue::glue("profvis_{now}.html"), selfcontained = TRUE)
# Branch B
# epi_slide
p <- profvis::profvis({
jhu_csse_county_level_subset %>% epi_slide(cases_sum5 = sum(.x$cases), .window_size = 5)
})
now <- format(Sys.time(), "%Y-%m-%d %H_%M_%S %Z")
htmlwidgets::saveWidget(p, glue::glue("profvis_{now}.html"), selfcontained = TRUE) |
a1948bb
to
5ce988b
Compare
41263d3
to
e3dfa32
Compare
cc3cb79
to
a3e805d
Compare
* key_colnames order change * replace kill_time_value with exclude arg in key_colnames * move duplicate time_values check in epi_slide
2b63c52
to
8202c0e
Compare
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.
Good to go. Will merge to main after this. epipredict PR will follow soon after.
Checklist
Please:
PR).
brookslogan, nmdefries.
DESCRIPTION
. Always incrementthe patch version number (the third number), unless you are making a
release PR from dev to main, in which case increment the minor version
number (the second number).
(backwards-incompatible changes to the documented interface) are noted.
Collect the changes under the next release number (e.g. if you are on
1.7.2, then write your changes under the 1.8 heading).
process.
Change explanations for reviewer
Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch
epi[x]_slide
inner computations #240other_keys
#446other_keys
to be printed, a constructor parameter, and more clearly documented #186epi_slide
andepix_slide
args #510...
#162epix_slide
is over versions #146