-
Notifications
You must be signed in to change notification settings - Fork 19
1294 add LCT SECIR model with two disease strains #1340
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
base: main
Are you sure you want to change the base?
Conversation
started changing Infection States, Parameters etc. to reflect 2 diseases
changed Infection States, Parameters etc. to reflect 2 diseases
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1340 +/- ##
==========================================
+ Coverage 97.26% 97.33% +0.07%
==========================================
Files 184 186 +2
Lines 15807 16320 +513
==========================================
+ Hits 15374 15885 +511
- Misses 433 435 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ScalarType tmax = 10; | ||
|
|
||
| // Set Parameters. | ||
| model.parameters.get<mio::lsecir2d::TimeExposed_a>()[0] = 3.; |
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 not sure whether the naming of the parameters and also of infection states is the best choice. Maybe we should use longer but more self-speaking names, like TimeExposedDisease1,... and for infection states something like: InfectedNoSymptomsDisease1Naive and InfectedNoSymptomsDisease1RecoveredDisease2? Or is that too long? @annawendler what do you think?
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 agree that the infection states are not that self-speaking but I also find the more explaining names really long and quite hard to read. I couldn't come up with a good compromise so I would go with the shorter names.
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.
Regarding names of infection states, I thought of renaming Recovered_a to Recovered_1a and Recovered_ab to recovered_2ab to make it more explicit how many infections the individuals have gone through.
annawendler
left a comment
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.
First part of review
annawendler
left a comment
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.
In a recent commit the use of floating point types was reworked to allow for automatic differentiation. This also affected the LCT-SECIR model, e.g. we now have an additional template parameter FP for the model class. Can you adapt your model accordingly so that the models are consistent? If you have any questions just let us know :)
|
@an-jung please use capitalization in github for shorthand notation such as LCT, SECIR, ... |
annawendler
left a comment
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.
Remaining part of review. Looks good in general! The interact() function was a bit unclear to me, see questions below. Otherwise my comments are mainly regarding documentation
annawendler
left a comment
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.
Looks good to me! :) I only have minor comments regarding naming of some variables.
| "The template parameters Group1 & Group2 should be valid."); | ||
| using LctStateGroup1 = type_at_index_t<Group1, LctStates...>; | ||
| using LctStateGroup2 = type_at_index_t<Group2, LctStates...>; | ||
| FP InfectedNoSymptoms_Group2_a = 0; |
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.
| FP InfectedNoSymptoms_Group2_a = 0; | |
| FP infectedNoSymptoms_group2_a = 0; |
Can you adjust the naming like this in the interact() function? According to our coding guidelines (https://memilio.readthedocs.io/en/latest/development.html#coding-guidelines) variables should use small letters. I know this isn't done 100% consistently throughout the code (such as N oder Ri_a) but here it also helps with consistency within the code of your model.
| LctStateGroup2::template get_num_subcompartments<InfectionState::InfectedSymptoms_2b>()) | ||
| .sum(); | ||
| // Size of the subpopulation Group2 without dead people. | ||
| FP N_Group2 = pop.segment(first_index_group2, LctStateGroup2::Count).sum(); // Sum over all compartments. |
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.
| FP N_Group2 = pop.segment(first_index_group2, LctStateGroup2::Count).sum(); // Sum over all compartments. | |
| FP N_group2 = pop.segment(first_index_group2, LctStateGroup2::Count).sum(); // Sum over all compartments. |
See above
| N_Group2 = N_Group2 - pop.segment(LctStateGroup2::template get_first_index<InfectionState::Dead_a>(), 1).sum() - | ||
| pop.segment(LctStateGroup2::template get_first_index<InfectionState::Dead_b>(), 1) | ||
| .sum(); // All people minus dead people. | ||
| const FP div_N_Group2 = (N_Group2 < Limits<FP>::zero_tolerance()) ? 0.0 : 1.0 / N_Group2; |
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.
| const FP div_N_Group2 = (N_Group2 < Limits<FP>::zero_tolerance()) ? 0.0 : 1.0 / N_Group2; | |
| const FP div_N_group2 = (N_Group2 < Limits<FP>::zero_tolerance()) ? 0.0 : 1.0 / N_Group2; |
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
If need be, add additional information and what the reviewer should look out for in particular:
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)
Closes #1294