Skip to content

Allow < 3 args in both epi_slide and epix_slide #478

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

Open
dsweber2 opened this issue Jun 26, 2024 · 3 comments
Open

Allow < 3 args in both epi_slide and epix_slide #478

dsweber2 opened this issue Jun 26, 2024 · 3 comments

Comments

@dsweber2
Copy link
Contributor

Forking off from #475 because it's a separate idea.
Looking at this warning and the code generating it a little more carefully

Error in `assert_sufficient_f_args()` at �]8;line = 309:col = 5;file:delphi/epiprocess/R/utils.R�epiprocess/R/utils.R:309:5�]8;;�:
! `f` might not have enough positional arguments before its
  `...`; in the current `epi[x]_slide` call, the group key and
  reference time value will be included in `f`'s `...`; if `f`
  doesn't expect those arguments, it may produce confusing error
  messages

This is about what inputs the function should expect. I definitely don't think this should be an error, and we should probably just not pass those args to the function if it isn't expecting them. There are plenty of cases where you don't actually need to pass the keys or the ref_time_value, which came up in the discussion yesterday. Perhaps there are cases where that's actually an error, but I think we should probably trust the user to figure that out; it should be pretty obvious to them if they're actually trying to get the keys.

@dsweber2 dsweber2 changed the title Allow < 3 args more explicitly in both epi_slide and epix_slide Allow < 3 args in both epi_slide and epix_slide Jun 26, 2024
@brookslogan
Copy link
Contributor

brookslogan commented Aug 21, 2024

We might want to tackle this after we raise an error on function(x) mean(x). I plan to work on that in #509 (it's related through a chain of difficulties).

Context: we originally inherited a >= 2-arg restriction from group_modify. We then kept this pattern when expanding to >= 3 args. To make this change, we'll probably need to work around group_modify, but we may also get to eliminate some code related to tacking on the third arg.

@dshemetov
Copy link
Contributor

Huh, I started thinking about this and the logic seems a bit tricky. Like even leaving aside tidyeval and leaving aside functions that allow dots, we have these cases:

  • if a user passes a one arg function (like mean above), sure we can just add 2 faux args
  • if a user passes a two arg function, we have to inspect the args and
    • first figure out which one is the data arg and which one is not
    • then figure out if the second arg is a group key or a ref_time_value or a function parameter - doing this based on argument names seems pretty fragile, but maybe it could work - and then either adding faux args or not
  • if a user passes an n arg function, the above logic is the essentially the same, but now you're searching for the group_key and the ref_time_value key in that list and hoping to find them and handling things the same way

Writing this out, it seems sort of doable, though a bit fragile. I haven't thought through the dots case, but it seems hard to get a grasp on.

@brookslogan
Copy link
Contributor

brookslogan commented Oct 5, 2024

We already attempted some of this logic in the warnings; they may provide some ideas. We constrained the arguments by position, not by name, so there was no searching through the argument names. We were a bit more trigger-happy there, but recent slide updates (.keep = TRUE, epi_df_forbidden_methods.R) may let us relax them.

Some notes:

  • On things that are "actually" errors / require additional syntax to distinguish (e.g., across support), since epi[x]_slide feeds epi_dfs into the slide comps, not individual columns:
    • mean is function (x, ...). We can immediately error or let it fall through to epi_df_forbidden_methods.R.
    • sum is function (..., na.rm = FALSE). Same deal.
  • On things that aren't:
    • 1-arg has to be a lot more popular than 2-arg. Maybe let's not even attempt 2-arg, especially not before deciding whether the future of epix_slide naming & general interface (e.g., whether to remove grouping support).

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

No branches or pull requests

3 participants