Skip to content

Generalize optimized epi_slide fn #433

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
merged 18 commits into from
Mar 27, 2024
Merged

Conversation

nmdefries
Copy link
Contributor

@nmdefries nmdefries commented Mar 25, 2024

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 epi_slide_mean more general. Let the user pass a data.table rolling fn (frollmean, frollsum, frollapply) or slider rolling function (slide_sum, etc) to expand optimized slide functionality.

@nmdefries nmdefries marked this pull request as draft March 25, 2024 17:46
@nmdefries nmdefries changed the base branch from dev to ndefries/specialized-slide-mean March 25, 2024 17:51
@nmdefries nmdefries force-pushed the ndefries/general-optimized-slide branch from f4ff336 to 4f4a67a Compare March 25, 2024 22:18
@nmdefries nmdefries force-pushed the ndefries/specialized-slide-mean branch from 237fc38 to 0125bee Compare March 26, 2024 17:27
Base automatically changed from ndefries/specialized-slide-mean to dev March 26, 2024 17:34
@nmdefries nmdefries force-pushed the ndefries/general-optimized-slide branch from 9fd90e7 to ddc1934 Compare March 26, 2024 22:02
@nmdefries nmdefries marked this pull request as ready for review March 26, 2024 22:08
@nmdefries nmdefries force-pushed the ndefries/general-optimized-slide branch from 6e6d198 to 1385d59 Compare March 26, 2024 22:14
@nmdefries nmdefries requested a review from dsweber2 March 26, 2024 22:42
Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than this bit with slider seems good

@dsweber2
Copy link
Contributor

Looks like the linter and styler disagree about the spacing for this:

  if (inherits(x$time_value, c("yearquarter", "yearweek", "yearmonth")) ||
    is.numeric(x$time_value)) {

so I guess the linter is going to fail indefinitely. Disappointing and weird.

Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we wait for Logan to get back? Wouldn't put it past him to have some arcane solution to the identical vs all.equal problems. I'd be fine with leaving that as an open issue for a potential future feature though. Actually using anons is mostly covered by passing args.

@nmdefries
Copy link
Contributor Author

David opened an issue RE styler here

Will have Logan take a look at this when he gets back, but if it needs changes, I'll do those in a separate PR. I don't want this to be hanging as a PR for a long time.

@nmdefries nmdefries merged commit 00c0a85 into dev Mar 27, 2024
@nmdefries nmdefries deleted the ndefries/general-optimized-slide branch March 27, 2024 18:05
@nmdefries
Copy link
Contributor Author

  • demonstrate use of new functions in vignettes

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.

2 participants