Implement proper tidyselect for epi_slide_opt
#452
Conversation
|
@brookslogan requesting an initial/broad review of this. Tests are pending. |
column names to avoid running the same compuation for each `.data_group`
| #' # Remove a nonessential var. to ensure new col is printed | ||
| #' dplyr::select(geo_value, time_value, cases, cases_7dav) %>% | ||
| #' dplyr::select(geo_value, time_value, cases, cases_7dav = slide_value_cases) %>% |
There was a problem hiding this comment.
musing: This also makes me wish for something like new_col_name that actually did what you'd naturally guess here (without requiring names_sep = NULL). Though I vaguely recall we were putting off coming up with a better interface here (especially regarding better prefixes/suffixes for means and sums)? Part of this was due to trying for consistency with what epi_slide() technically does, but currently I'm thinking a bit of technical inconsistency is fine. (And the solution to the inconsistency seems like it'd be removing as_list_col and names_sep globally, supporting multiple tidyeval expressions, and mirroring dplyr support for things like tibble(x = 1) %>% mutate(tibble(y = x + 1, z = x + 2)); that last one seems tricky but might be possible. Then we could make .new_col_name or .new_col_names or actually do what it sounds like since we'd never be automatically unnesting. I don't /think/ this loses out on functionality since the remaining use cast of nesting & unnesting I can think of --- hiding your true number of rows from epi_slide's number-of-row constraints --- you already have to do as_list_col = TRUE and unnest manually.)
There was a problem hiding this comment.
If we fully supported multiple tidyeval expressions, would we need to have a .new_col_name param at all? The renaming functionality in tidyselect is sufficient if we don't do the unnesting step.
For the epi_slide_opt and derived fns, we are currently passing ... as args to the slide computation (frollmean, etc) so multiple tidy expressions might be an issue
There was a problem hiding this comment.
The .new_col_name would still be helpful/convenient for non-tidy-eval (function/formula) epi_slide and for epi_slide_opt.
Regarding ... as args: I don't think we've ever been able to actually use this feature in a useful way, so we could consider removing it if it's a problem. But I'm not sure it is. The major pain I was thinking of at least was detecting whether f is a tidyeval computation or not when we allow unnamed tidyeval computations. This seems potentially possible: in the first/each computation application, evaluate f as if it were tidyeval, check if it actually results in a function or formula, and if so, adjust appropriately; if it results in a tibble, then it's the tidyeval on f and each of ...; if f is missing it's also tidyeval.
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
Turn off renaming behavior (see comments about awkward/confusing naming here).
Closes #435