-
Notifications
You must be signed in to change notification settings - Fork 0
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
Set-up temperature-R0 code and vignette #3
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.
Nice work @EmilieFinch! I've left a few inline comments in the files. Overall, it's looking good and is a solid foundation to develop from.
Let me know if you're unable to fix any of the failing checks and I'm happy to help out.
#' CxpiWNVxl; CxtaWEEV; CxtaWNVx; CxunWNVx | ||
#'@return numeric vector of relative R0 values corresponding to input temperature values | ||
#'@export | ||
temperature_r0 <- function(data, vector_pathogen){ |
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.
Will need an example (using @examples
) at some point as it's an exported function, but this can be added later.
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.
Fab, have added example here.
R/temperature_r0.R
Outdated
|
||
#' @description This function takes an input series of mean temperature values in °C | ||
#' and returns estimated relative R0 values according to temperature-R0 curves in | ||
#' from Mordecai et al 2019 https://doi.org/10.1111/ele.13335 |
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.
There is also the @references
tag from Roxygen to add full references to function documentation.
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.
Have moved reference to the @references
tag, thanks!
R/temperature_r0.R
Outdated
if(!vector_pathogen %in% vector_reference$vector_pathogen){ | ||
message("Vector-pathogen code is not recognised. This should be one of: | ||
AeaeDENV; AeaeZIKV; AealDENV; AetaRVFV; | ||
AetaSINV; AetrEEEV; AngaPfal; CxanMVEx; | ||
CxanRRVx; CxpiSINV; CxpiWNVxl; CxtaWEEV; | ||
CxtaWNVx; CxunWNVx.") |
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.
This can probably be replaced by match.arg()
(which is a function from base R).
It comes with a couple of features that you may not want or differ from the current code.
- It defaults to the first element in the options vector.
- It errors if the user gives an string not in the options vector, whereas currently it's a message.
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.
I've switched this to match.arg()
but added a nicer error message with a tryCatch()
R/temperature_r0.R
Outdated
r0_posteriors <- eval(parse(text = paste0(vector_pathogen, "_R0"))) | ||
constant_temps <- eval(parse(text = paste0(vector_pathogen, "_T"))) |
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.
I think it would be good to rewrite this without the use of eval(parse())
as this can be a code vulnerability, especially if what is being evaluated is a user-defined input. We can discuss what these lines do and what the best alternative is.
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.
Yes makes sense - I've moved the data from data
to inst/extdata
which has solved this issue as it's now called with system.file()
R/temperature_r0.R
Outdated
|
||
# Wrangle posteriors | ||
|
||
tr0 <- r0_posteriors |> |
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.
It may be better to rewrite this without the pipe (|>
) as chained operations with the pipe can make functions harder to debug and by using |>
you impose a minimum version of R on the package users of 4.1.0. As you're using {dplyr} then you can switch to %>%
without taking on any other dependencies.
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.
Have amended, thanks!
@@ -0,0 +1,14 @@ | |||
#' Fiji 2014. |
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.
The other data sets will also need to be documented to remove the warnings from R CMD check.
@@ -7,6 +7,3 @@ | |||
# * https://testthat.r-lib.org/articles/special-files.html | |||
|
|||
library(testthat) | |||
library(packagetemplate) | |||
|
|||
test_check("packagetemplate", stop_on_warning = FALSE) |
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.
I assume you removed this to stop the tests failing? I think it would be better to not remove it and replace packagetemplate
with climateR0
so that any unit tests you write in the future will be automatically run.
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.
Yes makes sense, have updated accordingly.
vignettes/temperature_R0.Rmd
Outdated
) | ||
``` | ||
|
||
Temperature is an important driver of vector-borne disease transmission, affecting vector reproduction, development, and survival, as well as the probability of pathogen transmission. Previous work by [Mordecai and colleagues](https://onlinelibrary.wiley.com/doi/10.1111/ele.13335) empirically estimated the effect of temperature on different vector traits, and used these to develop models of temperature dependent R~0~. |
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.
The reference can be added using a bibliography file and automatically render the reference list (similar to LaTeX). You don't need to do this in this PR, but could be a nice enhancement in the future.
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.
I'll leave this for now but would be nice to add in the future.
This PR adds preliminary package functionality including:
temperature_r0
function which takes a time-series of temperature data and a specified vector-pathogen combination as inputs and returns median posterior estimates of relative R0