-
Notifications
You must be signed in to change notification settings - Fork 1
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
Package structure #127
Comments
Thanks for raising @Karim-Mane and giving your reasoning on why this change would be useful. I'm personally not in favour of making this change as I believe it will increase the complexity of the package, but not provide extra functionality. The user will be required to carry out more steps in construct a line list, which currently can be done with a single function On some the advantages mentioned:
This statement is true, but the
This has been discussed previously (see #41) and it was decided that the output of This pipeline approach to building a line list was also mentioned in the GitHub discussion that started {simulist}. @jamesmbaazam stated on that discussion that going for single function calls would be preferable to chaining functions together with pipes. I see three viable development paths:
I'm personally in favour of option 1 or 2. A last point to consider. The example pipeline outlined above is for Depending on what option is selected it may influence the ability to use other transmission models in future development (see #34 & #43). Tagging @adamkucharski, @sbfnk, @CarmenTamayo, @avallecam, @Degoot-AM to get their opinions on this from an epidemiological user perspective first to see what is desirable functionality and UX/UI. Then if we decide to make a change from the current package structure I will tag developers to discuss how to implement such a change. |
I favour option 2 ...! |
From a user perspective, I see the most immediate benefit in path number 1, to keep current implementation, make that explicit in the design document, and try not to affect package complexity and maintenance burden. Additionally, I would say that if a user requires fewer columns, it should be feasible to drop the columns instead. (using Another feature wanted to consider for this evaluation was the possibility of generating simulations in different country settings: different population pyramid features from {epiCo}, population contact matrix from {socialmixr}, and the disease susceptibility profiles as we do in {finalsize}, and similarly with hospitalization. The vignettes in age-structured and age-stratified hospitalization risk and deaths seem to cover these features. I have not explored them yet, but the package may already have some infrastructure to include additional features that describe the population. |
My suggestions would have resulted in updating the
After reviewing @joshwlambert comments above, and taking other rounds of look into the function, the following argument seems to have more importance than the function structure:
This is because the patient and the disease outcome data are not accounted at this level, hence the function will need to be renamed.
However, if more data (columns) will be added to the simulated linelist in the future, I believe it is worth restructuring the function by exporting some internal building blocks of the linelist. Otherwise, the complexity that my suggestion would have introduced on the package will reflect on the function complexity. |
Thanks for the feedback @Karim-Mane. I agree with your points. I will leave this issue open as I think we can explore adding functions to iteratively build line lists with pipeable functions in a future version of {simulist}, but I will not focus on this until other higher priority features have been included. |
Looking at
sim_linelist()
, I think it might be worth considering the following format:add_age()
andadd_sex()
could be combined into the same function likeadd_socio_demography()
. This will make it easy to add more socio-demographics later on.The advantage of this approach will be:
sim_linelist()
and other functionsThe text was updated successfully, but these errors were encountered: