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
mrc-2432 Implement optimiser to strategize across regions #57
Conversation
Codecov Report
@@ Coverage Diff @@
## master #57 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 6 +1
Lines 375 409 +34
=========================================
+ Hits 375 409 +34
Continue to review full report at Codecov.
|
DESCRIPTION
Outdated
thor | ||
thor, | ||
dplyr, | ||
tibble, |
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 is a dependency of dplyr but seemingly needs to be specified explicitly here for make check
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 usually put those things in Suggests (and typically order these lists alphabetically as they're easier to maintain that way)
R/optimise.R
Outdated
@@ -0,0 +1,44 @@ | |||
library(dplyr, warn.conflicts = FALSE) | |||
|
|||
utils::globalVariables(c("zone", "intervention", "total_costs", "total_cases_averted", "value", "i", "j")) |
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.
Tried to avoid this using .data$
but it got very verbose, and I still had trouble eliminating all the warnings
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 thing you can do is to scope these variables within the function so do:
function(...) {
zone <- intervention <- NULL # used by dplyr
df %>% select(zone) # etc
}
There's no great way of doing this though. The .data$
trick is the canonical way but it's not nice to read
tests/testthat/test-optimise.R
Outdated
@@ -0,0 +1,43 @@ | |||
library(ROI.plugin.glpk) |
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.
Not sure of the correct way to do this - it's only required for its side-effects (i.e. loading the plugin), and apparently needs to be done in the context of where the code is invoked, not defined (i.e. it isn't sufficient to put this line in optimise.R
.
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 should be done by adding
##' @import ROI.plugin.glpk
somewhere near in R/optimise.R and rerun devtools::document()
so that NAMESPACE
contains an import call. That will mean that mintr can find the plugin
DESCRIPTION
Outdated
thor | ||
thor, | ||
dplyr, | ||
tibble, |
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 usually put those things in Suggests (and typically order these lists alphabetically as they're easier to maintain that way)
R/optimise.R
Outdated
@@ -0,0 +1,44 @@ | |||
library(dplyr, warn.conflicts = 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.
There should be no calls to library()
within package code. I presume this is working around something with dplyr's DSL? The import
that you have in the NAMESPACE should sort this?
R/optimise.R
Outdated
@@ -0,0 +1,44 @@ | |||
library(dplyr, warn.conflicts = FALSE) | |||
|
|||
utils::globalVariables(c("zone", "intervention", "total_costs", "total_cases_averted", "value", "i", "j")) |
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 thing you can do is to scope these variables within the function so do:
function(...) {
zone <- intervention <- NULL # used by dplyr
df %>% select(zone) # etc
}
There's no great way of doing this though. The .data$
trick is the canonical way but it's not nice to read
tests/testthat/test-optimise.R
Outdated
@@ -0,0 +1,43 @@ | |||
library(ROI.plugin.glpk) |
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 should be done by adding
##' @import ROI.plugin.glpk
somewhere near in R/optimise.R and rerun devtools::document()
so that NAMESPACE
contains an import call. That will mean that mintr can find the plugin
This refactors Dirk's code as a function to be called by the optimise endpoint (which will be implemented in mrc-2433)
Any feedback whatsoever on my novice R code very welcome. Biggest challenge was getting dplyr to pass
make check
- ideas for tidying this up without making it unreadable gratefully received. We inherited dplyr and could probably do without it but would take me a while to translate into pure R...