1159 add diffusive abm and smm #1162

jubicker · 2024-12-12T14:03:41Z

Changes and Information

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

add diffusive ABM (model, simulation, parameters and quadwell potential)
add SMM (model, simulation, parameters)
add tests for both models
add examples for both models

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).

closes #1159

codecov · 2024-12-13T12:07:36Z

Codecov Report

Attention: Patch coverage is 99.52153% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.15%. Comparing base (34a0277) to head (3740a60).

Files with missing lines	Patch %	Lines
cpp/models/d_abm/quad_well.h	98.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1162      +/-   ##
==========================================
+ Coverage   97.13%   97.15%   +0.02%     
==========================================
  Files         149      154       +5     
  Lines       13999    14208     +209     
==========================================
+ Hits        13598    13804     +206     
- Misses        401      404       +3

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

cpp/models/d_abm/quad_well.h

reneSchm

I seem to barely document code I write under a deadline. I will continue this with more suggestions later, but feel free to edit them - or add your own documentation, there is a lot to do...

The directory structure and naming looks pretty good. I have not checked the tests yet

cpp/models/d_abm/CMakeLists.txt

cpp/models/d_abm/README.md

cpp/models/d_abm/model.h

cpp/tests/test_d_abm_model.cpp

reneSchm · 2025-01-16T15:21:53Z

cpp/tests/test_d_abm_model.cpp

+    QuadWellModel<InfectionState>::Agent a1{Eigen::Vector2d{-1, 1}, InfectionState::S};
+    QuadWellModel<InfectionState>::Agent a2{Eigen::Vector2d{-1.2, 1}, InfectionState::I};
+    QuadWellModel<InfectionState> qw({a1, a2}, {{InfectionState::S,
+                                                 InfectionState::E,
+                                                 mio::dabm::Region(0),
+                                                 0.1,
+                                                 {InfectionState::C, InfectionState::I},
+                                                 {1, 0.5}}});
+    EXPECT_EQ(qw.adoption_rate(a1, InfectionState::E), 0.025);


Other branches that are not checked are

the rate for a2 to E is 0

another isolated infected does not influence the rate

the rate of a1 scales correctly with more nearby infected and non-infected

Can you add descriptions to the test what it is you check? Otherwise it is hard to tell what is going on here.

This is true for all added tests. For example, in TestQuadWell.well(_index ?) one comment at the top like "check that well_index returns indices in the correct order" would be enough.
Here, in TestQuadWell.adoptionRate, you could write down what the agents and rates are used for, and/or explain what the EXPECTs do. Like "check that a1 is affected by a2, but not the for off a3" for the first EXPECT call, and so on.

cpp/tests/test_d_abm_model.cpp

cpp/models/d_abm/quad_well.h

cpp/models/d_abm/parameters.h

cpp/models/d_abm/quad_well.h

cpp/models/smm/model.h

cpp/models/smm/parameters.h

Co-authored-by: reneSchm <[email protected]>

reneSchm · 2025-01-31T13:20:49Z

cpp/memilio/epidemiology/adoption_rate.h

+#ifndef ADOPTIONRATE_H
+#define ADOPTIONRATE_H


Suggested change

#ifndef ADOPTIONRATE_H

#define ADOPTIONRATE_H

#ifndef MIO_EPI_ADOPTIONRATE_H

#define MIO_EPI_ADOPTIONRATE_H

cpp/memilio/epidemiology/adoption_rate.h

reneSchm · 2025-01-31T13:27:26Z

cpp/memilio/epidemiology/adoption_rate.h

+    Status to; // j
+    mio::regions::Region region; // k
+    ScalarType factor; // gammahat_{ij}^k
+    std::vector<Influence<Status>> influences;


Suggested change

std::vector<Influence<Status>> influences;

std::vector<Influence<Status>> influences; // influences[tau] = ( Psi_{i,j,tau} , gammahat_{i,j,tau} )

I think that should be the symbols, right? Do we reference our paper in both model Readmes?

reneSchm · 2025-01-31T13:34:58Z

cpp/memilio/geography/regions.h

 namespace regions
 {

+/// @biref Index for enumerating subregions (cities, counties, etc.) of the modelled area.
+struct Region : public mio::Index<Region> {


I think it would be fine to put Region into the mio namespace directly. Or do you prefer to "hide" it here?

reneSchm · 2025-01-31T13:36:43Z

cpp/models/d_abm/parameters.h

-    {
-        return "AdoptionRates";
-    }
-};



This header is empty, should we remove it?

reneSchm · 2025-01-31T13:39:03Z

cpp/models/d_abm/quad_well.h

@@ -24,11 +24,13 @@
 #include "memilio/math/eigen.h"


I can't reach the include guard for comments, please change it to MIO_D_ABM_QUAD_WELL_H

reneSchm · 2025-01-31T14:11:14Z

cpp/tests/test_smm_model.cpp

    EXPECT_EQ(sim.get_result().get_last_time(), 30.);
 }
+
+TEST(TestSMMSimulation, covergence)


Suggested change

TEST(TestSMMSimulation, covergence)

TEST(TestSMMSimulation, convergence)

reneSchm · 2025-01-31T14:37:05Z

cpp/tests/test_smm_model.cpp

+        std::abs(rate * pop2 - std::abs(std::accumulate(transitions2.begin(), transitions2.end(), 0.0) / num_runs2)) /
+        (rate * pop2);
+
+    EXPECT_GE(rel_diff1, rel_diff2);


Why do you compare the two models with each other rather than with the expected result? The tolerance for comparing with the result is probably fairly large, but I think it would be better than comparing two "random" values.

Also, why the greater equal? If both models converge, reldiff1 < rel_diff2 should be possible, right? Maybe use one model, and compare the simulated rate against the expected rate, with some tolerance (with can be fairly large).

Moreover, should this be a random test perhaps? So that it checks not only that one specific seed works. It is okay for a random test to possibly fail, if it is very unlikely that it does. You can run it a couple 1000 times to check that the tolerance is large enough for most edge cases.

What I do here is check whether the simulated transition rate converges to the expected transition rate for num_runs, pop_size to infty. And I realize this by checking whether the relative difference between the simulated number of transitions and the expected number of transitions gets smaller if I increase the number of runs as well as the number of agents.

I can also only use one model and compare the simulated and expected number of transitions, but what would be an appropriate tolerance? 1%, 50%? And how many runs do we require to meet that tolerance?

And I realize this by checking whether the relative difference between the simulated number of transitions and the expected number of transitions gets smaller if I increase the number of runs as well as the number of agents.

Correct me if I'm wrong, but this is not what you test here. You test only for 2 samples from (num_runs, pop) to infty, and for fixed seeds. The result from that could show pretty much anything, if you were to select seeds intentionally. And as I said, I am pretty sure rel_diff1 >= rel_diff2 does not hold in general. On average, probably; in limit, almost surely. But two samples are enough to convince me of either.

That is why I would aim to test on pair (num_runs, pop), that the average perceived rate over some number of runs is close to the expected rate. Mathematically, the test result is a lot weaker than what you aim to do, but it allows us to make the test itself stronger.

but what would be an appropriate tolerance? 1%, 50%? And how many runs do we require to meet that tolerance?

That part is finicky, and will depend on the model setup. And since we want to keep runtime low for the test we cannot aim for 1-e10 or so, I actually think 50% would be fine. Though smaller would of course be nicer. I would start with averaging over a thousand runs, and do that a thousand times (with a temporary loop or via bash) to check how the variance looks. Then iterate from there by de-/increasing runs, pop or tolerance. You can additionally check the average after each run for whether it is "stable" enough at 1000, or still to fluctuating.

Co-authored-by: reneSchm <[email protected]>

jubicker added 2 commits December 12, 2024 12:49

add diffusive ABM and tests

3b8cedb

read me diffusive abm

294b601

jubicker linked an issue Dec 12, 2024 that may be closed by this pull request

Add diffusive ABM and SMM #1159

Open

2 tasks

jubicker added 4 commits December 13, 2024 11:16

add smm

9977cc8

add tests for smm

0b0ebe3

smm readme

2b95423

msvc is a special snowflake

8807c55

examples smm and dabm

26f7258

jubicker requested a review from reneSchm December 13, 2024 14:37

reneSchm reviewed Dec 16, 2024

View reviewed changes

cpp/models/d_abm/quad_well.h Outdated Show resolved Hide resolved

jubicker added 2 commits December 18, 2024 16:46

advance tests for smm and d_abm simulations

428cffa

Merge branch 'main' into 1159-add-diffusive-abm-and-smm

985bd06

reneSchm requested changes Jan 8, 2025

View reviewed changes

cpp/models/d_abm/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/models/d_abm/README.md Outdated Show resolved Hide resolved

cpp/models/d_abm/model.h Outdated Show resolved Hide resolved

reneSchm reviewed Jan 16, 2025

View reviewed changes

reneSchm reviewed Jan 27, 2025

View reviewed changes

jubicker and others added 14 commits January 28, 2025 15:57

add smm tests

4313cf6

add documentation for smm tests

6f0b6fd

enhance tests

136be89

review

b747a13

Apply suggestions from code review

bd40f9f

Co-authored-by: reneSchm <[email protected]>

Review

88407a6

CI

da23351

CI

eab5ddd

CI

72277d6

CI

e265e84

CI

63d3601

CI

37f062b

ci smm

e6a3516

smm stops at tmax test

67a939f

jubicker added 6 commits January 30, 2025 07:46

CI

0d33105

ci

dcf6c81

sim advance

648efa4

fix sanitize bug

568adec

Review

12dd3e4

Review and documentation

5f43aa0

reneSchm requested changes Jan 31, 2025

View reviewed changes

jubicker and others added 3 commits January 31, 2025 16:25

Update cpp/memilio/epidemiology/adoption_rate.h

8cc5a22

Co-authored-by: reneSchm <[email protected]>

merge main

75fc32e

Review

3740a60

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1159 add diffusive abm and smm #1162

1159 add diffusive abm and smm #1162

jubicker commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading

reneSchm left a comment

reneSchm Jan 16, 2025

reneSchm Jan 31, 2025

reneSchm Jan 31, 2025

reneSchm Jan 31, 2025

jubicker Feb 5, 2025

reneSchm Jan 31, 2025

reneSchm Jan 31, 2025

reneSchm Jan 31, 2025

reneSchm Jan 31, 2025

reneSchm Jan 31, 2025

reneSchm Jan 31, 2025

jubicker Jan 31, 2025 •

edited

Loading

reneSchm Jan 31, 2025

	std::vector<Influence<Status>> influences;
	std::vector<Influence<Status>> influences; // influences[tau] = ( Psi_{i,j,tau} , gammahat_{i,j,tau} )

	TEST(TestSMMSimulation, covergence)
	TEST(TestSMMSimulation, convergence)

1159 add diffusive abm and smm #1162

Are you sure you want to change the base?

1159 add diffusive abm and smm #1162

Conversation

jubicker commented Dec 12, 2024 • edited Loading

Changes and Information

Merge Request - Guideline Checklist

Checks by code author

Checks by code reviewer(s)

codecov bot commented Dec 13, 2024 • edited Loading

Codecov Report

reneSchm 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

Choose a reason for hiding this comment

jubicker Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubicker commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading

jubicker Jan 31, 2025 •

edited

Loading