Skip to content

Pass ref_time_value to epix_slide for functions and formulas #313

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 20 commits into from
Jun 9, 2023

Conversation

nmdefries
Copy link
Contributor

@nmdefries nmdefries commented May 5, 2023

Addresses #170 for epix_slide computation for functions and formulas

  • comp_value calculation in comp_one_grp passes ref_time_value to f.
  • Define an extended version of rlang::as_function that creates functions with three args to support use of ref_time_value in formulas. Rename args in tests and examples to establish new standard.
  • Modify check_sufficient_f_args to take a # of args arg. Until epi_slide also passes ref_time_value to user computations, it and epix_slide will expect different numbers of arguments from and pass different numbers of arguments to user function f.
  • Change description of f in slide definitions to NOT say that any particular arg names are required when passing f computation as a function.

@nmdefries
Copy link
Contributor Author

Arg names still under discussion.

@nmdefries nmdefries marked this pull request as ready for review May 5, 2023 18:44
Base automatically changed from ndefries/useful-slide-arg-errors to dev May 18, 2023 23:37
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Looks really good overall! Thanks for the attention to clear docs, being careful about corrupting environments, and attribution. The attribution part I'm also not clear on, but I've added my thoughts/exploration above. Also I've suggested a few minor wording/naming/etc. tweaks.

@nmdefries nmdefries requested a review from brookslogan June 5, 2023 15:19
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Some final minor tweaks todo.

@brookslogan
Copy link
Contributor

Let's see if merging this will auto-adjust the base branches of the dependent PRs or if it's going to close them...

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