epiforecasts / EpiSoon

Forecasting the effective reproduction number over short timescales
https://epiforecasts.io/EpiSoon/
Other
7 stars 3 forks source link

added support for models from brms #46

Closed medewitt closed 4 years ago

medewitt commented 4 years ago

I added a wrapper function for models from brms. This allows users to pass models using the bf model syntax through the brms_model function.

This allows for a wider range of models to be used via brms. Tested it with forecast_rt and it works well. It does add two dependencies in the form of brms and tidybayes.

seabbs commented 4 years ago

Thanks for this @medewitt (and sorry for not getting back via email promptly).

In principle, these changes look really nice (+ the nice clean up you have done). I have some work deadlines tomorrow but will review it properly on Monday. It might be good to make the dep on brms and tidybayes a suggests rather than full import. If every wrapper that gets added comes with a package dependency things might start getting quite heavy (though I realize that is not what we have done with fable and bsts so perhaps I am wrong on this).

If your using this somewhere would also be interested in seeing what your doing.

Thanks again for the pull request

Sam

medewitt commented 4 years ago

@seabbs no problem, I understand the delay and the concerns about bloating the package with lots of dependencies.

I moved tidybayes and brms to the suggests section as you suggested. I had to remove some of the importFrom for the functions, which shouldn't be a big deal. I also wrote a namespace check function to check if they are installed. This function could be paired with a match.call on some of the other wrappers to check if a suggested package is installed prior to failing silently.

Might be an opportunity to make a EpiSoonExtra (terrible name, I know) that has several additional models. I was coding up a couple of Stan models and putting them in a proper stand-alone package would help on a couple of fronts (keep EpiSoon lightweight outside of a generic Stan wrapper, compile Stan in a separate package, separate unit checks).

I shoot you what I'm working on by email tomorrow or Monday.

Best,

Michael

medewitt commented 4 years ago

@seabbs no worries and absolutely. I'll submit a PR with my information. I've been working on modeling Rt with some high frequency mobility data. Still trying to figure out how to package it, but it might be an interesting addition, if you're interested

seabbs commented 4 years ago

Awesome thanks.

Sounds interesting! One thing to note that we have been talking about is that mobility date is currently very predictive but a successful end to the lockdown is essentially when mobility is no longer predictive of cases (because other measures are preventing cases occurring other than restricting mobility).