-
Notifications
You must be signed in to change notification settings - Fork 10
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
Panel Data Vignette (Issue 99) #115
Conversation
bda4ea5
to
fb030b5
Compare
epi_keys_mold
fix1fd96bb
to
0b797b0
Compare
I gave this a quick once-over. I think the idea is great. Major comments:
Minor:
|
615cde4
to
e6f62ec
Compare
Thanks for the feedback & sorry this took so long - could you take another look @dajmcdon? |
Blocked by #291 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty great. A couple of minor things:
- Suppress messages & warnings for the first code chunk (else we get a bunch of attach package messages, at least when I knit the .Rmd)
- I made a couple of very minor changes to the sentences & fixed a few typos
- The size of the model diagnostics plot appears too large/elongated when I knitted the file & look at the output? Maybe that’s just an issue on my computer, but I’ve resized it in a simpler way, just in case that appears like that for others as well.
I’ve fixed these minor changes & pushed the updates to Github. I have not fixed the following more important things yet because I think that they require your input...
Important things for you to weigh in on (presented by section):
Model fitting and prediction:
-
The intercept and other values discussed below the model output seem wrong (unless they are transformed or something?). Also, see the sentence on what coefficients are significantly greater than zero.
-
Is that correct to say…”lags at 2 years and 3 years ago have coefficients significantly greater than zero.” Because isn’t the maximum lag used correspond to 2 years ago? The current way of talking about lags also comes up in the section titled Model fitting & postprocessing.
Autoregressive model with exogenous inputs:
- The model form seems wrong (currently shows interaction term, shouldn’t it be +)?
Model fitting & postprocessing:
-
I think that the model processing steps should be introduced and enumerated in the order they are performed (else it gets confusing to have the current out-of-order presentation).
-
I don’t see that the conclusions on significance are correct? Because, from a quick inspection, I thought that typically in R model output, one asterisk typically means “p < . 05”. If yeah, then there’s more/different terms that are significant than what’s currently indicated in the discussion of the model summary,
Flatline forecaster:
- Is the flatline forecaster model form correct? I am not convinced that the alphas should be there if we are still just propagating a value ahead… If that’s right, then I think the model form should be as it is in https://cmu-delphi.github.io/cste-forecast-workshop-2023/#/canned-forecasters-that-work-out-of-the-box.-2.
Overall thoughts:
- Related to my last point about the flatline forecaster presentation, I don’t see any location indices used anywhere or hats to indicate predictions? If we really want to be precise & consistent with other vignettes, presentations and tooling book chapters, it is probably good to include those. Refer to the previous link on the presentation you guys gave for an example of this.
- There seems to be a general lack of plots… Currently, I see a line plot at the beginning when exploring the data and a model diagnostics plot. It may be nice to show how to look at/use the predictions for panel data instead of just showing the reader the code.
- Related to my previous point: The vignette ends a little abruptly for me… Perhaps we should add a short summary at the end or a sentence or two after the final code block to suggest something for the reader to try on their own. Or we could expand the last section a bit (ex. add good plot to display for these results & discuss them briefly? A reader may find that useful and a nice way to end things).
I may be wrong about these points, but I think they are worth briefly talking about before merging.
@rachlobay All excellent points. I think I've hit them all. Would you mind giving it a quick once-over if you have a chance? |
Looks great! I'll merge now |
Addresses issue #99
Changes:
epi_df
formatepi_df
withepi_recipe
andepi_workflow