1081 rework ABM state transitions #1107

jubicker · 2024-08-13T09:56:11Z

Changes and Information

Please briefly list the changes (main added features, changed items, or corrected bugs) made:

add classes for Exponential, LogNormal and Constant ParameterDistributions
add rng as input argument for get_random_sample method of ParameterDistribution
add ParameterDistributionWrapper class that is used as parameter type for the transition times in the ABM
make the transition times in the ABM ParameterDistributionWrappers such that you can plug in any ParameterDistribution

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

Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
New code adheres to coding guidelines
No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
Tests are added for new functionality and a local test run was successful (with and without OpenMP)
Appropriate documentation for new functionality has been added (Doxygen in the code and Markdown files if necessary)
Proper attention to licenses, especially no new third-party software with conflicting license has been added
(For ABM development) Checked benchmark results and ran and posted a local test above from before and after development to ensure performance is monitored.

Checks by code reviewer(s)

Corresponding issue(s) is/are linked and addressed
Code is clean of development artifacts (no deactivated or commented code lines, no debugging printouts, etc.)
Appropriate unit tests have been added, CI passes, code coverage and performance is acceptable (did not decrease)
No large data files added in the whole history of commits(files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
On merge, add 2-5 lines with the changes (main added features, changed items, or corrected bugs) to the merge-commit-message. This can be taken from the briefly-list-the-changes above (best case) or the separate commit messages (worst case).

coses #1081

…e-state-transitions-in-the-infection

Signed-off-by: DavidKerkmann <[email protected]>

…state-transitions-in-the-infection

…e-state-transitions-in-the-infection

codecov · 2024-08-22T11:31:15Z

Codecov Report

Attention: Patch coverage is 83.19739% with 103 lines in your changes missing coverage. Please review.

Project coverage is 96.30%. Comparing base (f192ec6) to head (be07ec6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cpp/memilio/utils/parameter_distributions.h	66.49%	65 Missing ⚠️
cpp/memilio/utils/parameter_distribution_wrapper.h	69.49%	18 Missing ⚠️
cpp/models/abm/model_functions.cpp	40.00%	9 Missing ⚠️
cpp/models/abm/parameters.h	92.92%	8 Missing ⚠️
cpp/models/abm/infection.cpp	97.08%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1107      +/-   ##
==========================================
- Coverage   96.97%   96.30%   -0.67%     
==========================================
  Files         148      149       +1     
  Lines       13718    14061     +343     
==========================================
+ Hits        13303    13542     +239     
- Misses        415      519     +104

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

…m/SciCompMod/memilio into 1081-rework-abm-state-transitions

DavidKerkmann

Thank you for the implementation!
Apart from some smaller things, we could talk again about the detailed implementation of the distributions, I think @reneSchm will have some more comments and ideas once he has taken a deeper look. Many lines from the param distribtuions are listed as not tested, this could be fixed afterwards.
Related issues that will be closed should be linked correctly. We can have a look together, so that we also know what has to be done next.

DavidKerkmann · 2025-01-09T08:29:34Z

cpp/models/abm/parameters.h

+// Distribution that can be used for the time spend in InfectionStates
+using InfectionStateTimesDistributionsParameters = LogNormalDistribution<double>::ParamType;
+


Suggested change

// Distribution that can be used for the time spend in InfectionStates

using InfectionStateTimesDistributionsParameters = LogNormalDistribution<double>::ParamType;

DavidKerkmann · 2025-01-09T08:32:44Z

cpp/models/abm/parameters.h

+ * @brief Individual virus shed factor to account for variability in infectious viral load spread.
+*/
+struct VirusShedFactor {
+    using Type = CustomIndexArray<UniformDistribution<double>::ParamType, VirusVariant, AgeGroup>;


I'd make this and all other parameters where a fixed distribution is hardcoded also changable through the wrapper.

DavidKerkmann · 2025-01-09T08:36:39Z

cpp/models/ode_secir/parameter_space.h

+                std::min(std::max(min_val, (1 - dev_rel * 2.6) * v), 0.1 * std::numeric_limits<double>::max()),
+                std::min(std::max(min_val, (1 + dev_rel * 2.6) * v), 0.5 * std::numeric_limits<double>::max()),


One could use the definitions from lower_bound and upper_bound from above.

DavidKerkmann · 2025-01-09T08:39:11Z

cpp/models/abm/mobility_rules.cpp

@@ -34,7 +35,7 @@ LocationType random_mobility(PersonalRandomNumberGenerator& rng, const Person& p
    auto make_transition = [current_loc](auto l) {
        return std::make_pair(l, l == current_loc ? 0. : 1.);
    };
-    if (t < params.get<LockdownDate>()) {
+    if (t < params.get<mio::abm::LockdownDate>()) {


Is the mio::abm:: suddenly needed here (and below)?

DavidKerkmann · 2025-01-09T08:51:38Z

cpp/models/abm/parameters.h

 /**
- * @brief Time that a Person is infected but not yet infectious.
+ * @brief Time that a Person is infected but not yet infectious in day unit
 */
 struct IncubationPeriod {


I thought a bit more about the naming and came to the conclusion that IncubationPeriod doesn't really fit, as it usually describes the time from Exposure to the first Symptoms. Therefore, we should rename it. Perhaps ExposedToInfectedNoSymptoms?

DavidKerkmann · 2025-01-09T11:56:53Z

cpp/models/abm/analyze_result.h

@@ -56,139 +57,184 @@ std::vector<Model> ensemble_params_percentile(const std::vector<std::vector<Mode
        auto& new_params = get_param(percentile[n]);
        new_params       = single_element[static_cast<size_t>(num_runs * p)];
    };
+    mio::unused(param_percentile);


Suggested change

mio::unused(param_percentile);

DavidKerkmann · 2025-01-09T12:02:03Z

cpp/models/abm/analyze_result.h

+                        return model.parameters.template get<IncubationPeriod>()[{virus_variant, age_group}];
+                    },
+                    [](auto& dist1, auto& dist2) {
+                        return dist1.params()[0] < dist2.params()[0];


I am not sure if always using the first param is a good idea for ordering in general, but it is a good default case. Perhaps we can create a function for each dist that explains how to order, and then always call this function here?

DavidKerkmann · 2025-01-09T12:17:10Z

cpp/tests/distributions_helpers.cpp

-            EXPECT_THAT(self.get_lower_bound(),
-                        FloatingPointEqual(p_other_uniform_distribution->get_lower_bound(), 1e-12, 1e-12));
-            EXPECT_THAT(self.get_upper_bound(),
-                        FloatingPointEqual(p_other_uniform_distribution->get_upper_bound(), 1e-12, 1e-12));
-


Why are these checks gone? The lower and upper bounds still exist for these distributions.

DavidKerkmann · 2025-01-09T13:52:22Z

cpp/tests/test_abm_model.cpp

+    // Set trips to use weekday trips on weekends.
+    data.use_weekday_trips_on_weekend();


Suggested change

// Set trips to use weekday trips on weekends.

data.use_weekday_trips_on_weekend();

Duplication?

DavidKerkmann · 2025-01-09T13:52:46Z

cpp/tests/test_abm_model.cpp

    // Set trips to use weekday trips on weekends.
    data.use_weekday_trips_on_weekend();

-    // Mock the distribution to prevent infections or state transitions in the test.
+    // Mock the distribution to prevent infectionsin the test.


Suggested change

// Mock the distribution to prevent infectionsin the test.

// Mock the distribution to prevent infections in the test.

DavidKerkmann · 2025-01-10T09:27:44Z

My suggestion is to close
#811 and #664
while
#1081 and #688
include the symptomatic state inclusion/seperation, which is not covered here.

reneSchm · 2025-01-15T10:23:54Z

cpp/memilio/utils/parameter_distribution_wrapper.h

+        if (m_dist == nullptr) {
+            log_error("Distribution is not defined. Parameters cannot be deduced.");
+        }
+        return m_dist->params();


Logging is not enough here, as a nullptr dereference is undefined behaviour. If we are lucky, this return will immediately cause a segfault and crash.

xsaschako and others added 22 commits August 30, 2023 11:37

firstcommit

3fcb877

add first draft for tree

3113609

Merge main to update to newest version

cb2a563

Merge branch 'main' into 664-define-and-use-parameters-that-handle-th…

59d10be

…e-state-transitions-in-the-infection

Update parameters

25e7905

Use new enum iterator

7d92aa8

Fix parameters

a4f16e8

Fix infection setup

64f2edf

Fix analyze_result

8b995e2

Fix error in transmission

e74c26c

Merge branch 'main' into 664-define-and-use-parameters-that-handle-th…

e5afe7c

…e-state-transitions-in-the-infection

Some fixes

c7d6f12

Add author

5100fb8

Signed-off-by: DavidKerkmann <[email protected]>

Fix recovered people not beeing able to reinfect.

85ab275

Signed-off-by: DavidKerkmann <[email protected]>

Change back to infection course related time parameters and fix build.

10bd31e

Merge branch main into 664-define-and-use-parameters-that-handle-the-…

4a47c69

…state-transitions-in-the-infection

Fix from merge

6f489f6

Fix infection tests

054b746

Adjust contact rates correctly and fix dividing by 0 in exposure rate.

e9069a4

Merge branch 'main' into 664-define-and-use-parameters-that-handle-th…

223e281

…e-state-transitions-in-the-infection

merge main

21c3753

merge branch 664

1c36293

jubicker self-assigned this Aug 13, 2024

jubicker linked an issue Aug 13, 2024 that may be closed by this pull request

Make ABM infection states purely symptomatic states and provide interface to link infectivity and state transitions #1081

Open

2 tasks

Implement state transition dist

6f73cf4

jubicker marked this pull request as ready for review August 22, 2024 06:55

mock log normal dist in test

19a48c7

jubicker added 2 commits August 22, 2024 14:39

python bindings

eec7073

add exponential dist as state transition dist

fb155bf

mknaranja changed the title ~~1081 rework abm state transitions~~ 1081 rework ABM state transitions Oct 16, 2024

jubicker added 16 commits December 9, 2024 08:14

add adjust contact rates

f95561e

merge main

b03f75d

fix tests

af6111c

merge main

92a2034

fix tests

32f3a00

Fix msvc test

dd13ea9

add param dist wrapper and modify param dists

af643b9

add rng as input for get_rand_sample

e16b36d

replace state transition dist by parameter dist

9b4a51b

delete state transition dist

76fa145

constant instead of lognormal function for test

87619a1

fix sigma=0 for normal dist on msvc

d072b6b

fix

9efad16

Merge branch 'main' into 1081-rework-abm-state-transitions

1a06e61

Merge branch '1081-rework-abm-state-transitions' of https://github.co…

a1772b1

…m/SciCompMod/memilio into 1081-rework-abm-state-transitions

fix python bindings

be07ec6

jubicker requested review from DavidKerkmann and xsaschako January 8, 2025 13:15

DavidKerkmann requested changes Jan 9, 2025

View reviewed changes

DavidKerkmann mentioned this pull request Jan 10, 2025

664 Define and use parameters that handle the state transitions in the Infection #710

Closed

10 tasks

reneSchm reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1081 rework ABM state transitions #1107

1081 rework ABM state transitions #1107

jubicker commented Aug 13, 2024 •

edited by DavidKerkmann

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

DavidKerkmann left a comment

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann Jan 9, 2025

DavidKerkmann commented Jan 10, 2025

reneSchm Jan 15, 2025

		// Distribution that can be used for the time spend in InfectionStates
		using InfectionStateTimesDistributionsParameters = LogNormalDistribution<double>::ParamType;

		std::min(std::max(min_val, (1 - dev_rel * 2.6) * v), 0.1 * std::numeric_limits<double>::max()),
		std::min(std::max(min_val, (1 + dev_rel * 2.6) * v), 0.5 * std::numeric_limits<double>::max()),

		// Set trips to use weekday trips on weekends.
		data.use_weekday_trips_on_weekend();

	// Mock the distribution to prevent infectionsin the test.
	// Mock the distribution to prevent infections in the test.

1081 rework ABM state transitions #1107

Are you sure you want to change the base?

1081 rework ABM state transitions #1107

Conversation

jubicker commented Aug 13, 2024 • edited by DavidKerkmann Loading

Changes and Information

Merge Request - Guideline Checklist

Checks by code author

Checks by code reviewer(s)

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

DavidKerkmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidKerkmann commented Jan 10, 2025

Choose a reason for hiding this comment

jubicker commented Aug 13, 2024 •

edited by DavidKerkmann

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading