1250 enhance testing logic in abm #1276

xsaschako · 2025-05-15T23:02:15Z

Changes and Information

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

Enhance logic, readability and access time
Simplify: dont update activity status -> error prone and not better runtime

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

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

…ion for clarity

… location types

…enhance testing strategy checks

…g scheme handling in tests

…ance serialization, and improve test logic

Copilot

Pull Request Overview

This PR enhances the testing logic for the ABM by simplifying testing criteria, updating the API for testing schemes, and improving serialization. Key changes include:

Replacing previous testing scheme APIs (e.g. run_scheme) with new methods (run_scheme_and_check_if_test_positive and is_active with time parameter).
Updating functions to add testing schemes by location type or id.
Adapting tests and serialization accordingly.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
cpp/tests/test_abm_testing_strategy.cpp	Refactored test cases to use new testing scheme API and updated evaluation logic.
cpp/tests/test_abm_serialization.cpp	Updated reference JSON keys to match the new serialization format.
cpp/tests/test_abm_model.cpp	Modified test scenarios to use the new location-based testing scheme assignment.
cpp/models/graph_abm/graph_abmodel.h	Updated testing scheme call to use the new run_strategy_and_check_if_entry_allowed API.
cpp/models/abm/testing_strategy.h	Removed outdated functions and updated API for adding testing schemes.
cpp/models/abm/testing_strategy.cpp	Changes to testing scheme activation and strategy evaluation logic.
cpp/models/abm/model.cpp	Modified integration of testing strategy in mobility methods.
cpp/benchmarks/abm.cpp	Replaced old API calls with the new testing scheme API for location types.

cpp/models/abm/testing_strategy.cpp

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

…th add_testing_scheme_location_type for consistency across examples

…strategies

…t_model

codecov · 2025-05-19T10:01:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.23%. Comparing base (83c57ad) to head (2c5850d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1276      +/-   ##
==========================================
- Coverage   97.28%   97.23%   -0.05%     
==========================================
  Files         168      168              
  Lines       14858    14838      -20     
==========================================
- Hits        14454    14428      -26     
- Misses        404      410       +6

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

xsaschako · 2025-05-19T14:35:19Z

cpp/models/abm/model.cpp

-                    // the Person cannot move if the performed TestingStrategy is positive
-                    if (!m_testing_strategy.run_strategy(personal_rng, person, target_location, t)) {
+                    // The person cannot move if he has a positive test result
+                    if(!m_testing_strategy.run_strategy_and_check_if_entry_allowed(personal_rng, person, target_location, t)){


Shorten function names and maybe improve documentation

… consistency and readability across the codebase

…nality

…st compliance

…ction and update tests to remove direct calls

DavidKerkmann

Thanks for the improvements and also the nice new tests!
There are only a couple of remarks and inquiries.

DavidKerkmann · 2025-07-15T07:54:20Z

cpp/models/abm/testing_strategy.cpp

+    if (person.get_compliance(InterventionType::Testing) <
+            1.0 && // Dont need to draw a random number if the person is compliant either way
+        !person.is_compliant(
+            rng, InterventionType::Testing)) { // If the person is not compliant with the testing intervention
+        return true; // Assume positive test result as this should not allow entry although it is not the same
+    }


If a person is not compliant but has a valid test certificate from before, entry should still be allowed. This happens frequently with people that have moderate compliance, I suppose.

DavidKerkmann · 2025-07-15T08:02:10Z

cpp/models/abm/README.md

-model.get_testing_strategy().add_testing_scheme(testing_scheme_work);
+model.get_testing_strategy().add_scheme(testing_scheme_work);


Any specific reason for this rename here and in all other places?

DavidKerkmann · 2025-07-15T08:32:29Z

cpp/models/abm/testing_strategy.cpp

            }
        }
    }
-    return true;
+
+    // If the location is a home, entry is always allowed regardless of testing, no early return here because we still need to test


Not sure. Why do we need to test if a person goes home? This would say that there is a testing scheme that tests if a person has to test if going home.

DavidKerkmann · 2025-07-15T08:41:43Z

cpp/models/abm/testing_strategy.h

@@ -187,88 +172,74 @@ class TestingStrategy
     * A LocalStrategy with id of value LocationId::invalid_id() is used for all Locations with LocationType type.


Requires adaptation.
To be fair, I think we don't really need this struct anymore with these changes.

DavidKerkmann · 2025-07-15T08:46:08Z

cpp/tests/test_abm_testing_strategy.cpp

-
-    // Reactivate the scheme.
-    testing_scheme1.update_activity_status(mio::abm::TimePoint(0));
+    EXPECT_EQ(testing_scheme1.is_active(mio::abm::TimePoint(60 * 60 * 24 * 3 + 200)), false);


Could also be rewritten with days(3).

DavidKerkmann · 2025-07-15T08:55:39Z

cpp/tests/test_abm_testing_strategy.cpp

+    // Create PCR test parameters with high accuracy but long wait time
+    const auto test_params_pcr = mio::abm::TestParameters{0.95, 0.99, mio::abm::hours(24), mio::abm::TestType::PCR};
+
+    // Create rapid test parameters with lower accuracy but quick results (we need to set this to 24 hours to avoid the person getting healthy randomly)


Not sure I understand this. Can you elaborate why a person can get healthy? We look into the past with the evaluation time.

DavidKerkmann · 2025-07-15T09:02:00Z

cpp/tests/test_abm_testing_strategy.cpp

+
+    // Test at shop with specific ID - should run the scheme
+    bool result1 = test_strategy.run_and_check(rng, person, shop1, start_date);
+    EXPECT_EQ(result1, false); // Person should not be allowed to enter after positive test


Suggested change

EXPECT_EQ(result1, false); // Person should not be allowed to enter after positive test

EXPECT_EQ(result1, false); // Person should not be allowed to enter after not complying to the test

xsaschako added 7 commits April 15, 2025 14:57

first changes

0263ae8

secando changes

bdedb1a

Refactor testing strategy methods for clarity and functionality

08a1b68

Enhance TestingStrategy with compliance checks and improve documentat…

6213264

…ion for clarity

Add assertions and improve logic for testing scheme entry checks

9f93d6a

Enable benchmarks and adjust infection percentage in simulation logic

e43abdc

Refactor testing strategy to consolidate testing schemes for multiple…

c26ca83

… location types

xsaschako linked an issue May 15, 2025 that may be closed by this pull request

Enhance testing logic in ABM #1250

Open

2 tasks

xsaschako added 4 commits May 16, 2025 01:18

Update CMake options, adjust infection percentage in simulation, and …

298211f

…enhance testing strategy checks

Refactor TestingStrategy: remove deprecated methods and update testin…

7176447

…g scheme handling in tests

Refactor TestingStrategy and tests: streamline method signatures, enh…

75fad09

…ance serialization, and improve test logic

formatting

640f581

xsaschako marked this pull request as ready for review May 16, 2025 11:44

xsaschako requested a review from Copilot May 16, 2025 11:44

Copilot AI reviewed May 16, 2025

View reviewed changes

cpp/models/abm/testing_strategy.cpp Outdated Show resolved Hide resolved

cpp/models/abm/testing_strategy.cpp Outdated Show resolved Hide resolved

xsaschako and others added 6 commits May 16, 2025 14:26

Update cpp/models/abm/testing_strategy.cpp

18f8388

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

Refactor testing strategy method calls: replace add_testing_scheme wi…

e2ee172

…th add_testing_scheme_location_type for consistency across examples

Merge branch 'main' into 1250-enhance-testing-logic-in-abm

f931acd

Enhance TestingStrategy constructor: add overload for multiple local …

1bbf4b9

…strategies

Refactor test_abm: remove unused testing scheme initialization in tes…

36fcce3

…t_model

Fix indentation in abm.cpp: align method chaining for better readability

4bdfd4a

xsaschako commented May 19, 2025

View reviewed changes

xsaschako added 5 commits May 20, 2025 15:59

Refactor testing strategy method names: unify method names to improve…

3a8163f

… consistency and readability across the codebase

Add tests for edge cases in TestingCriteria and TestingScheme functio…

1778fd0

…nality

Enhance run_and_test documentation: clarify return value regarding te…

475b57d

…st compliance

Refactor TestingScheme: move start and end date getters to private se…

8db4f79

…ction and update tests to remove direct calls

Merge branch 'main' into 1250-enhance-testing-logic-in-abm

319cfc6

xsaschako requested a review from DavidKerkmann June 21, 2025 14:30

test

eca897c

xsaschako added 2 commits June 21, 2025 17:12

[skip CI] revert test

0f452ae

test bug

91fb8ba

xsaschako self-assigned this Jun 23, 2025

xsaschako added 8 commits June 23, 2025 19:27

test_bugfix

6483fd4

enhance performance and add test

923e01c

Merge branch 'main' into 1250-enhance-testing-logic-in-abm

ce43c62

redo

b18cf86

redo

db83cd3

redo

dd6cac7

redo

db41cd0

redo

2c5850d

DavidKerkmann requested changes Jul 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

1250 enhance testing logic in abm #1276

1250 enhance testing logic in abm #1276

Uh oh!

xsaschako commented May 15, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 19, 2025 •

edited

Loading

Uh oh!

xsaschako May 19, 2025 •

edited

Loading

Uh oh!

DavidKerkmann left a comment

Uh oh!

DavidKerkmann Jul 15, 2025

Uh oh!

DavidKerkmann Jul 15, 2025

Uh oh!

DavidKerkmann Jul 15, 2025

Uh oh!

DavidKerkmann Jul 15, 2025

Uh oh!

DavidKerkmann Jul 15, 2025

Uh oh!

DavidKerkmann Jul 15, 2025

Uh oh!

DavidKerkmann Jul 15, 2025

Uh oh!

Uh oh!

		model.get_testing_strategy().add_testing_scheme(testing_scheme_work);
		model.get_testing_strategy().add_scheme(testing_scheme_work);

		@@ -187,88 +172,74 @@ class TestingStrategy
		* A LocalStrategy with id of value LocationId::invalid_id() is used for all Locations with LocationType type.

	EXPECT_EQ(result1, false); // Person should not be allowed to enter after positive test
	EXPECT_EQ(result1, false); // Person should not be allowed to enter after not complying to the test

1250 enhance testing logic in abm #1276

Are you sure you want to change the base?

1250 enhance testing logic in abm #1276

Uh oh!

Conversation

xsaschako commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes and Information

Merge Request - Guideline Checklist

Checks by code author

Checks by code reviewer(s)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xsaschako May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann left a comment

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

DavidKerkmann Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xsaschako commented May 15, 2025 •

edited

Loading

codecov bot commented May 19, 2025 •

edited

Loading

xsaschako May 19, 2025 •

edited

Loading