Skip to content
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

Mkw hiv mort #26

Merged
merged 36 commits into from
Aug 18, 2023
Merged

Mkw hiv mort #26

merged 36 commits into from
Aug 18, 2023

Conversation

mwalte10
Copy link
Collaborator

  • Integrated the HIV natural history bit for children off treatment
  • changed all instances of hC to hc per jeff's comment in leapfrog check in on 25/7
  • made all paed model functions have 'child' as the second word to indicate that they are paediatric functions
  • removed all instances of hard coding related to age
  • separated hc1 from hc2 for output of the hiv stratified population

@mwalte10 mwalte10 requested review from jeffeaton and r-ash July 27, 2023 11:49
Copy link
Collaborator

@jeffeaton jeffeaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Adding a couple of small comments.

Please merge #11 into this branch and then re-review.

inst/include/child_model_simulation.hpp Outdated Show resolved Hide resolved
inst/include/child_model_simulation.hpp Outdated Show resolved Hide resolved
inst/include/child_model_simulation.hpp Outdated Show resolved Hide resolved
Comment on lines 105 to 107
//Not sure how to do just a single value, ASK ROB
// !!! TODO: Ask Rob about preferred approach to assigning a single value
const double ctx_effect = parse_data<double>(data, "ctx_effect", 1)(0); // select the first element of a TensorMap1 and assign to double
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@r-ash -- please review this. Need to set convention for how to parse a single value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added support into the code generation to manage this.

@mwalte10 mwalte10 requested a review from jeffeaton August 2, 2023 09:31
@mwalte10
Copy link
Collaborator Author

@r-ash here is the problem with the intermediate data

@r-ash
Copy link
Contributor

r-ash commented Aug 17, 2023

Not sure about the code factor issues here, it is complaining about empty lines but is seemingly referencing an old commit. Those blank lines have been removed.

@@ -58,6 +58,7 @@ void save_output(leapfrog::StateSaver<ModelVariant, double> &state_saver,
int main(int argc, char *argv[]) {
if (argc < 4) {
std::cout <<

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@@ -107,6 +108,7 @@ int main(int argc, char *argv[]) {
ss.hAG, // Age groups HIV 15+
1, // Scale CD4 mortality
0.2, // initiation_mortality_weight

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change


for (int g = 0; g < adult_ss.NS; ++g) {
for (int s = 0; s < ss.NS; ++s) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good spot, we should switch elsewhere loop variable to s, I think it might still be g in a lot of places. Doesn't need to be done in this PR though.

Comment on lines 105 to 107
//Not sure how to do just a single value, ASK ROB
// !!! TODO: Ask Rob about preferred approach to assigning a single value
const double ctx_effect = parse_data<double>(data, "ctx_effect", 1)(0); // select the first element of a TensorMap1 and assign to double
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added support into the code generation to manage this.

Comment on lines 22 to 26
for (int s = 0; s < ss.NS; ++s) {
//less than 5 because there is a cd4 transition between ages 4 and 5
for (int a = 1; a < hc_ss.hc2_agestart; ++a) {
for (int hd = 0; hd < hc_ss.hc1DS; ++hd) {
for (int cat = 0; cat < hc_ss.hcTT; ++cat) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loop order here should be NS, hc1AG, hcTT, hc1DS

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.39% 🎉

Comparison is base (939c4d3) 97.88% compared to head (5eb3228) 98.28%.

❗ Current head 5eb3228 differs from pull request most recent head b2f7a47. Consider uploading reports for the commit b2f7a47 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   97.88%   98.28%   +0.39%     
==========================================
  Files          18       18              
  Lines        1087     1222     +135     
==========================================
+ Hits         1064     1201     +137     
+ Misses         23       21       -2     
Files Changed Coverage Δ
R/run_model.R 91.30% <ø> (ø)
inst/include/frogger.hpp 100.00% <ø> (ø)
R/generate_cpp.R 100.00% <100.00%> (ø)
inst/include/child_model_simulation.hpp 100.00% <100.00%> (ø)
inst/include/hiv_demographic_projection.hpp 100.00% <100.00%> (ø)
inst/include/model_input.hpp 100.00% <100.00%> (ø)
inst/include/model_output.hpp 100.00% <100.00%> (ø)
inst/include/state_saver.hpp 94.31% <100.00%> (+0.89%) ⬆️
inst/include/types.hpp 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@r-ash r-ash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, there are a few bits of commented out code which I would be tempted to remove if they're not needed anymore, or add a comment about why they are commented out.

for (int hd = 0; hd < hc_ss.hc1DS; ++hd) {
intermediate.children.hc_posthivmort(hd, cat, a, s) =
state_next.children.hc1_hiv_pop(hd, cat, a, s) * (1 - cpars.hc1_cd4_mort(hd, cat, a));
//intermediate.children.hc_posthivmort(hd, cat, a, s) = state_next.children.hc1_hiv_pop(hd, cat, a, s) - (1.0 - cpars.ctx_effect * cpars.ctx_val(time_step)) * state_curr.children.hc1_hiv_pop(hd, cat, a, s) * cpars.hc1_cd4_mort(hd, cat, a);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needed at all?

@mwalte10 mwalte10 merged commit 2733a03 into main Aug 18, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants