Incorporate model() in mbsts.mcmc() and change predict.mbsts() and causal.mbsts() functions

FMenchetti / CausalMBSTS

16 stars 0 forks source link

Incorporate model() in mbsts.mcmc() and change predict.mbsts() and causal.mbsts() functions #33

Closed vliublinska closed 4 years ago

vliublinska commented 4 years ago

And do not export mbsts.mcmc (or whatever the renamed version is).

FMenchetti commented 4 years ago

Based on our discussion on PR #36 , I suggest to embed model into mbsts.mcmc and rename the resulting function as modelMBSTS (producing an object of class mbsts). Then, I'd rather predict.mbsts to be a method for the fitted object of class mbsts (as it appears to be common practice). Then, as suggested by @vliublinska , we could feed modelMBSTS and predict.mbsts to causal.mbsts. We can also rename the function as CausalMBSTS (main function that gives the name to package as it is done in CausalImpact). Would that be ok?

vliublinska commented 4 years ago

@FMenchetti, I see where you are coming from. Perhaps, a model that creates an object of class mbsts can be called as.mbsts to follow R convention?

Would it make sense then to keep causal.mbsts and predict.mbsts that naturally take in mbsts objects. Then, in addition, we could develop CausalMBSTS as a low-barrier wrapper that would combine as.mbsts, predict.mbsts, and causal.mbsts and let the user start using the package by running just one function?

FMenchetti commented 4 years ago

Yes, I think that would be useful!

vliublinska commented 4 years ago

Great! So, to summarize:

[x] Merge model() and mbsts.mcmc() and call it as.mbsts() (or something along those lines, if it makes sense)
[x] Make sure that the result of as.mbsts() are accepted by predict.mbsts or causal.mbsts
[x] Develop a low-barrier wrapper for causal.mbsts, e.g., called CausalMBSTS, that accepts regular data frames and simple model description (such as components = c("trend","seasonal"), seas.period = 7).

Please, edit if I misrepresented anything.

FMenchetti commented 4 years ago

@vliublinska while I was updating the functions, I realized that causal.mbsts can't accept objects of class 'mbsts', because it would need to run once again the mcmc on the pre-period only (thereby completely ignoring the results from as.mbsts). So, I simply changed causal.mbsts in CausalMBSTS .

The final structure of the package should be clear now: model and mcmc are internal functions and are used in as.mbsts (which defines and estimates the desired Bayesian model producing an object of class mbsts) ; predict.mbsts is a method for mbsts objects ; CausalMBSTS does everything from the model definition to the computation of the causal effect. Finally, there are two methods for object of class CausalMBSTS (print and plot).

So, I'm closing the issue (should you have further suggestions feel free to re-open it).