-
Notifications
You must be signed in to change notification settings - Fork 8
Should epi_signal
inherit from tsibble
?
#7
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
One potential advantage to |
If we don't want to use
|
So as I alluded to over on #12, #10, #8, I'm thinking about making a big redesign on a branch to address all of these issues, as well as the current one---moving over to the
As suggested on the other issues, it seems like the best way forward is to stick with the
The column names can be anything and the designation as So now I'm left wondering: what, if any, is the purpose of defining an Somewhat relatedly, though really not the same question, I'm wondering what is the importance of actually treating geographies explicitly anymore. Is geo value just one type of key, and we don't even attempt to distinguish it from an arbitrary key? Or should we be providing built-in utilities for handling standard geo types (like aggregation or dis-aggregation between standard geo types). I want to spend a bit of time thinking here before doing anything. Just treating everything as a @elray1 @dajmcdon @jacobbien Your thoughts? (Again I'm sure it would be really useful to hear from Logan but not tagging him so that he can enjoy a couple days off!) |
Yeah, I agree with @ryantibs that At first it might seem this isn't really necessary. For example, consider this example from the
If one wanted to aggregate to the This is directly analogous to how
An example: Suppose
Here Obviously, instead of |
Here are a few very quick/minor thoughts:
|
Thanks very much to both of you for helpful answers. I'm still struggling though. I'm trying to play devil's advocate here, to be clear, for both sides of the argument:
So is there any point in defining a custom To go back the specific question about structure of the data frame itself. Do we really let (Evan: I didn't really get your last comment which seems related to this. Was it about renaming |
Also quick, minor. (Drafted pre-Ryan, but posted after)
|
I basically don't have an opinion about Ryan's questions about whether we'd want to enforce more structure for the Brief follow up on the
This is a minor point, but I think a small argument in favor of defining a class and using a name other than |
Thanks @elray1. Just an update for you all @dajmcdon @jacobbien @brookslogan. This morning I worked on defining
So I'm going to trash all my changes from this morning and revert back to having
and so on. Meaning, just convert to a I think we should still keep it in mind that we may want to refactor everything in the future to inherit from |
- As per the discussion on #7, I ditched the `value` column in an `epi_signal` object completely - Aside from `geo_value` and `time_value`, the other columns are arbitrary and can be treated as measured variables - This required refactoring `pct_change()`, `estimate_deriv()`, and `sliced_cor()` so that the user can specify the variables they want to act on - In doing so, I used tidy evaluation (so the user just passes variable names directly), and extended this to the `by` arg of `sliced_cor()` as well - This will dovetail nicely into allow arbitrary groupings later on (for sliding by more than just geo, for example, as per #12) - The other thing I did here (probably shouldn't have lumped it into the same commit though ...) was to broaden the allowed `time_type`s to include a new "day-time" type, coded as a `POSIXct` object (time on a given day, measured to the second) - I expanded `slide_by_geo()` so that the user can specify the definition of a time step; so, for example, one can slide over the last 7 hours (if `time_type` was "day-time"), or the last 7 minutes, or whatever - Rewrote a lot of the documentation, updated the vignettes
Hi Team, Thanks for considering to leverage {tsibble} for
This design is on purpose:
We run validity checks when a tsibble is created, albeit it can be turned off with |
Thanks @earowang. It's great to have THE What you say makes a lot of sense. I think for now in the interest of moving quickly I'm going to keep the |
Just a heads up that I'm going to close this with #14. @jacobbien (Sorry to pick on you 🙂, but it's what your "reward" for having all sorts of useful comments about these): Can you please create two new issues? One on time aggregation and one on geo aggregation. Time aggregation one: should be about coming up with examples to demo casting to Geo aggregation one: should outline the kind of functionality we want implemented (summarize/consolidate the nice suggestions you and others made here), and also have as part of it filling out the second half of the "aggregation.Rmd" vignette. Thanks!! |
The tsibble package, as @jacobbien mentioned, looks like it has a structure that would be helpful to have
epi_signal
inherit, rather a plaintibble
. Atsibble
has the following structure:key
: for us this would begeo_value
, I think;index
: for us would be betime_value
, I think;value
, here.Generally, we should also check out the tidyverts, and yes, that's not a typo, click on the link. It would be nice to see if anything here just takes care of things we might need (or might have already implemented?) in
epitools
, and soon, inepipred
.@jacobbien @dajmcdon @capnrefsmmat @brookslogan Looking through all of this would be a big job, but do any of you guys just want to each take a look at some part, and report back what you think here, on this issue?
The text was updated successfully, but these errors were encountered: