Skip to content

Document the differences for column re-ordering in new_epi_df and as_epi_df #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rachlobay opened this issue Jul 26, 2022 · 1 comment
Labels
documentation Improvements or additions to documentation P1 medium priority

Comments

@rachlobay
Copy link
Collaborator

It probably would be good to document the differences for column re-ordering innew_epi_df and as_epi_df. For ex., new_epi_df always has column re-ordering so that the columns are geo_value, time_value ... . In contrast, as_epi_df does not appear to enforce this column re-ordering when we're already feeding an epi_df to it (it simply returns the epi_df object unchanged). But it does enforce the column re-ordering if we are converting a tbl_df, data frame, or tbl_ts to an epi_df (as it ultimately calls new_epi_df).

@rachlobay rachlobay added the documentation Improvements or additions to documentation label Jul 26, 2022
@brookslogan
Copy link
Contributor

Copying in some notes from #183 which seem to belong better here, and proposing that we change this from just documenting difference to including a few tweaks to make the differences easier to think about (assuming that it doesn't break anything):

  • 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.
  • 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?
  • 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.)
  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation P1 medium priority
Projects
None yet
Development

No branches or pull requests

2 participants