-
Notifications
You must be signed in to change notification settings - Fork 33
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
streamline S3 classes #451
Comments
Yes, I agree. This is a nice idea. Helps make a point that the models are really subsets/supersets of each other as well. |
I'm working on this and have a few questions:
Are all three functions going to return the unformatted Is
|
To me that sounds like a sensible approach, i.e. S3 methods which contain the stanfit, observations and args.
Yes
I think we want an
|
I think I'm missing some context here but if I may chime in: I'm never a big fan of including inputs (observations & args) in the output. Is there a specific reason this is necessary? If the user ever loses tracks of what these are, then something went wrong in their process and any potential fix here (by storing the inputs with the output) would not manage to solve entirely the problems this process issue introduces. Additionally, using raw stanfit objects provides nice benefits: you don't necessarily have to write custom methods for everything since you can directly leverage the entire ecosystem developed around stanfit objects. (Some related thoughts are shared in this blog post: https://epiverse-trace.github.io/posts/parent-class/.) We had a related discussion in epiverse-trace/serofoi#89. |
Could you provide some evidence for this? In particular I would be interested in hearing more user evidence as to the relative advantages of methods that "just work" vs the value of learning to map your data to your model fit. In my experience the output users directly want is rarely the output provided directly by the stan methods. I agree there is a lot of value if having an output that is simple and close to what users may have experience with (though the vast majority of push back I hear from the epiverse ecosystem is that users do not have familiarity with stan and so it seems odd to now see exposing this to them very clearly as being an advantage). A good counter point to you should just output stan models is brms which overloads stan with things like the model fit data to make for an easier user experience. As a counterpoint to above points in |
I think we are making related but slightly different points. Please let me know if I misunderstood your point:
But my initial point, here and in the other issue, was specifically that:
The result of these different considerations led to keeping the As an aside:
There are a number of points on which there isn't a single viewpoint within the project, hence why I find this kind of discussion useful & informative |
I think the point about not saving input data makes sense but in this case the rationale to store inputs is that they're used for further processing, i.e. we need the observations to be able to plot them alongside the predictions (and to identify dates), and we need the stan arguments / passed data in order to know which delay variable corresponds to which epidemiological delay (and, once #871 has been merged, other parameters). In principle this information could be stored separately but that would add more overhead so that I think it's easier from a conceptual and maintenance point of view to store the data as they're already passed to stan. As an aside I really like the idea of inheriting from the the stan objects. In this case this is tricky for two reasons:
So we might be better off with a list-based class that has a |
Currently the three models included in the package return S3 class with similar but slightly different contents. Whilst to some degree that's expected because of their different use cases it might be nice to streamline this a bit to simplify the user experience and allow common access functions.
I'm thinking that perhaps as a minimum they should all return
args
(the result ofcreate_stan_args
),fit
(thestanfit
object) andobservations
as everything else, I think, could be constructed from these three elements using appropriatesummary
orextract_...
methods.If doing this changes would be likely to break existing workflows, but as we're introducing breaking changes towards 2.0.0 anyway this might be a good time to do this if at all.
Current returns are:
Created on 2023-09-08 with reprex v2.0.2
The text was updated successfully, but these errors were encountered: