cmu-delphi / cmu-delphi/epipredict

Provide better interface or documentation for per-geo modeling

Open
#336 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
R
Stars
18
Forks
13
Avg merge
21d 58m
Merged PRs (30d)
1

Description

## Context

@dsweber2 was just noting that
```r
epi_df %>%
epi_slide(
~ .x %>%
group_by(geo_value) %>%
arx_forecaster(.....)
)
```
doesn't fit per-geo models; it actually just ignores the grouping altogether.

We suggested "transposing" the operations, but @rnayebi21 found that
```r
epi_df %>%
group_by(geo_value) %>%
epi_slide(~ arx_forecaster(.x, .....), .....)
```
doesn't work either; `.x` doesn't have the `geo_value` column thus lacks `epi_df`ness. I believe these problems also apply to when you are trying to do version-faithful backtesting with `epix_slide()`.

Workarounds seem a little bit of a pain, either
- fixing up the first approach by doing something like
- split + map + bind_rows, or
- group_by + group_split + map + bind_rows, or
- group_by + group_modify(.keep = TRUE), or
- group_by + reframe (using the deprecated-but-not-replaced cur_data_all()...)
- fixing up the second approach by reconstructing an `epi_df` inside the slide computation using `.x`, `.group_key`, and `.ref_time_value`, or
- [`mutate(geo_value2 = geo_value)` and group by that instead of `geo_value`. Or just `group_by(geo_value2 = geo_value)`.]

The first workaround seems more modular (you can have a list of forecasters that can all rely on ungrouped slides, rather than having to do a different type of slide call for each one).

## Proposal

1. Make `arx_forecaster()` check specifically if there's a missing `geo_value` and hint that if they were doing a grouped `epix_slide()` or `epi_slide()` with `geo_value` in the group variables, that won't work, and to do instead.
2. Check if input to `arx_forecaster()` etc. is grouped; if so, either
- warn
- abort
- fit & forecast one model per group
3. [Also check for groupedness and warn/abort in the epi workflow internals.]

## Musings

We can also probably make things easier epiprocess-side, by adding a `.keep` parameter if we're not already able to forward to `group_modify()` via dots. But I'm not sure we actually want to... this makes it easier to use `epi_slide()` for forecasting when it shouldn't actually be (`epix_slide()` should be favored and maybe renamed to make this clear).

[@dshemetov points out we should document this geo-grouped `epi_slide` gotcha in epiprocess. And actually fixing what's going wrong is part of a much larger project, epiprocess#223.]

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.