291 date period

[dajmcdon]

Checklist

Please:

• Make sure this PR is against "dev", not "main".
• Request a review from one of the current epipredict main reviewers:
  dajmcdon.
• Makes 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).

Change explanations for reviewer

This should allow for weekly and annual forecasts.

• Ahead/lag always operate on the time_type of the epi_df. But forecast_date / target_date always were dates. These can clash in non-daily forecast tasks.
• We now validate these against the time_type of the epi_df. This should now work naturally. There's a test case for annual forecasts in the panel-data vignette in Panel Data Vignette (Issue 99) #115 .
• While here, there were a few minor things I fixed:
  • prefix head and tail with utils:: to avoid R CMD Check notes.
  • switch a few rlang::abort() to cli::cli_abort()
  • layer_population_scaling() was producing messages because the by argument could be empty
  • Note the tolower() processing that is commented out
  • lag/ahead are coerced to integer silently
  • In some tests, prep() needed to have the training data.

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

• Resolves Date processing mismatch #291

[dajmcdon]

I think this is wrong, but I'm not yet certain that it's safe to remove.

[dsweber2]

doing some timetravel, it looks like it's been here in one form or another forever. What's confusing to me is that this looks like it does nothing. Was the point to make all the characters columns lowercase (such as geo_value) to match correctly maybe, but there was a parenthesis problem? Was dplyr::mutate(dplyr::across(tidyselect::where(is.character)), tolower) meant?

[dajmcdon]

Yeah, I'm not sure why it was there in the first place. I suspect that the included state population dataset used capitals while typical usage from the API gives geos in lower case. But that shouldn't have resulted in hardcoded workarounds here (that are prone to failure).

So I think this should go forever, but I wanted to be sure that if some downstream use errored out, I could find this and try to track it more carefully.

[dsweber2]

Generally looks good to me. Might want to slightly restructure the validate_date error to reduce on redundancy and improve readability, but that's not really a huge change.

I tried to clarify some of the tests a little bit, since I was struggling to follow them, but its nothing essential.
If I were to suggest more tests, it would be a for loop over pairs of types for layer_add_forecast_date() and latest, with the expectation that all but the matching ones would fail. Probably overkill.

I think it makes sense to merge this before #296, as that also involves date types and working with the ahead.

[dsweber2]

doing some timetravel, it looks like it's been here in one form or another forever. What's confusing to me is that this looks like it does nothing. Was the point to make all the characters columns lowercase (such as geo_value) to match correctly maybe, but there was a parenthesis problem? Was dplyr::mutate(dplyr::across(tidyselect::where(is.character)), tolower) meant?

[dsweber2]

Is this ok'd so we can add months? e.g. "May"?

[dajmcdon]

It's not that this is ok, but rather that the layer_add_*() function can no longer validate the date format immediately. It has to happen later.

[dshemetov]

Read most of the code, but didn't have enough context to fully follow it. Dropped a few thoughts for now, hopefully they're helpful and not distracting.

[dshemetov]

Does having this go from max_time_value + ahead to forecast_date + ahead cause this to function to behave differently? If yes and testing this PR becomes tough, then we could reduce scope here and punt these changes to another PR.

[dshemetov]

Separate question: how do the units in ahead change depending on the time type? Is this primarily up to the user to make sure they specify aheads in the right units?

[dajmcdon]

On max_time_value + ahead, this was the default previously, and remains the fall back (see lines below). However, if there is a forecast_date, then I think the default for target_date should be to use the specified forecast_date first.

This PR is meant to allow for ahead to always be an integer. Then it should be in units relative to the time_type of the epi_df. It took a lot of effort to make that happen though.