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

Doc multinom #102

Merged
merged 16 commits into from Mar 1, 2018
Merged

Doc multinom #102

merged 16 commits into from Mar 1, 2018

Conversation

@thibautjombart
Copy link
Contributor

@thibautjombart thibautjombart commented Feb 21, 2018

This adds a new vignette "discrete" on discrete deterministic and stochastic models, with a strong emphasis on the latter. Explains how binomial, Poisson, and multinomial distributions are used in this context.

Provides a simple example using SIR models and then a more advanced one using a SEIRDS model with imported cases. Still needs some proof-reading but should be useful as it is.

@thibautjombart thibautjombart requested a review from richfitz Feb 21, 2018
@richfitz
Copy link
Member

@richfitz richfitz commented Feb 21, 2018

Can you please re-point this at i100_multinom please? If that's a faff don't worry - I can merge that one in at the same time. I'll go through this tomorrow. Thanks again!

@codecov-io
Copy link

@codecov-io codecov-io commented Feb 21, 2018

Codecov Report

Merging #102 into i100_multinom will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##           i100_multinom    #102   +/-   ##
=============================================
  Coverage           99.5%   99.5%           
=============================================
  Files                 21      21           
  Lines               3827    3827           
=============================================
  Hits                3808    3808           
  Misses                19      19

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dfe154...064ef7f. Read the comment docs.

@thibautjombart thibautjombart changed the base branch from master to i100_multinom Feb 21, 2018
@thibautjombart
Copy link
Contributor Author

@thibautjombart thibautjombart commented Feb 21, 2018

@richfitz sorry, didn't think about the base, now repointed to the right branch

`x$run()`, providing time steps as single argument, e.g.:

``` {r sir-deterministic}
sir_col <- c("#8c8cd9", "#cc0044", "#999966")

This comment has been minimized.

@richfitz

richfitz Feb 22, 2018
Member

My preference with the plots is to:

  • drop the main = title
  • set the margins to be smaller (par(mar = c(4.1, 4.1, 0.5, 0.5)) or so)
  • drop the box on the legend (bty = "n")
  • make the lwd just 1
  • use las = 1 to align the y labels more nicely

These are all negotiable, but eventually all the plots should have similar style across the package

This comment has been minimized.

@thibautjombart

thibautjombart Feb 27, 2018
Author Contributor

Will be done in revision.




Stochastic discrete SIR

This comment has been minimized.

@richfitz

richfitz Feb 22, 2018
Member

Missing title

This comment has been minimized.

@thibautjombart

thibautjombart Feb 28, 2018
Author Contributor

Done at a6f2476

as follows:

``` {r load_sir_s}
path_sir_model_s <- system.file("examples/discrete_stochastic_sir.R", package = "odin")

This comment has been minimized.

@richfitz

richfitz Feb 22, 2018
Member

can you please indicate which files you feel these included will have replaced the ones I popped into 9dfe154 and I'll rewire the tests to point your new versions

This comment has been minimized.

@thibautjombart

thibautjombart Feb 28, 2018
Author Contributor

Corresponding test files are basically all files in tests/testthat/stochastic:

thibaut@nuxVM ~/dev/odin $ ls tests/testthat/stochastic/
sir_discrete.R  sir_discrete_stochastic2.R  sir_discrete_stochastic_multi.R  sir_discrete_stochastic.R
``` {r }
sir_model_s <- odin::odin(path_sir_model_s, verbose = FALSE, skip_cache = TRUE)
sir_model_s
x <- sir_model_s(I_ini = 10) # customise param: I_ini = 10 individuals

This comment has been minimized.

@richfitz

richfitz Feb 22, 2018
Member

I would lift the comment here into the prose

This comment has been minimized.

@thibautjombart

thibautjombart Feb 28, 2018
Author Contributor

done at e1e05cf


We can use the same workflow as before to run the model:
``` {r }
sir_model_s <- odin::odin(path_sir_model_s, verbose = FALSE, skip_cache = TRUE)

This comment has been minimized.

@richfitz

richfitz Feb 22, 2018
Member

Elsewhere I call these "generators" (so odin::odin creates a generator, and running the generator creates a model). So perhaps re-name as:

sir_s <- odin::odin(...)
mod_sir_s <- sir_s(I_ini = 10)

there is no obviously correct set of names, and this does start making more sense as an approach once things are packaged

This comment has been minimized.

@thibautjombart

thibautjombart Feb 28, 2018
Author Contributor

Yes it is clearer this way: cc8886a

```

``` {r echo = FALSE, comment = NA}
cat(readLines(path_seirds_model), sep = "\n")

This comment has been minimized.

@richfitz

richfitz Feb 22, 2018
Member

wrap these in output_r - see the source of the main vignette for details

This comment has been minimized.

@thibautjombart

thibautjombart Feb 28, 2018
Author Contributor

done at 9ce2144

```

``` {r seirds}
seirds_model <- odin::odin(path_seirds_model, verbose = FALSE, skip_cache = TRUE)

This comment has been minimized.

@richfitz

richfitz Feb 22, 2018
Member

drop the skip_cache

This comment has been minimized.

@thibautjombart

thibautjombart Feb 28, 2018
Author Contributor

done at 6529993

Does it mean it will be default behaviour?

@thibautjombart
Copy link
Contributor Author

@thibautjombart thibautjombart commented Feb 28, 2018

@richfitz all changes done as of 6529993
Should be ready to merge into i100... and then into master?

thibautjombart and others added 2 commits Feb 28, 2018
Primarily style and formatting
@richfitz richfitz merged commit c2decd2 into i100_multinom Mar 1, 2018
0 of 4 checks passed
0 of 4 checks passed
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@richfitz richfitz deleted the doc_multinom branch Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.