Conversation
|
Hi @richfitz, do you have time for this PR this week? |
richfitz
left a comment
richfitz
left a comment
There was a problem hiding this comment.
I had got part way through this last week - can you look at the comments below and we'll get this done this week
R/modified_update.R
Outdated
| ##' | ||
| ##' @export | ||
| modified_update_calculate <- function(con, touchstone_name_mod, touchstone_use) { | ||
| modified_update_calculate <- function(con, touchstone_name_mod, touchstone_use, method = "method2", version = "v2") { |
There was a problem hiding this comment.
This is a bit confusing with method and version both containing version numbers and version being numeric but stored as a character. Can you write a list of the versions somewhere so we can see how this might be simplified?
There was a problem hiding this comment.
OK. Documented in the beginning of this function.
There was a problem hiding this comment.
Sorry, I should have been clearer: version = "v2" is pretty confusing - if you want to have a numeric version and numeric methods, why not go with version = 2? And is method2/version2 not more reasonably just method = "method3" which could also be spelt as method = 3?
There was a problem hiding this comment.
The thing is that we are already talking about method 3 elsewhere (although not yet developed). Shall we go with method = 2 and version = 2 ?
| touchstone_mod <- DBI::dbGetQuery(con, sql, touchstone_name_mod) | ||
| i <- touchstone_mod$id != '201510gavi-42' | ||
| touchstone_mod <- touchstone_mod[which.max(touchstone_mod$version[i]), ] | ||
| i <- touchstone_mod$id != '201510gavi-42' |
There was a problem hiding this comment.
Can you document the significance of this touchstone?
There was a problem hiding this comment.
Documented above this line of code.
R/modified_update.R
Outdated
| dat <- merge_in(dat, mu_fix_coverage(d_cov_new), | ||
| c(coverage_new = "coverage", | ||
| coverage_target_new = "target", | ||
| gavi_support_new = "gavi_support")) |
There was a problem hiding this comment.
not sure if this is a diff artefact but this closing brace being missing in the new version seems surprising (use side-by-side diff, ignoring whitespace)
There was a problem hiding this comment.
The closing brace } is not missing, but moved to the line before dat <- merge)in(...). It is not making any difference actually.
R/modified_update.R
Outdated
|
|
||
| ret <- fvps_new * rate_tot_old | ||
| mu_scale <- function(name, d, method = "method2") { | ||
| ## This chunck becomes simpler, since only method 2 is used throughout. - Not any more, we need method 1 now. |
There was a problem hiding this comment.
can you rewrite this comment to describe only the new code?
R/modified_update.R
Outdated
|
|
||
| meta$data <- data | ||
|
|
||
| if(version == "v2") { |
There was a problem hiding this comment.
sorry to be pedantic, but can you keep a space between if and ( here and elsewhere (similarly for for, etc)
There was a problem hiding this comment.
Done here and else where.
|
I have done detailed checking. It is not possible to deal with |
|
Done. |
2 participants
Youtrack:
https://vimc.myjetbrains.com/youtrack/issue/VIMC-1738
Task:
This PR has two major tasks.
Tested:
I will double test the changes made in this PR with a few orderly reports before coming to you.