-
Notifications
You must be signed in to change notification settings - Fork 8
Make tidy sliding play nicely with data frame inputs #55
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
Comments
We just ran into this as part of cmu-delphi/epipredict#8 and cmu-delphi/epipredict#3. Definitely want this in one way or another. Thinking about the implementation makes me wonder if the interface of Implementation: I could work on this after forecaster calibration exploration, but that will take a bit.
|
Thanks for the response. As for the epipredict issues: that's partly why I thought of it. Though I'm in the middle of writing a long comment on cmu-delphi/epipredict#8 that's going to argue against letting the user pass a straight data frame. So I think we should think of the use case in the current issue as just one we'd want to support in general. Responses are helpful but I think may be a bit of confusion here? I don't see a way to simplify And the multicolumn case you're describing I believe would be handed by returning a list column and using the default argument So to be clear, the When using tidy sliding, I'm proposing you can access the entire data frame with |
Just following up: the analogous implementation (whatever we do) should be something we carry over to And in thinking that through, I realized I never implemented a formula option in Should we implement that? Would give the greatest degree of symmetry in between the two sliding functions which I think we should try to maintain. |
Thanks for clearing up my confusion on the current functionality. So maybe "simplifying" would just be removing useful functionality. For [(epipred, other generalized)] coding on top of Implementation detail-wise we might need to check why |
Sorry, I'm not sure I understand your current perspective.
I see a use case for both, and now given your implementation notes (thank you, helpful), I'm actually leaning towards doing nothing further right now for tidy sliding---meaning, tidy sliding can only access variables by name. If you ever want to "get this whole data frame", then you should be able to do that with a formula or function, and that's one of their main use cases. If we wanted to do something as advanced as what you're suggesting, in your last comment (with piping) then it's best to use a function. I don't think pipes play nicely with formulas (my impression based on basic experimentation just now). Thoughts? |
p.s. I fixed the formula problem (that If you think it's more fluid we can talk about this and other issues on the call later today. Thanks! |
Neither now, sorry for the noise. As you said, both have use cases. The tidy eval + |
No worries, your thoughts & discussions points are helpful (here and always). So I think we can leave this issue open but plan not to attack it until we feel that it's risen to a much higher priority. |
Note that some dplyr operations support
|
--- CURRENT STATE: --- |
Tidy sliding is pretty convenient. But in some applications, I might want to actually be sliding a function that takes an entire data frame rather than pass in individual columns as arguments.
For example, let's suppose
my_fun()
is a complicated function that I've written that acts on a whole data frame, whose signature is:Then I could do:
But I could NOT easily do some tidy sliding, because there's currently no way to get the tidy eval thing I've written to understand "pass the sliding data frame". It only understands "pass these sliding variables".
Ideally, we'd code in some special variable name that
epi_slide()
(and analogouslyepix_slide()
) understands and treats in a special way, say.cur_df
(prefixing a dot so that it doesn't conflict with variable names in the environment) and then we could do:which is more intuitive/convenient for folks who like tidy code.
@lcbrooks @dajmcdon @jacobbien @elray1 what do you think? Anybody want to take a stab at implementing it?
The text was updated successfully, but these errors were encountered: