[Issue 182] Update epi_df examples, check additional_metadata type at construction

[mgyliu]

Addresses #182

Changes:

• Update new_epi_df examples so that (i) additional_metadata argument is type list instead of vector and (ii) there is an example with multiple other_keys
• Add a check in new_epi_df to ensure additional_metadata is a list
• Write a test for the previous change

I checked in epipredict for calls to as_epi_df and all additional_metadata arguments there are lists so this change will not break epipredict. Afaik there aren't any dependencies on epiprocess other than epipredict.

[dajmcdon]

Everything here looks good.

As I read this though, I think the epi_df and as_epi_df documentation should be more specific. A tbl_ts is converted so that additional keys go as other_keys = xxx. I think we should mention this somewhere.

If someone wants to make an epi_df to use with epipredict that correctly uses those keys, they MUST be called other_keys or they'll be ignored.

[brookslogan]

If other_keys settings are causing issues, we should consider printing them in print.epi_df rather than having the user have to separately print the metadata to see. [Or even "promote" it to be its own parameter in the new&as functions, rather than in additional_metadata? Maybe a separate issue.]

[mgyliu]

@dajmcdon Good suggestions - I'm thinking of adding a blurb in the additional_metadata section under "Arguments" that talks about other_keys. The as_epi_df documentation does already state those things about tbl_ts conversion, under "Methods by class". Perhaps the wording isn't clear enough? What do you think?

If someone wants to make an epi_df to use with epipredict that correctly uses those keys, they MUST be called other_keys or they'll be ignored.

^ To be clear, we would just add some documentation to make this clear to the user? i.e., not "strictly" enforce it in the "throw an error if the specification is wrong" sense?

I do agree with @brookslogan's suggestion of having other_keys as its own argument in those functions. It seems important enough to not be considered "metadata" at all? For this PR though, I'll add the extra documentation and open an issue for Logan's suggestions.

[dajmcdon]

I agree that as_epi_df() would "get it right" when converting from a tbl_ts. But the documentation for as_epi_df doesn't say it very explicitly.

So, if I call as_epi_df() on a regular tibble that has some extra keys, the docs don't tell me what to do with that. It should say something like: "if your tibble has extra keys, be sure to add them as a character vector to the other_keys component of the additional_metadata list or bad things could happen". The same goes for the docs on new_epi_df().

[mgyliu]

@dajmcdon Gotcha. I just pushed up changes to the as_epi_df and new_epi_df documentation to explain this under the additional_metadata argument. Also updated the epiprocess vignette with an explanation. Let me know what you think!

[dajmcdon]

Looks good to me. @brookslogan?

[brookslogan]

Requests/questions [actually these may just be separate issues as well --- if we put them in other Issues then think this is good to go]:

• I'm afraid new_epi_df vs. as_epi_df might not be clear from the roxygen docs. Perhaps the description of one or both could include a compare&contrast or rationale? (I think the most important thing to note here is the behavior on epi_dfs --- that as_epi_df will return an epi_df exactly as is, keeping the current column order and ignoring any metadata arguments, while new_epi_df will order geo_value and time_value first, and reset/overwrite --- most --- metadata.) E.g., like in ?rlang::new_quosure / ?rlang::as_quosure, tibble::new_tibble, dplyr::new_grouped_df, etc. moved to Document the differences for column re-ordering in new_epi_df and as_epi_df #166.
• Is as_of the only metadata field that new_epi_df will preserve when called on an epi_df? Do we want it that way? --> Document the differences for column re-ordering in new_epi_df and as_epi_df #166
• If the docs for all of the common parameters of these two functions are the same, use @inheritParams <otherfn> in one of them to not have to copy-paste all the roxygen updates. (If they're not the exact same, there are more tedious ways to reduce duplication, but maybe that could be a different issue.) --> Document the differences for column re-ordering in new_epi_df and as_epi_df #166

A few lingering thoughts which probably belong in separate issues:

• Unfortunately, I don't think we ever explained what "keys" were in the context of an epi_df, and googling "tibble key" is surprisingly unhelpful (currently giving links to lock picks and decoder tools). Maybe explaining this could just be rolled into the issue for adding other_keys as a parameter, though; it'd be easier to do in its own @param. --> "Promote" other_keys to be printed, a constructor parameter, and more clearly documented #186
• ex3 requiring conversion away from an epi_df in order not ignore other_keys and all the other parameters seems error-prone. There is precedent with rlang::{new,as}_quosure, but still. Wondering what we should do here; maybe something like calling Abort if the user passes in metadata that doesn't match the pre-existing metadata. --> Document the differences for column re-ordering in new_epi_df and as_epi_df #166

[brookslogan]

Migrating the notes above to other issues, so this is good to go!

[brookslogan]

@mgyliu should we now close #182 as completed?

I've copied/moved some of the notes above from here and in #121 into #166 and #186.

[mgyliu]

Thanks @brookslogan! Yes I'll close 182.