-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
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.
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?
Thanks @sbfnk - agree with all. Will wait perhaps a week or two in case more feedback comes in. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e99365a is merged into main:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e99365a is merged into main:
|
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. |
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 hope this is helpful, let me know if anything is unclear! |
Thanks @alxsrobert - all helpful suggestions that I'll incorporate as edits to this document. |
d5831ed
to
3fe4be5
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 6258c0c is merged into main:
|
3fe4be5
to
939b223
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if d2d1fe4 is merged into main:
|
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.