-
Notifications
You must be signed in to change notification settings - Fork 20
1393 improve parameters io for graph ode based models #1430
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?
1393 improve parameters io for graph ode based models #1430
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1430 +/- ##
==========================================
- Coverage 97.31% 97.30% -0.01%
==========================================
Files 187 187
Lines 16018 15820 -198
==========================================
- Hits 15588 15394 -194
+ Misses 430 426 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Great addition :)
Most of my comments are minor. When fixing them, please keep in mind that they often affect other models as well.
What still concerns me yet is the new h5 file. Have you tried with h5dump to see exactly what has changed? And to what extend?
|
|
||
| StateId get_state_id(int county) | ||
| { | ||
| // integer division |
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.
| // integer division |
|
|
||
| struct EpidataFilenames { | ||
| private: | ||
| EpidataFilenames(std::string& pydata) |
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.
| EpidataFilenames(std::string& pydata) | |
| EpidataFilenames(const std::string& pydata) |
same for the others below
| namespace de | ||
| { | ||
|
|
||
| struct EpidataFilenames { |
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.
add a documentation above the struct
|
|
||
| s.case_data_path = mio::path_join(pydata, "cases_all_state_age_ma7.json"); | ||
| s.divi_data_path = mio::path_join(pydata, "state_divi_ma7.json"); | ||
| s.vaccination_data_path = mio::path_join(pydata, "vacc_state_ageinf_ma7.json"); |
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 vaccination data is the same for states and county. intended? if yes, add a comment.
| { | ||
| } | ||
|
|
||
| public: |
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.
overall, this is all very static. At least the moving average should be adaptive.
use sth like:
std::string ma_suffix = "_ma" + std::to_string(moving_average) + ".json";
in combination with default moving_average=7.
| { | ||
| BOOST_OUTCOME_TRY(auto&& case_data, mio::read_confirmed_cases_data(path)); | ||
|
|
||
| // sort case_data into regions and ignore once with no region associated |
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.
| // sort case_data into regions and ignore once with no region associated | |
| // sort case_data into regions and ignore ones with no region associated |
please also correct in sceir and secirvvs
| } | ||
| model.populations[{AgeGroup(i), InfectionState::SusceptibleImprovedImmunity}] = num_rec[i]; | ||
| if (set_death) { | ||
| // in set_confirmed_cases_data initilization, deaths are now set to 0. In order to visualize |
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 set_confirmed_cases_data initilization, deaths are now set to 0. In order to visualize | |
| // in set_confirmed_cases_data initialization, deaths are now set to 0. In order to visualize |
also use search + replace
| ASSERT_EQ(vacc_data[1].num_vaccinations_refreshed_additional, 3.0); | ||
| } | ||
|
|
||
| TEST(TestEpiData, set_vaccination_data) |
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.
why not reuse this test in the secirts test file?
| TEST(TestGraph, set_nodes_secir) | ||
| { | ||
|
|
||
| mio::osecir::Parameters<double> params(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.
why delete?
| * @param[in] end_date Date at the end of the simulation. | ||
| */ | ||
| template <class FP, class Model, class ContactPattern> | ||
| void set_german_holidays(const mio::VectorRange<Node<Model>>& nodes, const mio::Date& start_date, |
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 generell, we should try to keep this generic and add a overall function set_holidays which allows a country specifier e.g., for Germany.
however, since we only have data for germany at this moment, this is ok :)
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
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)