Skip to content

Lcb/slide unnest dedupe cols #509

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

Merged

Conversation

brookslogan
Copy link
Contributor

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

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

  • Resolves #{issue number}

@brookslogan brookslogan force-pushed the lcb/slide-unnest-dedupe-cols branch 2 times, most recently from 74241a8 to ab0c50a Compare August 21, 2024 10:36
@brookslogan brookslogan changed the base branch from dev to lcb/slide-improvements-2024-06 August 21, 2024 20:51
@dshemetov dshemetov self-requested a review August 21, 2024 21:16
@brookslogan brookslogan force-pushed the lcb/slide-unnest-dedupe-cols branch 2 times, most recently from 86f8342 to d759628 Compare August 22, 2024 07:38
brookslogan and others added 9 commits August 26, 2024 12:49
…ames

In `epix_slide()`:

- warn-deprecate `.ref_time_values =` in favor of `.versions =`
- allow tidyeval or formula comps to use `.ref_time_value` or `.version` to
  access the ref_time_value/version (currently, these two things are always the
  same)
- output a `version` column, not a `time_value` column
- rename `epix_slide_ref_time_values_default` -> `epix_slide_versions_default`
- some other cleanup from a rebase combining with dot-prefixing and other slide changes
- Default is now to not mark any versions as clobberable; simply remove
  discussion of old default as it was to explain a surprise/annoyance in normal
  use.
- Favor using `$versions_end` to get the latest version; while in examples it's
  probably similar, in general, it's more "correct" and should be faster.
since this seems like more appropriate and consistent naming for the main use
case of extracting an `epi_df` snapshot.
Collapse with empty string in order to not have extra whitespace if used with
`cat` rather than `cli_*`.
Forbidding `new_col_name` being among the labeling columns addresses some dedupe
cases where deduping would always lead to failure except for
completely-redundant computations (that only output computation labels rather
than and actual computation).

- This might not be complete in a edge case where `"slide_value"` is a grouping
variable. (E.g., from using a slide to assign a categorical trend, then doing a
grouped slide based on the trend.)

This is definitely only part of the dedupe handling.  Unpacked-column outputs
need to actually be de-duped.

Also, fix incorrect documentation for time_value filter for .all_versions = TRUE
while rebasing on other slide updates.
@brookslogan brookslogan force-pushed the lcb/slide-unnest-dedupe-cols branch from ce5b91f to 9127952 Compare August 26, 2024 20:46
brookslogan and others added 2 commits August 26, 2024 20:48
Since we're passing along ... from outer fns to our inner helper fns taking ...,
the internal fns should also dot-prefix if outer should.
@brookslogan brookslogan force-pushed the lcb/slide-unnest-dedupe-cols branch 2 times, most recently from 2eabcc0 to 1181b97 Compare September 3, 2024 21:57
@brookslogan brookslogan merged commit 1181b97 into lcb/slide-improvements-2024-06 Sep 4, 2024
2 checks passed
@brookslogan
Copy link
Contributor Author

brookslogan commented Sep 10, 2024

[Apparently I merged this already somehow? And we thought this was a draft but somehow wasn't? Which explains why I didn't see it and needed to open #521 and thought I had somehow pushed some stuff directly to the slide improvements mega-PR... Removing redundant checklist here.]

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.

1 participant