Add specialized epix_slide for epi_slide_opt #611

brookslogan · 2025-02-26T21:15:06Z

Checklist

Please:

Make sure this PR is against "dev", not "main" (unless this is a release
PR).
Request a review from one of the current main reviewers:
brookslogan, nmdefries.
Makes sure to bump the version number in DESCRIPTION. Always increment
the 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).
Describe changes made in NEWS.md, making sure breaking changes
(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).
See DEVELOPMENT.md for more information on the development
process.

Change explanations for reviewer

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

Resolves Implement smart sparse archive -> archive slide #609.

brookslogan · 2025-02-26T21:40:27Z

handle all possible .f, before
finish docs, tests
[-] port map_ea at least, maybe base on iterators
- handle in Add separate epi_archive constructor based on a "iterator"/list/df of snapshots #173 or beyond
in-source todos

For re-use in an epix_epi_slide_opt

We actually coincidentally caught the tibble-to-DT key-setting issue with code intended for addressing a separate issue with `data.table`s passed in.

nmdefries

Various nits and some questions. The logic looks fine to me, but given the complexity of this change, someone else should review as well.

nmdefries · 2025-03-10T22:38:17Z

R/epi_slide_opt_edf.R

+        "problem with {rlang::expr_label(rlang::caller_arg(f))}",
+        "i" = "`f` must be one of `data.table`'s rolling functions (`frollmean`,


nit:

Suggested change

"problem with {rlang::expr_label(rlang::caller_arg(f))}",

"i" = "`f` must be one of `data.table`'s rolling functions (`frollmean`,

"problem with {rlang::expr_label(rlang::caller_arg(.f))}",

"i" = "`.f` must be one of `data.table`'s rolling functions (`frollmean`,

nmdefries · 2025-03-10T22:48:12Z

R/epi_slide_opt_edf.R

+#' @keywords internal
+across_ish_names_info <- function(.x, time_type, col_names_quo, .f_namer,
+                                  .window_size, .align, .prefix, .suffix, .new_col_names) {
+  # The position of a given column can be differ between input `.x` and


nit:

Suggested change

# The position of a given column can be differ between input `.x` and

# The position of a given column can differ between input `.x` and

nmdefries · 2025-03-10T23:17:02Z

R/epi_slide_opt_edf.R

+#'
+#' * On `epi_df`s, it will take care of looping over `geo_value`s, temporarily
+#'   filling in time gaps with `NA`s and other work needed to ensure there are
+#'   exactly n consecutive time steps per computation, and has some other


nit:

Suggested change

#' exactly n consecutive time steps per computation, and has some other

#' exactly `n` consecutive time steps per computation, and has some other

nmdefries · 2025-03-10T23:17:44Z

R/epi_slide_opt_edf.R

+#' @description
+#'
+#' `epi_slide_opt` calculates n-time-step rolling means&sums,
+#' cumulative/"running" means&sums, or other operations supported by


nit:

Suggested change

#' cumulative/"running" means&sums, or other operations supported by

#' cumulative/"running" means&sums, and other operations supported by

nmdefries · 2025-03-12T15:27:08Z

R/patch.R

+  # Most input validation + handle NULL earlier_snapshot. This is a small function so
+  # use faster validation variants:
+  if (!is_tibble(later_tbl)) {
+    cli_abort("`later_tbl` must be a tibble")


suggestion: please add class names to errors.

nmdefries · 2025-03-13T20:48:40Z

R/epi_slide_opt_archive.R

+        group_updates <- group_values %>%
+          nest(.by = version, .key = "subtbl") %>%
+          arrange(version)
+        # TODO move nesting inside the helper?


issue: some TODOs and "XXX"s left. Please either implement, or convert to issues and link to from code.

nmdefries · 2025-03-13T22:17:12Z

R/epi_slide_opt_archive.R

+      # in slide outputs in the range t1-after..t2+before, and to compute those
+      # slide values, we need to look at the input snapshot from
+      # t1-after-before..t2+before+after. nolint: commented_code_linter


question: I understand how we arrived at t1-after..t2+before, but where did the t1-after-before.... come from?

Suppose before = 6, after = 0 and there's an update to the 1d value at time t alone. We need to compute the trailing 7dav for t..t+6. But in order to compute the trailing 7dav for time t, we need 1d data from t-6..t. So we end up needing the union of the ranges t-6..t, t-5..t+1, t-4..t+2, ..., t..t+6, which gives us t-6..t+6.

nmdefries · 2025-03-14T15:02:23Z

R/epi_slide_opt_archive.R

+    if (f_from_package == "data.table") {
+      for (col_i in seq_along(in_colnames)) {
+        if (before == Inf) {
+          slide[[out_colnames[[col_i]]]] <-
+            f_dots_baked(slide[[in_colnames[[col_i]]]], seq_len(slide_nrow), adaptive = TRUE)
+        } else {
+          out_col <- f_dots_baked(slide[[in_colnames[[col_i]]]], before + after + 1L)
+          if (after != 0L) {
+            # data.table always puts NAs at tails, even with na.rm = TRUE; chop
+            # off extra NAs from beginning and place missing NAs at end:
+            out_col <- c(out_col[seq(after + 1L, slide_nrow)], rep(NA, after))
+          }
+          slide[[out_colnames[[col_i]]]] <- out_col
+        }
+      }
+    } else if (f_from_package == "slider") {
+      for (col_i in seq_along(in_colnames)) {
+        slide[[out_colnames[[col_i]]]] <- f_dots_baked(slide[[in_colnames[[col_i]]]], before = before, after = after)
+      }
+    } else {
+      cli_abort("epiprocess internal error: `f_from_package` was {format_chr_deparse(f_from_package)}")
+    }


suggestion: data.table functions can process more than one column at a time and I assume it's faster. So we don't need the column loop into the data.table branch here.

Suggested change

if (f_from_package == "data.table") {

for (col_i in seq_along(in_colnames)) {

if (before == Inf) {

slide[[out_colnames[[col_i]]]] <-

f_dots_baked(slide[[in_colnames[[col_i]]]], seq_len(slide_nrow), adaptive = TRUE)

} else {

out_col <- f_dots_baked(slide[[in_colnames[[col_i]]]], before + after + 1L)

if (after != 0L) {

# data.table always puts NAs at tails, even with na.rm = TRUE; chop

# off extra NAs from beginning and place missing NAs at end:

out_col <- c(out_col[seq(after + 1L, slide_nrow)], rep(NA, after))

}

slide[[out_colnames[[col_i]]]] <- out_col

}

}

} else if (f_from_package == "slider") {

for (col_i in seq_along(in_colnames)) {

slide[[out_colnames[[col_i]]]] <- f_dots_baked(slide[[in_colnames[[col_i]]]], before = before, after = after)

}

} else {

cli_abort("epiprocess internal error: `f_from_package` was {format_chr_deparse(f_from_package)}")

}

if (f_from_package == "data.table") {

if (before == Inf) {

slide[, out_colnames] <-

f_dots_baked(slide[, in_colnames], seq_len(slide_nrow), adaptive = TRUE)

} else {

out_cols <- f_dots_baked(slide[, in_colnames], before + after + 1L)

if (after != 0L) {

# data.table always puts NAs at tails, even with na.rm = TRUE; chop

# off extra NAs from beginning and place missing NAs at end:

out_cols <- purrr::map(out_cols, function(.x) {

c(.x[seq(after + 1L, slide_nrow)], rep(NA, after))

})

}

slide[, out_colnames] <- out_cols

}

} else if (f_from_package == "slider") {

for (col_i in seq_along(in_colnames)) {

slide[[out_colnames[[col_i]]]] <- f_dots_baked(slide[[in_colnames[[col_i]]]], before = before, after = after)

}

} else {

cli_abort("epiprocess internal error: `f_from_package` was {format_chr_deparse(f_from_package)}")

}

nmdefries · 2025-03-14T15:08:39Z

R/epi_slide_opt_archive.R

+          if (after != 0L) {
+            # data.table always puts NAs at tails, even with na.rm = TRUE; chop
+            # off extra NAs from beginning and place missing NAs at end:
+            out_col <- c(out_col[seq(after + 1L, slide_nrow)], rep(NA, after))


issue: This doesn't respect data.table's fill param. If a computation uses fill, these values should not be NA. I think what we really want to do is move the first after values to the end of the result.

epi_slide_opt.epi_df has the same behavior.

Nice catch! I think I was worried about some garbage-rather-than-NA being in the tails which is why I didn't just copy that approach, but on second look that worry seems unfounded.

nmdefries · 2025-03-14T15:33:06Z

R/epi_slide_opt_archive.R

+    prev_inp_snapshot <<- inp_snapshot
+    prev_out_snapshot <<- out_snapshot
+    out_diff$version <- version
+    out_diff


suggestion: this always returns a compactified archive. Will users ever want a non-compactified result? If we add a compactify arg or inherit compactification status from the original archive, the user could toggle whether we return out_snapshot here instead of out_diff.

Perhaps; some thoughts:

Recording all full out_snapshots seems like it will potentially give a very hefty result, and they could get that same result by something like epix_slide(~ .x) on the result or a helper function doing something like that.

Recording just out_snapshot restricted to times that were in inp_update plus any times that are required for validity (e.g., an change in just the time t 1d value producing an update to the trailing 7dav for t+1..t+6 as well as t) seems maybe more useful. I'm not sure how hard/slow this would be; I had some issues trying to selectively apply compactify_tol to the 7dav/etc. cols and set aside that idea.

dsweber2 assigned brookslogan Feb 26, 2025

brookslogan force-pushed the lcb/archive-agg branch 6 times, most recently from 2949de6 to 661cece Compare February 28, 2025 05:33

brookslogan mentioned this pull request Feb 28, 2025

Add abstraction to allow for easier implementation of streaming PID recalibration #616

Open

brookslogan force-pushed the lcb/archive-agg branch 14 times, most recently from 9903efd to e908168 Compare March 6, 2025 19:33

brookslogan added 7 commits March 6, 2025 16:55

refactor: hoist some epi_slide_opt pre-processing to helpers

d8700fb

For re-use in an epix_epi_slide_opt

feat: WIP epix_epi_slide_opt

9effa5c

epix_epi_slide_opt: don't re-order input columns in output

7f97236

feat: support before = Inf in epix_epi_slide_opt

e59e2e9

feat(epi_slide_opt): improve feedback when .f is forgotten

486b488

fix(epix_epi_slide_opt): support ... forwarding

df36e2c

Clean up some comments&code + note bug

8b94f79

brookslogan force-pushed the lcb/archive-agg branch from fcfd9c2 to 5d24262 Compare March 10, 2025 10:42

brookslogan added 2 commits March 10, 2025 10:46

docs(new_epi_archive): roxygen2 for new param requirements

56d7cb0

fix(epi_slide_opt.epi_archive): as.data.table(tibble) key setting

0435460

brookslogan force-pushed the lcb/archive-agg branch from 5d24262 to 40e8778 Compare March 10, 2025 17:47

tests(epi_slide_opt): on example data sets

dd84924

brookslogan force-pushed the lcb/archive-agg branch from 1fbe7b8 to dd84924 Compare March 10, 2025 19:52

Fix & test epi_slide_opt.grouped_epi_archive behavior

84c9db0

brookslogan marked this pull request as ready for review March 10, 2025 20:00

brookslogan requested review from dsweber2 and nmdefries March 10, 2025 20:00

Rename approx_equal -> vec_approx_equal

8149aa2

brookslogan force-pushed the lcb/archive-agg branch from 7c3d243 to 8149aa2 Compare March 10, 2025 20:13

dsweber2 assigned dsweber2 and nmdefries Mar 10, 2025

dshemetov self-requested a review March 10, 2025 20:17

brookslogan added 9 commits March 10, 2025 13:31

Fix missing n_groups import + epiprocess::: CHECK lint

5df41cd

Address additional lints, CHECK issues

cbe1cb1

Address missing ::: in tests

3bb956c

Fix another missing :::

27ff6dd

Fix + add NEWS.md entries

58ce1d0

We actually coincidentally caught the tibble-to-DT key-setting issue with code intended for addressing a separate issue with `data.table`s passed in.

Fix CHECK doc line length lint

afc5d06

Fix missing library(dplyr) in example

df8ad0c

Fix {epiprocess} -> {epiprocess} in roxygen

168af56

docs: add vec_approx_equal to pkgdown reference index

cc22517

brookslogan mentioned this pull request Mar 11, 2025

Add separate epi_archive constructor based on a "iterator"/list/df of snapshots #173

Open

nmdefries reviewed Mar 14, 2025

View reviewed changes

docs(vec_approx_equal): mention vctrs::vec_proxy_equal

bd86054

This was referenced Mar 19, 2025

Deal with dist_quantiles() CHECK issue #631

Open

Address dist_quantiles() deprecation #634

Open

Generalize sum_groups_epi_df to archives #636

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add specialized epix_slide for epi_slide_opt #611

Add specialized epix_slide for epi_slide_opt #611

brookslogan commented Feb 26, 2025 •

edited

Loading

brookslogan commented Feb 26, 2025 •

edited

Loading

nmdefries left a comment •

edited

Loading

nmdefries Mar 10, 2025

nmdefries Mar 10, 2025

nmdefries Mar 10, 2025

nmdefries Mar 10, 2025

nmdefries Mar 12, 2025

nmdefries Mar 13, 2025

nmdefries Mar 13, 2025

brookslogan Mar 19, 2025

nmdefries Mar 14, 2025

nmdefries Mar 14, 2025

brookslogan Mar 19, 2025

nmdefries Mar 14, 2025

brookslogan Mar 19, 2025

		"problem with {rlang::expr_label(rlang::caller_arg(f))}",
		"i" = "`f` must be one of `data.table`'s rolling functions (`frollmean`,

	# The position of a given column can be differ between input `.x` and
	# The position of a given column can differ between input `.x` and

	#' exactly n consecutive time steps per computation, and has some other
	#' exactly `n` consecutive time steps per computation, and has some other

	#' cumulative/"running" means&sums, or other operations supported by
	#' cumulative/"running" means&sums, and other operations supported by

Add specialized epix_slide for epi_slide_opt #611

Are you sure you want to change the base?

Add specialized epix_slide for epi_slide_opt #611

Conversation

brookslogan commented Feb 26, 2025 • edited Loading

Checklist

Change explanations for reviewer

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

brookslogan commented Feb 26, 2025 • edited Loading

nmdefries left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brookslogan commented Feb 26, 2025 •

edited

Loading

brookslogan commented Feb 26, 2025 •

edited

Loading

nmdefries left a comment •

edited

Loading