Skip to content

Conversation

@brookslogan
Copy link
Contributor

@brookslogan brookslogan commented Jan 10, 2025

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).
  • See DEVELOPMENT.md for more information on the development
    process.

Change explanations for reviewer

  • Make key_colnames.epi_archive output its unique key colnames (including version).
  • Make key_colnames.data.frame require other_keys to be passed.
  • Remove key_colnames.default.
  • Require exclude = to be passed by name in key_colnames.
  • Require dots to be empty in key_colnames.
    • Note: this triggers some errors in epipredict tests. See below.

epipredict errors

Originally, I misread and thought these were from passing other_keys = hopefully-redundantly into key_colnames.epi_df and having them be ignored. I was planning to do something like

  expected_other_keys <- attr(x, "metadata")$other_keys
  if (is.null(other_keys)) {
    other_keys <- expected_other_keys
  } else {
    if (!identical(other_keys, expected_other_keys)) {
      cli_abort(c(
        "`other_keys` was provided, but didn't match expectations from inspecting `x`",
        "*" = "`other_keys` was {format_chr_with_quotes(other_keys)}",
        "*" = "`expected_other_keys` was {format_chr_with_quotes(expected_other_keys)}",
        "i" = "If you resolve this discrepancy by adjusting the metadata of `x`, you
               shouldn't have to pass `other_keys =` here anymore unless you want to
               continue to perform this check."
      ))
    }
  }

However, the error is actually from passing extra_keys = (which was also previously ignored, but it sounds like it has a different meaning). I am looking a bit more into this.

  • todo: decide what to do re. extra_keys =
    • idea for now: forbid + PR to epipredict to specify other_keys = instead. If current behavior of other_keys = with epi_dfs breaks later, then decide whether to change things here or adjust any offending steps/layers.
  • todo: check whether epipredict#410 is caught... requiring other_keys = in key_colnames.data.frame should be flagging this, right?
    • It is. Also need to include fix in epipredict PR before merging this.
  • todo: prepare epipredict PR and get it merged

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

* Make `key_colnames.epi_archive` output epikey-time-version rather than just
  epikey-time.
* Make `key_colnames.data.frame` require `other_keys` be provided.
* Remove `key_colnames.default`.
* Make `key_colnames` forbid passing `exclude` positionally.
* Update downstream `revision_summary`.
@brookslogan brookslogan requested a review from dajmcdon January 10, 2025 19:39
@brookslogan
Copy link
Contributor Author

brookslogan commented Jan 10, 2025

  • don't actually merge this into current target branch; target branch is just for a more limited diff. Let Fix age group rate aggregation example #591 merge into dev first. then either
    • change base to dev, get Update revision_summary #540 merged into this, combine titles & changelog
    • change base to lcb/key_colnames-revision_summary-age_agg-updates and open PR here to hold combined changelog (although it shouldn't actually list the age agg updates)

Base automatically changed from lcb/fix-age-group-rate-agg-example to lcb/key_colnames-revision_summary-age_agg-updates January 24, 2025 07:35
@brookslogan brookslogan changed the base branch from lcb/key_colnames-revision_summary-age_agg-updates to dev January 24, 2025 07:35
@brookslogan brookslogan changed the base branch from dev to lcb/key_colnames-revision_summary-age_agg-updates January 24, 2025 07:36
@brookslogan brookslogan merged commit 16e1f46 into lcb/key_colnames-revision_summary-age_agg-updates Jan 24, 2025
@brookslogan brookslogan deleted the lcb/rework-key_colnames branch January 24, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants