446 promote other_keys as an argument to as_epi_df()

[dajmcdon]

Checklist

Please:

• Make sure this PR is against "dev", not "main" (unless this is a release
  PR).
• Request a review from one of the current main reviewers:
  brookslogan, nmdefries.
• Makes sure to bump the version number in DESCRIPTION. 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
  1.7.2, then write your changes under the 1.8 heading).

Change explanations for reviewer

• Promotes other_keys and deprecates additional_metadata as arguments to as_epi_df()
• One can still populate additional_metadata with attr(epi_df, "additional_metadata") <- list(...).
• Nothing is broken internally, but this will likely break some {epipredict} vignettes and the tooling book.

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

• Resolves [enh] promote other_keys  #446

[brookslogan]

Thanks for taking this on! A few haphazard suggestions & a question about subclass.

[dajmcdon]

• I'm removing the subclass options.
• On the other_keys = character(0): @brookslogan should the default be NULL or character(0)?

[brookslogan]

On the other_keys = character(0): @brookslogan should the default be NULL or character(0)?

I'd favor character(0); it seems cleaner to reason about type/class-wise. If it needs to be NULL for some reason we might need to regenerate the example data structures.

[and if you mean just what the default arg should be rather than what attr value it maps to, I think tidy style guide suggests = NULL as a placeholder only when the default is a more complicated thing.]

[dajmcdon]

The downside of character() is that assigning metadata$other_keys <- character() means that this component will always exist, where as metadata$other_keys <- NULL gives a list without the other_keys component. So I think the options are:

1. Default character(); other_keys is always present in metadata. Printing could ignore or always print the empty field.
2. Default character(); internals process it away so that other_keys is not present in metadata when there aren't any.
3. Default NULL; other_keys not present in metadata when none are specified. Requires less internal control flow than 2 to implement.

It sounds like you prefer 1 or 2?

[brookslogan]

I prefer 1. over 3. because 3. required checking how NULL behaved with every function we used (it seemed to act right for most/all of our uses, but so would character(0)), seemed to me like it actually required more control flow (though maybe not with checkmate), and complicated the "type" of other_keys. 2. doesn't make sense to me, though I guess having multiple ways to say zero other_keys also doesn't sound great.

[dajmcdon]

On 2., maybe I should be more explicit. Something like:

as_epi_df <- function(..., other_keys <- character(0), ... ) {
  ...
  metadata$other_keys <- if (length(other_keys)) other_keys else NULL
}

In this case, no other_keys slot will be populated in the metadata list.

And you would need similar handling in print() to avoid lines like * other_keys: .

[brookslogan]

Yeah, for 2., that's what I was thinking of. I think we had 2. 3. up to some point, and I changed it, I think maybe for a concrete reason, but I'm not sure. Could try to track it down. Here's where we made the switch from 3. to 1. Perhaps this was one of the cases where NULL doesn't act like character(0) automatically.

Is the concern about keeping it in the metadata just about RAM space or mindspace? On the mindspace-part, I feel it's a bit easier to think about than possibly-NULLness.

[dshemetov]

Playing around a bit, confused why I can't see other_keys in jhu_csse_daily_subset:

r$> attributes(jhu_csse_daily_subset)$metadata
$geo_type
[1] "state"

$time_type
[1] "day"

$as_of
[1] "2024-01-26 09:27:32 PST"

UPDATE: Ah, I see, the dataset needs to be regenerated. Tricky.

r$> x <- jhu_csse_daily_subset %>% tibble %>% as_epi_df

r$> attributes(x)$metadata
$geo_type
[1] "state"

$time_type
[1] "day"

$as_of
[1] "2024-01-26 09:27:32 PST"

$other_keys
character(0)

[brookslogan]

I think this might have been a missed chore from #390? Somehow I thought we already noticed it and regenerated, but guess not.