-
Notifications
You must be signed in to change notification settings - Fork 32
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
Develop #343
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Yep, just haven't had a chance to put in an Issue / address yet. Will do so. |
This comment was marked as outdated.
This comment was marked as outdated.
…t use the new dist input
This comment was marked as outdated.
This comment was marked as outdated.
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.
Looks good. Good idea to add the @author
field to give credit. Is there anyone else who may have contributed to any of the functions and would need to be acknowledged?
My only hesitation is in the set up for the delay distributions. The arguments to be passed to generation_time
, delays
, and truncation
are all of the same type (a delay distribution) but at the moment need to be invoked using different function, which could be avoided. See here for an alternative implementation, removing some of the _opts
model (a downside perhaps) but reducing these three options to all involve calls to dist_spec
(a benefit). If we go further with it it needs a bit more checking, possibly improved code organisation (e.g. the code of munging delays is duplicated at the moment and could be shipped out into a function) and updating the documentation. Related discussion at
#343 (comment)
I'm also not 100% convinced we really need to support the mixture of fixed and uncertain delay distributions (which causes a lot of code overhead). It might be simpler to only support one or the other, in which case we could combine multiple delays in R instead of stan.
Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
I really like the dist_spec usage as an input and I think the s3 c() method is a good idea. I also like having seeding time be it’s own function. I am struggling a little with the amount of internal code replication and complexity this adds. We could obviously reduce this with some internal functions etc but they would still have to be reliably called on all inputs. I would also be a bit worried about future extensions using this approach (for example what is the input if you had time varying distributions or needed some option that was specific to a given input (for example if we addd the ability to control the left truncation of the generation time). The main upside (beyond dist_spec usage as input which as I said is nice) is that the user doesn’t have to write an of the opts functions but the cost is more opaqueness in what is happening? I’ll do some cleaning up of what you have implemented and see how it feels as I do that. I agree on the distribution mixing not being incredibly useful but perhaps as it is implemented we should stick with it and see if a use if found and if not depreciate? |
Yes agree on all of these. Perhaps a half way house would be to bring the _opts functions back but have all of them take an S3 delay_dist object as first argument? The post processing of e.g. delays could then move back into these functions. This would also remove the need for dot arguments anywhere (as we can specify multiple delays with the c function). |
Yes I think we should definitely do this as a minimum. The |
It would be nice if we can work out what needs to be done (i.e do we want to bring in the suggested extended version of In my view this is good to merge and |
From slack chats my conclusion is we are happy to merge here and move towards a release today. |
Merges in development changes