cmu-delphi / epiprocess

Tools for basic signal processing in epidemiology
https://cmu-delphi.github.io/epiprocess/
Other
13 stars 9 forks source link

`epi_slide*` no column behavior #475

Open dsweber2 opened 3 months ago

dsweber2 commented 3 months ago

When I don't specify a column, epi_slide and epi_slide_mean have pretty different behavior. Example command epi_slide_mean(before = 6L) or epi_slide(mean, before = 6L).

epi_slide_mean outputs a Can't recycle input of size 0 to size 1 error, while epi_slide just returns a ton of NA values.

Ideally they would either

  1. assume the correct target (in the case of 1 non-key column there's an obvious choice)
  2. throw an error saying we need to specify a target (probably the right behavior if there's 2+ columns)

edit: second epi_slide_mean should've been just epi_slide

brookslogan commented 3 months ago

Couple notes:

We can add additional validation/defaults for the epi_slide_mean() case. To stop the epi_slide() case, we'd probably need to turn that warning into an error.

dsweber2 commented 3 months ago

Ah, I guess I was a bit confused about epi_slide working after including the column name. The lack of parallel is unfortunate; it would be nice if epi_slide accepted this sort of arg that switched it to operating on the specific column.

brookslogan commented 3 months ago

The tidyverse way would be to use across(). But that may be in the group of things that really delve into dplyr internals and aren't compatible with (simple usage of) the rlang analogues (that might not even back dplyr ops despite sounding that way??), so allowing across() in epi_slide() might not be simple.

brookslogan commented 1 month ago

In #521 I'm just going for

throw an error saying we need to specify a target

in all cases, plus hinting them to use epi_slide_mean etc. We could reconsider whether we want to do some sort of special behavior when there's only 1 sensible value column, or maybe doing something for every value column if they are all sensible. But I'm just trying to make sure we have a hard & helpful error as a starting point.

brookslogan commented 1 month ago

521 lacks the epi_slide_mean fix [0 cols being fed into epi_slide_mean. The epi_slide part is "addressed" with an error message.]