-
Notifications
You must be signed in to change notification settings - Fork 14
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 hypnozoites to the P.v model #321
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.
Hi Richard,
A few Qs to followup on for this one, alongside some suggestions 🌟
R/biting_process.R
Outdated
bitten_humans$insert(bitten) | ||
if(parameters$parasite == "vivax"){ | ||
# p.v must pass through the number of bites each individual receives | ||
n_bites_each <- table(bitten) |
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.
Does using table here work for when someone doesn't get sampled in the fast_weighted_sample()
call above?
For example:
t1 <- malariasimulation:::fast_weighted_sample(20, c(0.8, 0.001, 0.3))
table(t1)
Person index 2 doesn't show up. It wasn't clear to me when n_bites_each
is used downstream in human_infection.R
how this is dealt with?
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 tabulate(bitten, nbins = length(lambda))
is an efficient way to do this if the above is an issue
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.
So I was getting around that by passing through the full table - with column names indicating the individuals that had been bitten. But I think using tabulate can make lots of things a bit simpler. I've had another go with this. Let me know what you think. I've tried to use bitsets where I can.
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.
...a few more changes suggested
R/biting_process.R
Outdated
n_bites_each <- table(bitten) | ||
# p.v must pass through the number of bites per person | ||
bites_per_person <- tabulate(bitten, nbins = length(lambda)) | ||
bitten_humans <- individual::IntegerVariable$new(initial_values = bites_per_person) |
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 see how this seems tidier, but I have a few concerns.
- We now have a single variable
bitten_humans
that means different things for Pf and Pv and ... - that leads to a mismatch in the
calculate_infections()
function, which takes the argumentbitten_humans()
currently defined as a bitset of bitten humans.
Would it be clearer to:
- Create the bitset of
bitten_humans
based onn_bites
andlambda
for both Pf and Pf and pass this through tocalculate_infections()
as an argument. - Additionally create a
IntegerVariable
for vivax called and storingn_bites
. This can be accessed incalculate_infections()
directly in that function as you're already passingvariables
as an argument.
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 given this a go (and rebased) and I think it's ready for review.
c892bc3
to
29b1fed
Compare
9c143ac
to
5f5260a
Compare
29b1fed
to
39949ff
Compare
4b38d43
to
1916fa1
Compare
39949ff
to
b8cce39
Compare
1916fa1
to
b481153
Compare
b8cce39
to
3472e76
Compare
b481153
to
42083f2
Compare
bfe4c0a
to
cb35f28
Compare
42083f2
to
e2adee2
Compare
/benchmark |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e2adee2 is merged into dev:
Further explanation regarding interpretation and methodology can be found in the 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.
Functionally looks fine to me. I have suggested some cleaner ways to organise the code and the competing hazard stuff.
R/human_infection.R
Outdated
if(parameters$parasite == "falciparum"){ | ||
source_humans <- variables$state$get_index_of(c('S','A','U'))$and(bitten_humans) | ||
} else if (parameters$parasite == "vivax"){ | ||
## source_humans must include individuals with hypnozoites which may be impacted by prophylaxis/vaccination | ||
hypnozoites_humans <- variables$hypnozoites$get_index_of(set = 0)$not(T) | ||
source_humans <- bitten_humans$copy()$or(hypnozoites_humans) | ||
} |
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.
if(parameters$parasite == "falciparum"){ | |
source_humans <- variables$state$get_index_of(c('S','A','U'))$and(bitten_humans) | |
} else if (parameters$parasite == "vivax"){ | |
## source_humans must include individuals with hypnozoites which may be impacted by prophylaxis/vaccination | |
hypnozoites_humans <- variables$hypnozoites$get_index_of(set = 0)$not(T) | |
source_humans <- bitten_humans$copy()$or(hypnozoites_humans) | |
} | |
if(parameters$parasite == "falciparum"){ | |
source_humans <- variables$state$get_index_of(c('S','A','U'))$and(bitten_humans) | |
} else if (parameters$parasite == "vivax"){ | |
# source_humans must include individuals with hypnozoites which may be impacted by prophylaxis/vaccination | |
hypnozoites_humans <- variables$hypnozoites$get_index_of(set = 0)$not(T) | |
source_humans <- bitten_humans$copy()$or(hypnozoites_humans) | |
} |
R/human_infection.R
Outdated
@@ -48,91 +51,124 @@ simulate_infection <- function( | |||
#' @description Infection rates are stored in the infection outcome competing hazards object | |||
#' @param variables a list of all of the model variables | |||
#' @param bitten_humans bitset of bitten humans | |||
#' @param n_bites_per_person vector of number of bites each person receives (p.v only) | |||
#' @param parameters model parameters | |||
#' @param renderer model render object | |||
#' @param timestep current timestep | |||
#' @noRd | |||
calculate_infections <- function( |
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.
[optional] This is bordering on being cleaner as 4 functions: calculate_falciparum_infections
, calculate_vivax_infections
and pev.R:pev_efficacy
and treatment_efficacy
.
I only say this because the if (parasite==...)
conditions do make this function long and trickier to follow
R/human_infection.R
Outdated
@@ -145,14 +181,16 @@ calculate_infections <- function( | |||
#' @param renderer model render object | |||
#' @param parameters model parameters | |||
#' @param prob vector of population probabilities of infection | |||
#' @param relative_rates relative rates of hypnozoite relapse relative to total infection rate | |||
#' @noRd | |||
infection_outcome_process <- function( |
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.
Again, this function is quite long. It would be cleaner as falciparum_infection_outcome
and vivax_infection_outcome
.
R/processes.R
Outdated
prob = rate_to_prob(infection_outcome$rates), | ||
relative_rates = infection_outcome$relative_rates) |
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 only just realised how ugly and genius this is...
I don't yet know how I feel about it
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 the above edit to CompetingOutcome is a better way to deal with optional arguments for the processes. That way it generalises to different competing hazard scenarios and you don't have to continue adding fields to the R6 object for more arguments.
R/competing_hazards.R
Outdated
set_rates = function(target, rates){ | ||
stopifnot(target$size() == length(rates)) | ||
|
||
self$target$copy_from(target) | ||
self$rates <- rates | ||
}, |
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 a more readable, generalisable - and perhaps less error prone - method would be to explicity accept extra arguments for your outcome when you set rates.
set_rates = function(target, rates){ | |
stopifnot(target$size() == length(rates)) | |
self$target$copy_from(target) | |
self$rates <- rates | |
}, | |
set_rates = function(target, rates, ...){ | |
stopifnot(target$size() == length(rates)) | |
self$target$copy_from(target) | |
self$rates <- rates | |
self$args <- list(...) | |
}, |
R/competing_hazards.R
Outdated
set_relative_rates = function(target, relative_rates){ | ||
stopifnot(target$size() == length(relative_rates)) | ||
|
||
self$target$copy_from(target) | ||
self$relative_rates <- relative_rates | ||
}, |
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.
set_relative_rates = function(target, relative_rates){ | |
stopifnot(target$size() == length(relative_rates)) | |
self$target$copy_from(target) | |
self$relative_rates <- relative_rates | |
}, |
R/competing_hazards.R
Outdated
}, | ||
target = NULL, | ||
rates = NULL | ||
rates = NULL, | ||
relative_rates = NULL |
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.
relative_rates = NULL | |
args = NULL |
R/competing_hazards.R
Outdated
|
||
self$target$copy_from(target) | ||
self$relative_rates <- relative_rates | ||
}, | ||
execute = function(t, target){ | ||
private$targeted_process(t, target) |
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.
private$targeted_process(t, target) | |
do.call(private$targeted_process, c(c(t, target), self$args) |
R/processes.R
Outdated
@@ -96,7 +98,8 @@ create_processes <- function( | |||
targeted_process = function(timestep, target){ |
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.
targeted_process = function(timestep, target){ | |
targeted_process = function(timestep, target, prob, relative_rates){ |
R/human_infection.R
Outdated
## capture infection rates to resolve in competing hazards | ||
infection_outcome$set_rates( | ||
source_humans, | ||
infection_rates) |
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 edit to CompetingOutcome would allow you to add prob and relative_rates here...
/benchmark |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e2adee2 is merged into dev:
Further explanation regarding interpretation and methodology can be found in the documentation. |
/benchmark |
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.
Very nearly there for me!
R/human_infection.R
Outdated
# infection_outcome$set_relative_rates( | ||
# hypnozoites_humans, | ||
# relative_rates) |
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.
dead code
# infection_outcome$set_relative_rates( | |
# hypnozoites_humans, | |
# relative_rates) |
R/human_infection.R
Outdated
@@ -685,7 +944,7 @@ severe_immunity <- function(age, acquired_immunity, maternal_immunity, parameter | |||
parameters$theta0 * (parameters$theta1 + (1 - parameters$theta1) / ( | |||
1 + fv * ( | |||
(acquired_immunity + maternal_immunity) / parameters$iv0) ** parameters$kv | |||
) | |||
) |
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.
) | |
) |
R/competing_hazards.R
Outdated
}, | ||
execute = function(t, target){ | ||
private$targeted_process(t, target) | ||
do.call(private$targeted_process, list(t, target, self$args)) |
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.
Oh, I think it's most intuitive to call targeted_process
with the extra arguments in the same way they were passed to $set_rates
. i.e.
outcome$set_rates(rates, a, b)
should call the process with:
targeted_process(target, a, b)
<- this version preserves the structure of my extra arguments
not
targeted_process(target, list(a, b))
<- this version turns my extra arguments into a list.
do.call(private$targeted_process, list(t, target, self$args)) | |
do.call(private$targeted_process, c((t, target), self$args)) |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3b833ff is merged into dev:
Further explanation regarding interpretation and methodology can be found in the documentation. |
/benchmark |
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!
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 7166cba is merged into dev:
Further explanation regarding interpretation and methodology can be found in the 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.
Thanks for the hard work @RJSheppard!
One non-critical clarification from me
#' @param renderer model renderer | ||
#' @param timestep current timestep | ||
#' @noRd | ||
relapse_bite_infection_hazard_resolution <- function( |
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'm probably just not following, but given the name of this function I was expecting to see some referecnce to the competing hazards object within it, but can'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.
The main CH resolution is between infections and disease progression. This part of the code resolves between relapses and bite infections and doesn't use the competing hazards object. It's relatively simple and discretely resolved, so doesn't need the object.
…ctions, decay of hypnozoite batches, new infections via relapse and the reseting of batch number to 0 for new births. A hypnozoite batch variable is created. The vivax model considers the number of new bites an individual receives (in contrast to whether an individual has been bitten regardless of number of bites, as currently in the P. falciparum model) to calculate the infection rates. This is now captured in bitten_humans, which is now a list that also includes n_bites_each. The vivax model does not factor the eir by the population level biting heterogeneity (although it does use the heterogeneity to distribute biting, but retains the overall total biting according to the EIR). biting_rate.R In vivax, we track bites in all individuals, not just SAU as in falciparum. This is because new hypnozoite batches can occur in any human state, regardless of whether this results in a new infection. The number of bites is considered in calculating probability of a new infection. To calculate the infection rates for vivax, we take the biting infection rates and add them to the relapse rate. We also save the relative rates in the competing hazards object to resolve these competing hazards later. A new function to resolve between bites and relapse infections is created. For each bite we increase the number of batches - with capacity for prophylaxis to be added at a future step. We also cap the total number of batches. We add a hypnozoite batch decay process. Note that this process differs from the immunity decay processes. We work in discrete outputs (you can't have a fractional batch). We also work in the probability that an individual experiences a single loss, rather than loss at the hypnozoite level. When an individual dies, we reset the hypnozoite batches back to 0. We now render number of relapses, average hypnozoite batch number and the number of individuals with hypnozoite batches. We may also output these by specified age groups. Added a test to check relapses occur as expected and expanded the p.v vignette.
… within the function. This also removed the "set_relative_rates" part of the competing hazards, and now we have args, such that the competing hazards object can still store the relative rates it needs, but retains flexibility. Calculate_infections has been rewritten as four functions: calculate_falciparum_infections, calculate_vivax_infections, treatment_prophylaxis_efficacy and pev_efficacy. Infection_outcome_process has been split into falciparum and vivax _infection_outcome_process. Tests have been adjusted to reflect these changes.
…peting hazards object and reverting to previous competing hazard tests.
7166cba
to
da381b9
Compare
/benchmark |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if da381b9 is merged into dev:
Further explanation regarding interpretation and methodology can be found in the documentation. |
We must model hynozoite batch formation via bite infections, decay of hypnozoite batches through, new infections via relapse and the reseting of batch number to 0 for new births.
We create a hypnozoite variable.
The vivax model considers the number of new bites an individual receives (in contrast to whether an individual has been bitten regardless of number of bites) to calculate the infection rates. This is now captured in bitten_humans, which is now a list that also includes n_bites_each.
The vivax model does not factor the eir by the population level biting heterogeneity (although it does use the heterogeneity to distribute biting, but retains the overall total biting according to the EIR). biting_rate.R
In vivax, we track bites in all individuals, not just SAU as in falciparum. This is because new hypnozoite batches can occur in any human state, regardless of whether this results in a new infection.
The number of bites is considered in calculating probability of a new infection.
To calculate the infection rates for vivax, we take the biting infection rates and add them to the relase rate. We also save the relative rates in the competing hazards object to resolve these competing hazards later.
A new function to resolve between bites and relapse infections is created. For each bite we increase hte number of batches - with capacity for prophylaxis to be added at a future step. We also cap the total number of batches.
We add a hypnozoite batch decay process. Note that this process differs from the immunity decay processes. We work in discrete outputs (you can't have a fractional batch). We also work in the probability that an individual experiences a single loss, rather than loss at the hypnozoite level.
When an individual dies, we reset the hypnozoite batches back to 0.
We now render number of relapses, average hypnozoite batch number and the number of individuals with hypnozoite batches. We may also output these by specified age groups.
Added a test to check relapses occur as expected.