Refactor slide computation function generation and move to as_slide_computation
#337
Conversation
In `as_slide_computation`, call `eval_tidy` on the quosure `x` passed directly to `as_slide_computation`, rather than generating an `f_wrapper` computation function that takes the quosure as an argument. The computation function is regenerated each time `slide` is called, with a new quosure, so the computation function doesn't need to be flexible enough to run with different `quo`s. This change makes the function/formula and quosure forks more similar, since `group_modify`, `slide_one_grp` in the `epi_slide` case, and `comp_one_grp` in the `epix_slide` case no longer need a `quo` argument in the quosure fork. To make the two forks fully identical, the quosure fork was changed to pass an empty set of dots to the computation functions. The `as_slide_computation` call and `group_modify` call can now be pulled out of the if/else block.
nmdefries
changed the title
as_slide_computation
Drop `calc_ref_time_value` and `before` args to `as_slide_computation`; they were only used to calculate `.ref_time_value` for `epi_slide` computations.
…fns too, to avoid name conflicts with dots
`... = missing_arg(); some_function(...)` will pass zero arguments to `some_function`, so we don't need to accept `...` in converted quosures.
|
I have a few minor edits queued up locally that I'll ask you to look over after I finish a couple tests. Question: what do you think about making |
Could you be a little more specific on this? What does "baking in the dots" mean? Are you saying that |
instead of ignoring them.
Yes.
Not sure what counts as "local vars". I was thinking one approach would be: as_slide_computation = function(f, ...) {
[...]
if (rlang::dots_n(...) > 0L) {
function(.x, .group_key, .ref_time_value) {
f(.x, .group_key, .ref_time_value, ...)
}
} else {
# in case forwarding dots is expensive, we could special-case 0 dots to not incur the forwarding expense
f
}
[...]
} |
A couple warnings were leaking out of `test()` from test cases that produced a warning + error but only used `expect_error()`. Use `expect_warning` in addition to test for specific expected warnings.
There was a problem hiding this comment.
I'm not sure what I meant by untested changes locally. I've pushed the tweaks I had. Please sanity-check them. I think the only lingering question is about whether to try to "bake in" the dots + de-duplicate the tidyeval stuff. Maybe I'll just mark this approved and that could be in another PR?
Description
Move
assert_sufficient_f_argscalls to withinas_slide_computation.Move computation function creation into
as_slide_computation. Includes:epi_slidecomputation function creation for the function/formula caseepix_slideforwarding an unmodified function in the function/formula case, andepi[x]_slidecomputation function creation in the quosure caseChanges involved adding quosure-handling block to
as_slide_computation. Stop passingquoas an argument to the quosure-case functions. Makeepi_slidequosure-case data masking procedure match that ofepix_slide(for easier refactoring).Changes
R/grouped_epi_archive.RR/slide.RR/utils.R.datamatches access via.xand without a pronoun.Fixes
Closes #326