Leap time handling #445

dsweber2 · 2025-02-27T23:28:17Z

Checklist

Please:

Make sure this PR is against "dev", not "main".
Request a review from one of the current epipredict main reviewers:
dajmcdon.
Make sure to bump the version number in DESCRIPTION and NEWS.md.
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
0.7.2, then write your changes under the 0.8 heading).
Consider pinning the epiprocess version in the DESCRIPTION file if
- You anticipate breaking changes in epiprocess soon
- You want to co-develop features in epipredict and epiprocess

Change explanations for reviewer

So weeks and days both have to deal with inconsistent numbers of days. What this does:

separate out the "weird" day/week into a separate bin, labeled 999 and only use up to the "normal" number of days/weeks (so 365/52)
if the window goes over the boundary (so includes both week 52 and week 1, or both day 59 and day 60), then add in the data from
if the window is around the leap day/week, include the window days before and the window days after (so days 57, 58, 59, leap_day, 60, 61, 62)

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

Resolves #{issue number}

dajmcdon

Some suggestions, but I think there's a logic error when rolling with the iter. (Probably was there before as well)

dajmcdon · 2025-02-28T17:50:46Z

R/step_climate.R

-      .idx = x$time_aggr(time_value), .weights = wts,
-      .idx = (.idx - x$forecast_ahead) %% modulus,
-      .idx = dplyr::case_when(.idx == 0 ~ modulus, TRUE ~ .idx)
+      .idx = time_value %m-% ahead_period,


Suggested change

.idx = time_value %m-% ahead_period,

# subtracts a month w/o rollover (usual behavior on weeks/days)

.idx = time_value %m-% ahead_period,

dajmcdon · 2025-02-28T17:54:08Z

R/step_climate.R

@@ -341,12 +350,63 @@ roll_modular_multivec <- function(col, .idx, weights, aggr, window_size, modulus
    tidyr::nest(data = c(col, weights), .by = .idx)
  out <- double(nrow(tib))
  for (iter in seq_along(out)) {
-    entries <- (iter - window_size):(iter + window_size) %% modulus
+    # +1 from 1-indexing
+    entries <- ((iter - window_size):(iter + window_size) %% modulus)


Suggested change

entries <- ((iter - window_size):(iter + window_size) %% modulus)

entries <- (iter - window_size):(iter + window_size) %% modulus

Don't need the outer paren do we?

Actually, I'm concerned this logic fails if given partial years. Same with keying on the iteration below. Suppose that I give you only data for January-March for 25 years (and ask for daily). Then max(iter)=91 so the leap branch will never happen. The same would be true if I removed the summer for every year and asked for epiweek climate. I think it needs to key on the .idx rather than iter.

The tests all presume you have every possible .idx covered and arranged.

dajmcdon · 2025-02-28T18:08:52Z

R/step_climate.R

+    } else if (modulus == 52) {
+      # we need to grab just the window around the leap week on the leap week
+      if (iter == 53) {
+        entries <- ((53 - window_size):(53 + window_size - 1) %% 52)


Suggested change

entries <- ((53 - window_size):(53 + window_size - 1) %% 52)

entries <- (53 - window_size):(53 + window_size - 1) %% 52

dajmcdon · 2025-02-28T18:34:25Z

R/step_climate.R

+      # we need to grab just the window around the leap day on the leap day
+      if (iter == 366) {
+        entries <- ((59 - window_size):(59 + window_size - 1) %% modulus)
+        entries <- c(entries, 366)


do we need a

entries[entries == 0] <- 365

here?

dajmcdon · 2025-02-28T18:43:46Z

R/step_climate.R

+yday_leap <- function(time_value) {
+  ifelse(
+    !lubridate::leap_year(time_value),
+    lubridate::yday(time_value),
+    ifelse(
+      (lubridate::month(time_value) == 2) & lubridate::day(time_value) == 29,
+      999,
+      lubridate::yday(time_value) - (lubridate::month(time_value) > 2)
+    )
+  )
+}


Suggested change

yday_leap <- function(time_value) {

ifelse(

!lubridate::leap_year(time_value),

lubridate::yday(time_value),

ifelse(

(lubridate::month(time_value) == 2) & lubridate::day(time_value) == 29,

999,

lubridate::yday(time_value) - (lubridate::month(time_value) > 2)

)

)

}

leap_day <- function(x) lubridate::month(x) == 2 & lubridate::day(x) == 29

yday_leap <- function(time_value) {

dplyr::case_when(

!lubridate::leap_year(time_value) ~ lubridate::yday(time_value),

leap_day(time_value) ~ 999,

TRUE ~ lubridate::yday(time_value) - as.numeric(lubridate::month(time_value) > 2L)

)

}

Just a thought for clarity, avoiding nested ifelse() calls. ifelse() can be brittle. See the warning in ?ifelse().

dajmcdon · 2025-02-28T18:58:28Z

R/step_climate.R

+#' epiweek, but it assigns week 53 the value of -1 instead so it mirrors the assignments in yday_leap
+#' @keywords internal
+epiweek_leap <- function(time_value) {
+  lubridate::epiweek(time_value) %>% ifelse(. == 53, 999, .)


Piping and . can be hard to discern. Same below.

Suggested change

lubridate::epiweek(time_value) %>% ifelse(. == 53, 999, .)

w <- lubridate::epiweek(time_value)

w[w == 53] <- 999

w

dajmcdon · 2025-02-28T18:59:42Z

R/step_climate.R

+#' isoweek, but it assigns week 53 the value of -1 instead so it mirrors the assignments in yday_leap
+#' @keywords internal
+isoweek_leap <- function(time_value) {
+  lubridate::isoweek(time_value) %>% ifelse(. == 53, 999, .)


Suggested change

lubridate::isoweek(time_value) %>% ifelse(. == 53, 999, .)

w <- lubridate::isoweek(time_value)

w[w == 53] <- 999

w

dajmcdon · 2025-02-28T19:00:14Z

R/step_climate.R

+    )
+  )
+}
+#' epiweek, but it assigns week 53 the value of -1 instead so it mirrors the assignments in yday_leap


Do you mean this to say -1 rather than 999?

dsweber2 added 3 commits February 27, 2025 15:18

step_climate leap weeks/days

a5dea72

same for climatological_forecaster

b3db007

feb 29 is day 60, so the boundary is 59-60

fda832f

dsweber2 requested a review from dajmcdon as a code owner February 27, 2025 23:28

dsweber2 self-assigned this Feb 27, 2025

dajmcdon requested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leap time handling #445

Leap time handling #445

dsweber2 commented Feb 27, 2025

dajmcdon left a comment

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

dajmcdon Feb 28, 2025

	.idx = time_value %m-% ahead_period,
	# subtracts a month w/o rollover (usual behavior on weeks/days)
	.idx = time_value %m-% ahead_period,

	entries <- ((iter - window_size):(iter + window_size) %% modulus)
	entries <- (iter - window_size):(iter + window_size) %% modulus

	entries <- ((53 - window_size):(53 + window_size - 1) %% 52)
	entries <- (53 - window_size):(53 + window_size - 1) %% 52

-  lubridate::epiweek(time_value) %>% ifelse(. == 53, 999, .)
+  w <- lubridate::epiweek(time_value)
+  w[w == 53] <- 999
+  w

Leap time handling #445

Are you sure you want to change the base?

Leap time handling #445

Conversation

dsweber2 commented Feb 27, 2025

Checklist

Change explanations for reviewer

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

dajmcdon left a 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