Add specialized epix_slide for epi_slide_opt #611
+2,337
−765
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
brookslogan
force-pushed
the
lcb/archive-agg
branch
6 times, most recently
from
2949de6 to
661cece
Compare
brookslogan
force-pushed
the
lcb/archive-agg
branch
15 times, most recently
from
57dd9ea to
6dcf722
Compare
brookslogan
force-pushed
the
lcb/archive-agg
branch
from
3a147d0 to
6d801a2
Compare
brookslogan
force-pushed
the
lcb/archive-agg
branch
3 times, most recently
from
1fbe7b8 to
dd84924
Compare
brookslogan
force-pushed
the
lcb/archive-agg
branch
from
09a1af5 to
5fc1dc5
Compare
14% or 17% time reduction for 7daving on a couple of test archives.
brookslogan
force-pushed
the
lcb/archive-agg
branch
from
665d558 to
f48fbf0
Compare
…chars Something still seems different; some Date arithmetic stuck out, and attempts to avoid helped somewhat, but this still seems slower for archives than pre-unified approach.
As `vec_c` is slower on 3 scalar `Date`s, and it's probably pretty uncommon to use `time_value`s with `c` incompatible with `vec_c` (though possible... maybe tibbles with year, week, wday, like from MMWRweek; though these would probably break inside archive DT).
Revert earlier change to try to reduce time inside `time_minus_time_in_n_steps`, as `c.Date` is more costly. This also works back towards generality from the previous `vec_c` -> `c` change.
Also change a map -> lapply so we can get srcref to internal error immediately, add a missed .window_size-missingness check, delete some helper functions that were sort of helpful but also tacked on significant run time when used in a natural way.
brookslogan
force-pushed
the
lcb/archive-agg
branch
from
e42d6b4 to
62375f8
Compare
The error message says that slide computations can't be rownamed data frames. This is both bad wording (having rownames != having non-automatic rownames) and we're not actually enforcing any restriction of the sort anyway. We drop/ignore the rownames though (via dplyr/tibble). In `epix_slide`, the allowance of non-automatic rownames was deliberate; in `epi_slide`, it also seems likely the way to go; one might convert to `data.frame` and have some filter-like operation return non-automatic rownames.
Use `... =` to specify this. Prevents some confusing error messages on ``` archive_cases_dv_subset %>% epi_slide_opt(percent_cli, frollmean, 0, .window_size = 7) ``` but does not solve all issues that can happen with unnamed `...`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TODO
epi_slide_opt(.before = Inf)behavior from refactor (CHECK check not running at the moment despite some others running?)