Skip to content

Make epix_slide more like reframe #290

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

Conversation

brookslogan
Copy link
Contributor

Resolves #275, addresses _pkgdown.yml typo, adds GitHub Actions checks to dev branch PRs, implements current approach to #213, adds missing test-grouped_epi_archive.R file.

`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.
@brookslogan brookslogan requested a review from dajmcdon as a code owner March 28, 2023 19:56
@brookslogan brookslogan changed the base branch from main to dev March 28, 2023 19:56
@brookslogan brookslogan marked this pull request as draft March 28, 2023 19:58
@brookslogan
Copy link
Contributor Author

brookslogan commented Mar 28, 2023

  • The last point in Change epix_slide to be more group_modify-like #275 still needs to be checked. [punted this decision and always output ungrouped tibble for now.]
  • NEWS.md needs to be updated.
  • Fix DESCRIPTION version to be 0.6.0.9999
  • Resolve check failures
  • Fix not getting epi_dfs out of some non-all-versions (at least ungrouped ones; check also on grouping by geo) [punted]
  • Fix documentation/implementation in light of reframe always outputting an ungrouped object.
  • Clean up branch issues from accidentally PR-ing off of fork
  • Debug as_list_col = TRUE actually outputting an edf/tibble col bundle

@brookslogan
Copy link
Contributor Author

Sometimes we can get an epi_df out of epix_slide as I believe was originally intended. But not very elegantly:

  • We don't output epi_df when ungrouped
  • We output epi_df when grouped by geo_value, but if slide computation was trying to output an epi_df or add a key column, it's ignored/corrupted by default, or raises errors with names_sep=NULL.

And if we want an epi_df out of epix_slide with time_values from the slide computations, not the ref_time_values, we may be stuck trying to wrap epi_dfs in our computation inside a list column of a tibble output, then attempt to unnest and deal with naming conflicts simultaneously, which may not be possible, so we may have to rename & lose any outer epi_df-ness, then unnest, then add an other_keys entry for whatever the ref_time_values label was. Or, alternatively, we don't wrap, do the rename shuffling, then have to reclass manually and reset metadata, some of which would have been discarded in intermediate steps (which means hardcoding or breaking the pipe to assign to a temporary to allow later access; both awkward).

I'm not going to try to make this easier, because it again raises questions of whether we really do want an epi_df out sometimes, or if we want something else. Perhaps we should only output a tibble like with all_versions=TRUE, or consider alternatives such as another epi_archive or a generalized archive. And revisit #163.

@brookslogan
Copy link
Contributor Author

This PR is based off of a branch from the wrong remote. It is replaced by #311.

@brookslogan brookslogan closed this May 4, 2023
@brookslogan brookslogan deleted the lcb/make_epix_slide_more_like_reframe branch May 5, 2023 10:09
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.

Change epix_slide to be more group_modify-like
2 participants