Make epix_slide more like group_modify
#311
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`epix_slide` already has summarize/reframe-like column behavior: it outputs the grouping columns + columns from the user computations. Make it also have `reframe`-like row behavior: * Accept any number of elements/rows out of user computations (instead of requiring either 1 or a particular number derived from an inferred/assumed "computation effective key"). * Do not recycle length-1 user computation outputs to the "expected" length/nrow.
Previously, broadcasting in `epix_slide` would remove any rows filled in by `.drop=FALSE`, making the `.drop` setting more or less useless. Now that `epix_slide` does not broadcast, update docs&tests accordingly. Additionally, add some missing validation for the `.drop` parameter. Other changes: * Tweak code using `setDT` to do it in a pipe to read better (avoid a line setting an `out_DT` variable to a non-DT value, even temporarily). * Format some roxygen comments.
This was referenced May 4, 2023
|
While we decide on when we should output |
…bble.epi_df` To make `epix_slide` more like `reframe` and to make `as_tibble` on `epi_df`s act more like other tibble subclasses.
|
brookslogan
changed the title
epix_slide more like reframeepix_slide more like group_modify
rather than `reframe`. This way we can match the grouping/ungroupedness just like in `epi_slide`, except for the zero-variable groupings not supported by `grouped_df`.
Remove the backticks around the parameter names, which don't belong and were caught by `check()`.
nmdefries
requested changes
May 12, 2023
not the reasons for deprecation.
Add test for `epix_slide` on function or tidyeval with `all_versions=FALSE`, and for formula or tidyeval with `all_versions=TRUE`.
Document behavior when `all_rows = TRUE` and `as_list_col = TRUE`; this behavior might be different from before move to use `vctrs` though. Add tests to cover this case + others, which was caught only via a vignette build failing.
|
Think this is now pretty much fixed up. Some remaining minor TODOs:
|
dajmcdon
approved these changes
May 29, 2023
|
Checked with nmdefries on this and think it's ready to merge. |
This was referenced Jul 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replacement for #290 using branch from the correct remote.