-
Notifications
You must be signed in to change notification settings - Fork 1
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
Births to WLHIV #39
Births to WLHIV #39
Conversation
…the 15 entrants to align. still need to align births. think it is something with art births
Codecov ReportAttention:
... and 1 file with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
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 looks great, couple of small suggestions but they are fairly inconsequential. I'm approving so not blocked on me whilst I am away next week but would be worth a review from science side before merging this.
@@ -28,7 +28,7 @@ paed_cd4_prog,hc1_cd4_prog,real_type,ModelVariant::run_child_model,,FALSE,1,chil | |||
adol_cd4_prog,hc2_cd4_prog,real_type,ModelVariant::run_child_model,,FALSE,1,children.hc2DS,,, | |||
ctx_effect,ctx_effect,real_type,ModelVariant::run_child_model,,FALSE,1,1,,, | |||
ctx_val,ctx_val,real_type,ModelVariant::run_child_model,,FALSE,1,proj_years,,, | |||
paed_art_elig_age,hc_art_elig_age,real_type,ModelVariant::run_child_model,,FALSE,1,proj_years,,, | |||
paed_art_elig_age,hc_art_elig_age,int,ModelVariant::run_child_model,,FALSE,1,proj_years,,, | |||
paed_art_elig_cd4,hc_art_elig_cd4,real_type,ModelVariant::run_child_model,,TRUE,2,options.p_idx_hiv_first_adult,proj_years,, |
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.
Chatted about this but I think worth making this a int
whilst you're here
@@ -99,24 +121,40 @@ void run_hiv_and_art_stratified_ageing(int time_step, | |||
} | |||
|
|||
// TODO: add HIV+ 15 year old entrants see https://github.com/mrc-ide/leapfrog/issues/8 |
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.
Can this TODO be removed now?
} | ||
} | ||
} | ||
} | ||
} | ||
// ADD HIV+ entrants here |
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.
Do you want to put this as a TODO?
With syntax // TODO:
then gives a consistent thing we can search for later to review remaining work
@@ -1,6 +1,7 @@ | |||
test_that("child model can be run for all years", { | |||
demp <- readRDS(test_path("testdata/demographic_projection_object_child.rds")) | |||
parameters <- readRDS(test_path("testdata/projection_parameters_child.rds")) | |||
parameters$laf = 1 |
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.
What is this variable? Do you want to set it in the stored parameters rds?
Co-authored-by: Rob <39248272+r-ash@users.noreply.github.com>
Co-authored-by: Rob <39248272+r-ash@users.noreply.github.com>
Co-authored-by: Rob <39248272+r-ash@users.noreply.github.com>
Co-authored-by: Rob <39248272+r-ash@users.noreply.github.com>
@jeffeaton I've added you to look through the methods per Rob's comment of approval from the technical side. I'm incorporating his suggestions but let me know if you have any others. |
Calculates the number of births to WLHIV, needed for MTCT. Please review the 15 year old entrants bit that has changed in the demographic model in particular to make sure that won't cause any issues.
Included births in test child file to make sure it is never negative.