Skip to content
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

First draft of predict method #48

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

First draft of predict method #48

wants to merge 5 commits into from

Conversation

zsusswein
Copy link
Collaborator

@zsusswein zsusswein commented Oct 14, 2024

I took a crack at the bones of a predict method. At the moment it only pulls out observed cases, but it has the machinery to also do incidence, little r, and Rt.

I originally implemented all of the above, but I started to have second thoughts that this was the right way to do things. I wanted to pare things back and get some feedback that this was on the right track before I did all the docs and tests. Accordingly, things are a bit of a mess still -- please ignore tests/docs/correctness for now.

The key thing I want feedback on is extensibility of this design:

  • I set up predict() to take a model and an arg type to generate the value we want. Then I'll handle the indexing and draws stuff. I was thinking each arg would have its own function that could do S3 dispatch under the hood and the functions would be cased with if blocks.
  • But then how to handle user-specification of whether to nowcast?
  • How to handle user specification of expectation vs posterior prediction?

Potential solutions:

  • Nowcasting could be a flag and I do another if? But messy.
  • Posterior predictions vs expectations could be fitted.values vs. predict? Maybe?
  • We could make everything an arg to predict and dispatch all the way down, but I'm not sure that's actually any cleaner.

The other thing I could use advice on is the use of gratia. It's really nice to have the functionality, but it's more of an interactive package and I don't love all the dependences it brings along (but it's mainly dplyr and it doesn't cost that much...)

Closes #47

@zsusswein zsusswein marked this pull request as ready for review October 14, 2024 18:27
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Just flagging I have seen this and we discussed it f2f and agreed some changes.

@zsusswein zsusswein marked this pull request as draft October 16, 2024 23:18
@zsusswein
Copy link
Collaborator Author

zsusswein commented Oct 17, 2024

My notes from that meeting:

  • Handle posterior predictive quantities in predict.RtGam(), posterior expectations in fitted.values() or something similar, and handle nowcasting in some to-be-specified top-level function down the road
  • Use type to switch between parameters in predict.RtGam() with casing of functions as we do currently. Keep the plan to make those functions dispatch on the backend. Expose those functions (maybe @ keywords internal them?) for more advanced users.
  • Move the date handling logic and input checks into those the cased type-specific functions

Sam, let me know if any of the above seems wrong to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Posterior draws: cases
2 participants