Skip to content
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

Add developer side walkthrough #210

Merged
merged 19 commits into from
May 30, 2024
Merged

Add developer side walkthrough #210

merged 19 commits into from
May 30, 2024

Conversation

pratikunterwegs
Copy link
Collaborator

This PR aims to fix #193 by adding a developer side walkthrough vignette that lays out how to make some quick edits to the ODE models.

@pratikunterwegs pratikunterwegs self-assigned this Mar 26, 2024
@pratikunterwegs pratikunterwegs added the Documentation Improvements or additions to documentation label Mar 26, 2024
Copy link

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this will be really useful for people wanting to make changes to the model(s). I left a few comments where I stumbled across things.

Can you add a section "how to add a new model"? I.e. should I start with copying the default model and then take it from there?

vignettes/developer-walkthrough.Rmd Outdated Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Outdated Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Outdated Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Outdated Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Show resolved Hide resolved
vignettes/developer-walkthrough.Rmd Show resolved Hide resolved
@pratikunterwegs
Copy link
Collaborator Author

Thanks @sbfnk - agree with all. Will wait perhaps a week or two in case more feedback comes in.

Copy link

github-actions bot commented Apr 9, 2024

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e99365a is merged into main:

  • ❗🐌default_ode: 14.2ms -> 14.4ms [+0.05%, +2.7%]
  • ✔️default_ode_interventions: 71.9ms -> 71.9ms [-0.51%, +0.49%]
  • ❗🐌default_ode_param_vec: 827ms -> 833ms [+0.07%, +1.33%]
  • ✔️default_ode_paramvec_intervs: 6.2s -> 6.26s [-0.01%, +1.96%]
  • ✔️ebola: 34.5ms -> 34.6ms [-1.16%, +1.91%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link

github-actions bot commented Apr 9, 2024

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e99365a is merged into main:

  • ✔️default_ode: 16.2ms -> 16.3ms [-2.12%, +3.16%]
  • ✔️default_ode_interventions: 76.2ms -> 76.3ms [-1.1%, +1.48%]
  • ✔️default_ode_param_vec: 867ms -> 872ms [-0.14%, +1.29%]
  • 🚀default_ode_paramvec_intervs: 6.56s -> 6.5s [-1.76%, -0.17%]
  • ✔️ebola: 50.3ms -> 49.5ms [-5.5%, +2.32%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@pratikunterwegs
Copy link
Collaborator Author

Thanks @sbfnk and @adamkucharski for taking a look at this. I've pushed changes that show the minimal package structure, and a section on adding and removing parameters. Hope this makes it easier to read. I'm aiming to merge this by the end of the week.

@alxsrobert
Copy link

Hi Pratik, Adam asked me last week to have a look at the developer vignette for the perspective of someone who is not very familiar with the package (so I've also looked at the "design decision" vignette to understand the structure of epidemics). I've written a few notes. Overall I thought the vignette was clear and well-designed, but I thought of some clarifications:

  • I think that the "guide to package structure" section in the developer walkthrough does not make sense to someone who is not already quite familiar with the package:
    • Some explanations of the files do not really make sense to me ("functions to construct cross-checking fns", "MODEL FUNC WRAPPING ODE SYSTEM; LEVEL 2")
    • The levels are not yet defined and therefore appear confusing.
    • I would suggest removing the mentions of scripts that are not mentioned in this vignette (so have a "simplified" structure, and maybe have the full structure at the end of the vignette, where there can be a slightly longer description of each file ). And introduce what the levels correspond to before the package structure.
  • Similarly, in the "design decisions" vignette, I would put figure 1 and 2 (so the "package architecture" section) after the definitions of some terms and broad concepts:
    • Figure 2 is quite overwhelming, so I didn't really know what I was meant to look for and take from it. I was also wondering what terms like "composable elements" from Figure 1 meant, only to have it defined at the next section, it would be better if it was defined before the figure (or right after it).
    • I would suggest adding a few sentences explaining what the figure is for / what we should look at. Maybe also splitting Figure 2 into multiple figures?
  • I think the section "adding or removing parameter" could do with some example code for each subsection. For instance, "Modify the compartmental transitions in the FunctionObject ODE model inst/include/model_*.h appropriately to include the mortality rate" seems unclear to me, does that just refer to adding / removing an extra argument? Having the description without the example makes it sound complicated, while if I understand well each step requires little re-coding (for a simple example).
  • I think the sections with examples are good and clear. It would be good to remind the reader in what script each snippet of code is added (I think they are mentioned in the "Modifying compartmental flow..." section heading, but it would be clearer if it was repeated here).
  • I'm don't think that every single section needs a specific example, but in my opinion the vignette should start with walk-through examples (like in "modelling birth"/ "modelling waning immunity"), so the reader would start with simple code, and become more familiar with the structure, before sections that just feature explanations.

I hope this is helpful, let me know if anything is unclear!

@pratikunterwegs
Copy link
Collaborator Author

Thanks @alxsrobert - all helpful suggestions that I'll incorporate as edits to this document.

Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 6258c0c is merged into main:

  • ❗🐌default_ode: 14.2ms -> 14.4ms [+0.31%, +3.41%]
  • ✔️default_ode_interventions: 72.1ms -> 72.4ms [-0.26%, +0.92%]
  • ✔️default_ode_param_vec: 827ms -> 832ms [-0.13%, +1.23%]
  • ✔️default_ode_paramvec_intervs: 6.23s -> 6.24s [-0.47%, +0.64%]
  • ✔️ebola: 44.8ms -> 44.7ms [-4.73%, +4.03%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if d2d1fe4 is merged into main:

  • ✔️default_ode: 10.6ms -> 10.6ms [-0.52%, +0.62%]
  • ✔️default_ode_interventions: 94.5ms -> 94.3ms [-1.15%, +0.82%]
  • ❗🐌default_ode_param_vec: 817ms -> 821ms [+0.07%, +1.05%]
  • ✔️default_ode_paramvec_intervs: 6.14s -> 6.13s [-0.71%, +0.56%]
  • ✔️ebola: 539ms -> 539ms [-0.51%, +0.82%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@pratikunterwegs pratikunterwegs merged commit 8bb4fb0 into main May 30, 2024
14 checks passed
@pratikunterwegs pratikunterwegs deleted the vignette-walkthrough branch May 30, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation
Development

Successfully merging this pull request may close these issues.

Document how to make simple, developer-side model edits
4 participants