`group_by()` for `epi_archive` objects

[ryantibs]

Should we implement group_by() as public method in the epi_archive object? We could do that since it's an R6 object, and when group_by() is called, we could just have it set a private field called group_keys to whatever variables are passed.

The only downstream behavior this would affect is the as_of() and slide() methods for the epi_archive object. (These are its own public methods.) This would either return a group epi_df snapshot, or do a grouped sliding computation, respectively.

Pros:

• Would allow you for an epi_archive object x to do:

   x %>% group_by(geo_value) %>% epix_slide(col = my_fun(...))
  

  to do a sliding computation grouped by geo_value. This would be precisely analogous to how we would do it on an epi_df object with epi_slide(). Currently we have to set the grouping variables as an argument to epix_slide().

Cons:

• After calling epix_slide(), the object x above would still be grouped. This is because R6 objects obey reference semantics. So we'd have to ungroup() it, which may be a bit counterintuitive since it's not how dplyr verbs work on data frames or tibbles.

@lcbrooks @dajmcdon @jacobbien What do you guys think?

[dajmcdon]

I feel like the Pro is pretty major, and the Con is pretty minor. But maybe I'm not representative.

My potential confusion:

For dplyr, group_by() followed by summarize() results in an ungrouped object while it remains grouped after mutate(). It feels to me that _slide() is more like mutate() then summarize(), so maybe leaving it grouped isn't so odd? (With the caveat that all of this involves assignment to a result).

On the other hand, the grouping/not behaviour in dplyr is one of those things that I often forget about, and I find myself erroneously assuming something is ungrouped. This has happened many times, so maybe I'm an outlier in terms of actually absorbing the dplyr logic.

[brookslogan]

I think the drawback Ryan is pointing out is not that we need to group(....) %>% epix_slide(.....) %>% ungroup(....) to get no groups, but rather, is this situation:

x = <ungrouped epi_archive>
z1 = x %>% epix_slide(<stuff1>)
y = x %>% group_by(grpvar) %>% epix_slide(<stuff2>)
z2 = x %>% epix_slide(<stuff1>) # different from z1!

epi_slide is like mutate, but epix_slide is more like summarize [except it still partially broadcasts]; while the former only adds columns, the latter will be dropping nonkey nongroup columns + outputting a different class result (epi_archive --> epi_df). I believe that, until recently, summarize left things grouped anyway or wouldn't drop them completely; then it moved to .groups="drop_last" + a message by default; now it seems to be .groups="drop_last" + no message by default. So users should be used to worrying about what happens to their groups... but maybe we could provide a .groups option with a noisy default as well?

[This con] would be [re]solved by moving away from R6+data.table for epi_archive and instead using list+lazy_dt, or having epi_archive be a non-reference-semantics wrapper on top of an R6 EpiArchive backend. We decided this move was low priority though.

[brookslogan]

Addressing/ameliorating the Con:

• Higher effort: move to list + lazy_dt
• Medium effort: group_by should produce a "grouped_epi_archive" object, which records the groups + a pointer back to the (unmodified) epi_archive
• Low effort: as Ryan described, but have first step of epix_slide be to record the groups, then (side-effectfully) removing the group setting from the epi_archive. The con is now if someone did grouped_x = group_by(x, grpvar); grouped_x %>% epix_slide(<stuff1>); grouped_x %>% epix_slide(<stuff1>) they'd get different answers. Based on how I've used group_by and summarize in the past, I think this would be less likely to encounter compared to situation I noted in the post above [.... but I'm not sure on second thought]. (((We could prevent this by warning or stopping if there is no group setting in epix_slide. (To use no groups they would have to do x %>% group_by() %>% epix_slide(.....) or something like that.))))

[dajmcdon]

Oh, I see. Thanks Logan!

[ryantibs]

@lcbrooks @dajmcdon What is your current thinking on this?

Based on our conversation yesterday, it seems like we want to abide by the rule:

Functions applied to R6 objects should not be side-effectful. Only public member functions should be side-effectful.

So that means that we could go with something like Logan's "medium" solution: group_by() should clone everything in the epi_archive object EXCEPT the underlying data.table, and return it as an grouped_epi_archive object.

Are we OK with the fact that we don't clone the underlying data.table? I think so, provided we record this very clearly in the documentation. Plus, we could think of this as not an "R6 special case handling", but a "data table special case handling". Data tables are not cloned, they are typically handled in memory.

Thoughts?

[brookslogan]

Thinking about this clone-based "medium" solution:

Just regular archive$clone() might work; I think it's shallow by default (and don't know whether the deep clone feature actually recognizes data.tables and does the special copy operation that would be required). Eventually we may want to move to the "higher effort" solution... we might want to document that while currently, it will be pointing to the same DT as the original, this might change in the future to use something that will copy on write, so users shouldn't rely on this pointer behavior?

There's a bit of complication due to the combination of S3 and R6. A couple of decisions to make:

• Does group_by output S3 class c("grouped_epi_archive","R6"), c("grouped_epi_archive", "epi_archive","R6"), or c("epi_archive","R6")?
  • A: The first one requires writing an implementation for every S3 method that epi_archive implements and forwarding to the epi_archive implementation. If users write an S3 implementation for an epi_archive they will need to also write one for grouped_epi_archives. But we will also not accidentally automatically get non-grouped behavior for any methods that we forgot to write special grouped implementations for.
  • B: The second one has this behavior automatically and matches dplyr's approach, but will have an S3 triple class which might not be normal for R6 classes, which could be confusing
  • C: Similar to B, but could be inappropriate if there are S3 functions that should only apply to grouped epi archives
• Does group_by output R6 class grouped_epi_archive or epi_archive (with grouping support built into epi_archive)?
  • 1: the first one requires thinking about / dealing with R6 inheritance
  • 2: the second one might be inappropriate if there are R6 functions that should only apply to grouped epi archives

Top contenders are

• A1: S3 class is c("grouped_epi_archive","R6"), R6 class is "grouped_epi_archive"
• B1: S3 class is c("grouped_epi_archive","epi_archive","R6"), R6 class is "grouped_epi_archive"
• C2: S3 class is c("epi_archive","R6"), R6 class is "epi_archive"

A1 involves more S3 implementations, but could also catch some issues when we don't want to directly inherit epi_archive behavior. B1 is closest to dplyr, but might or might not confuse R6 users with the triple class. C2 might be a little confusing with group methods listed for ungrouped archives.

I can't see a clear winner here. Maybe A1 or B1 over C2.

[ryantibs]

@lcbrooks Thanks Logan for the super detailed proposal and analysis!

I'm in favor of B1. The only negative you point out is that it has a triple class, but I don't really see this as a negative to be honest.

Tagging @dajmcdon to see if he has any further thoughts, but barring that, I think to get the ball rolling, you could go ahead and implement this. Seems like it could be a good idea to address #67 in the same PR since it's a related issue.

[brookslogan]

Returning to this. I was trying to write a justification for B1 in a comment, and in doing so convinced myself to switch to A1, because looking at, e.g., the backcasting preprocessors and compactify, I think we're going to want to force ourselves&users be precise about when we're dealing with an ungrouped vs. a grouped epi_archive. And the overhead is comparable to B1, because with B1, we still need to check every inherited function to make sure it's compatible; it's just that if we forget to implement a method in A1, we get errors about unimplemented methods, which can be worked around, and if we forget to specialize/disable in B1, we get invalid results. I'm also adding a safeguard against re-grouping a grouped epi_archive. The epipredict development should hopefully give us an idea of how this all works out.