-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix: update for compatibility with epiprocess==0.9.0 #386
Conversation
42397bf
to
93f4140
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main things are the change in the ordering of the keys. Is there a reason for this? Preference? Were they changed on the relevant epiprocess branch?
@@ -95,7 +95,7 @@ epi_recipe.epi_df <- | |||
keys <- key_colnames(x) # we know x is an epi_df | |||
|
|||
var_info <- tibble(variable = vars) | |||
key_roles <- c("geo_value", "time_value", rep("key", length(keys) - 2)) | |||
key_roles <- c("geo_value", rep("key", length(keys) - 2), "time_value") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to keep these in the same order as https://github.com/cmu-delphi/epiprocess/blob/16f38b2b386522f7f8f4880157432a3fa4a8d6ae/R/epi_df.R#L168.
Does this break something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is updated in our upcoming epiprocess PR. If this line isn't changed then the wrong columns are associated with the wrong roles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Is there a reason for that choice? I think the previous ordering was based on a Slack vote that @brookslogan took a while back? I can't seem to find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the Slack vote asked for people's preference for the order to add arguments to a function, not how they want to order their epi_df.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Right, and this was about API data-fetching functions, rather than epiprocess ones. In arrange_canonical I think we just borrowed this vote without doing another one... and I do wonder about my polling methodology anyway.)
I'm approving, subject to resolving the remaining bits. |
Co-authored-by: Daniel McDonald <[email protected]>
a044e7e
to
0e7d90f
Compare
Did a hotfix to the date validation code in layer_forecast_date and layer_target_date for now, but I can handle this better in another PR #390. |
Checklist
Please:
dajmcdon.
DESCRIPTION
andNEWS.md
.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).
(backwards-incompatible changes to the documented interface) are noted.
Collect the changes under the next release number (e.g. if you are on
0.7.2, then write your changes under the 0.8 heading).
epiprocess
version in theDESCRIPTION
file ifepiprocess
soonepipredict
andepiprocess
Change explanations for reviewer
Updating for epiprocess changes here cmu-delphi/epiprocess#477.
The main changes are:
as_epi_df(result)
and append the metadata. But the fix is simpleas_epi_df(result, as_of = old_meta$as_of, other_keys = old_meta$other_keys)
.additional_metadata = list(other_keys = ...)
in epi_df constructorsother_keys
can no longer be NULL in epi_df constructors, so a few%||% character()
addinskey_colnames
in epiprocess to returngeo_value, other_keys, time_value
(since that makes the most sense when grouping and ordering epi_dfs), so I propagated the change to the key_colnames here as much as I could.Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch