1210 abm paper logger by xsaschako · Pull Request #1507 · SciCompMod/memilio

xsaschako · 2026-03-09T16:09:13Z

Changes and Information

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

ad vizualizations
added logger
added one logger to the parameter study example

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 within the code (Doxygen) for new functionality has been added in the code
Appropriate external documentation (ReadTheDocs) for new functionality has been added to the online documentation
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).

…on runs

…es script

…rove function structure, and add new plotting capabilities for location types.

… documentation, and improve function argument handling for better usability

…st cases for loading H5 results and plotting functions

…ments for clarity

…ll_requires

…th comparison plots

…arguments for better flexibility

…ilio-plot

…from-paper-to-memilio-plot

…into 1210-abm-paper-logger

Copilot

Pull request overview

This PR adds a new ABM (Agent-Based Model) logger for tracking new infections per location type per age group (LogInfectionPerLocationTypePerAgeGroup), integrates it into ResultSimulation via a new history_detailed history and get_result_detailed() method, updates the abm_parameter_study.cpp example to save both standard and detailed results, and introduces a new Python plotting module (plotAbmInfectionStates.py) with tests for visualizing the results.

Changes:

Added LogInfectionPerLocationTypePerAgeGroup (and unused LogInfectionStatePerAgeGroup) logger structs in common_abm_loggers.h
Added m_t_prev tracking to Simulation class and integrated the new detailed logger into ResultSimulation
Updated abm_parameter_study.cpp example to collect and save both aggregated and detailed results in separate subdirectories
Added new Python plotting module plotAbmInfectionStates.py with functions to visualize infection states and infection-by-location-type data, plus unit tests

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
`cpp/models/abm/simulation.h`	Adds `m_t_prev` member and `get_prev_time()` to track previous simulation time step, needed by new logger
`cpp/models/abm/common_abm_loggers.h`	Introduces two new logger structs for per-age-group data; one is unused
`cpp/models/abm/result_simulation.h`	Adds `history_detailed` and `get_result_detailed()` using the new logger; changes `get_result()` return type
`cpp/examples/abm_parameter_study.cpp`	Updated example to run 10 simulations over 10 days and save both standard and detailed results
`pycode/memilio-plot/memilio/plot/plotAbmInfectionStates.py`	New Python module for loading and plotting ABM infection state and location-type results
`pycode/memilio-plot/tests/test_plot_plotAbmInfectionStates.py`	Unit tests for the new Python plotting module

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-03-09T18:35:50Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.47%. Comparing base (0888d6c) to head (64998ea).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1507      +/-   ##
==========================================
+ Coverage   97.44%   97.47%   +0.02%     
==========================================
  Files         190      190              
  Lines       16035    16066      +31     
==========================================
+ Hits        15626    15661      +35     
+ Misses        409      405       -4

☔ View full report in Codecov by Harness.
📢 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.

DavidKerkmann

Thanks, I have a few comments which should be discussed/resolved.
Furthermore, I think fixing #1406 here would make a lot of sense.

DavidKerkmann · 2026-10-07T14:31:18Z

+    {
+        return m_t_prev;
+    }
+


It's equivalent, but I somewhat prefer to use 'get_time() - get_time_step()' and make a method for the latter one.

Good point! I ended up removing m_t_prev and get_prev_time() entirely and the logger now keeps track of its previous log time itself, so Simulation is unchanged compared to main.
Also made changes, see below for the logger, since it was buggy since it logged the locations after the tilmestep (we had this bug already I kind of remember).

DavidKerkmann · 2026-10-08T11:13:20Z

+
+            // save_result splits each row into num_groups groups. Both results are a single vector per time point,
+            // so they are written as one group each, whose "Total" holds all entries of that vector. Passing the
+            // number of age groups here would instead split those entries across that many groups.
+            mio::unused(mio::save_result(ensemble_percentiles, {0}, 1,
+                                         (result_dir_standard / fmt::format("Results_p{:02d}.h5", p)).string()));
+            mio::unused(mio::save_result(ensemble_percentiles_detailed, {0}, 1,
+                                         (result_dir_detailed / fmt::format("Results_p{:02d}.h5", p)).string()));


Why is this in mio::unused?

there was no good reason. Both save_result calls now check the result and print the error, like in ode_secir_parameter_study.cpp.

DavidKerkmann · 2026-10-08T11:22:57Z

    mio::History<TimeSeriesWriter, LogInfectionState> history{
        Eigen::Index(InfectionState::Count)}; ///< History used to create the result TimeSeries.
+
+    mio::History<TimeSeriesWriter, LogInfectionPerLocationTypePerAgeGroup> history_detailed{
+        Eigen::Index(LocationType::Count) *
+        this->get_model().parameters.get_num_groups()}; ///< History used to create the detailed result TimeSeries.


I find it a bit unclean that we have a hardcoded result_simulation which is tailored to the one example (parameter study) with these particular loggers/histories.
It would be better to have a framework that allows us to do the parameter studies in a way where the make_model and the history(-ies) is/are provided instead.

Agreed. I reverted ResultSimulation to main and moved the study-specific histories into a small StudySimulation wrapper in the example.

DavidKerkmann · 2026-10-08T11:25:34Z

+    EXPECT_EQ(detailed.get_value(1)[index], 1.0);
+}
+
+TEST(TestSimulation, logInfectionStatePerAgeGroup)


Would it make sense to have a similar test for the other new logger logInfectionStatePerLocationTypePerAgeGroup?

Yes, that’s covered by TestSimulation.logInfectionStatePerAgeGroup. I also reworked the test for LogInfectionPerLocationTypePerAgeGroup (see below).

…on logger - Revert ResultSimulation and Simulation to main; the parameter study example now uses a small StudySimulation wrapper holding its histories. - LogInfectionPerLocationTypePerAgeGroup keeps its previous log time and location types itself and attributes new infections to the location where the interaction happened, not to where the person moved afterwards. - Gather the detailed results across MPI ranks by returning them from the processing lambda. - Handle the IOResult of save_result instead of discarding it. - LogInfectionState counts in a single pass over all persons (#1406).

xsaschako

See changes

also I made a new PR request for #1406
for this logger, which is the inly one that uses it, LogInfectionState, to be independent of it.

xsaschako · 2026-10-08T16:25:16Z

+    {
+        return m_t_prev;
+    }
+


Good point! I ended up removing m_t_prev and get_prev_time() entirely and the logger now keeps track of its previous log time itself, so Simulation is unchanged compared to main.
Also made changes, see below for the logger, since it was buggy since it logged the locations after the tilmestep (we had this bug already I kind of remember).

xsaschako · 2026-10-08T16:26:37Z

+
+            // save_result splits each row into num_groups groups. Both results are a single vector per time point,
+            // so they are written as one group each, whose "Total" holds all entries of that vector. Passing the
+            // number of age groups here would instead split those entries across that many groups.
+            mio::unused(mio::save_result(ensemble_percentiles, {0}, 1,
+                                         (result_dir_standard / fmt::format("Results_p{:02d}.h5", p)).string()));
+            mio::unused(mio::save_result(ensemble_percentiles_detailed, {0}, 1,
+                                         (result_dir_detailed / fmt::format("Results_p{:02d}.h5", p)).string()));


there was no good reason. Both save_result calls now check the result and print the error, like in ode_secir_parameter_study.cpp.

xsaschako · 2026-10-08T16:27:05Z

    mio::History<TimeSeriesWriter, LogInfectionState> history{
        Eigen::Index(InfectionState::Count)}; ///< History used to create the result TimeSeries.
+
+    mio::History<TimeSeriesWriter, LogInfectionPerLocationTypePerAgeGroup> history_detailed{
+        Eigen::Index(LocationType::Count) *
+        this->get_model().parameters.get_num_groups()}; ///< History used to create the detailed result TimeSeries.


Agreed. I reverted ResultSimulation to main and moved the study-specific histories into a small StudySimulation wrapper in the example.

xsaschako · 2026-10-08T16:27:18Z

+    EXPECT_EQ(detailed.get_value(1)[index], 1.0);
+}
+
+TEST(TestSimulation, logInfectionStatePerAgeGroup)


Yes, that’s covered by TestSimulation.logInfectionStatePerAgeGroup. I also reworked the test for LogInfectionPerLocationTypePerAgeGroup (see below).

xsaschako added 30 commits May 27, 2025 21:02

Add scripts for analyzing infection states and ICU data from simulati…

840303d

…on runs

Remove unused imports and enable plotting functions in infection stat…

c7ffab3

…es script

Refactor infection states plotting script: enhance documentation, imp…

e986d96

…rove function structure, and add new plotting capabilities for location types.

Refactor plotAbmInfectionStates.py: update authorship, enhance module…

6e4c6fe

… documentation, and improve function argument handling for better usability

Add unit tests for plotAbmInfectionStates: implement comprehensive te…

663a189

…st cases for loading H5 results and plotting functions

Refactor setup.py: enhance PylintCommand documentation and update com…

28c3187

…ments for clarity

Fix comment in setup.py: update example for Excel file types in insta…

2ba672e

…ll_requires

Remove plotAbmICUAndDeadComp.py: delete unused script for ICU and dea…

baa272b

…th comparison plots

formatting

20b651e

Implement martins suggestions

706aea0

Refactor plotAbmInfectionStates.py: update paths to use command line …

ab2e0e8

…arguments for better flexibility

Julias suggestions p1

e4df6cb

julia review p2

c5cf7dc

julia review pt3

f5dffd6

Merge branch 'main' into 1290-add-abm-visualization-from-paper-to-mem…

f124f04

…ilio-plot

format

75e1105

fix tests

ddb9cb4

julia review pt5

c2433fc

julia review pt7

606ba7f

fix: improve error message for missing file in fake filesystem

e316b6a

Merge branch 'main' into 1290-add-abm-visualization-from-paper-to-mem…

a446792

…ilio-plot

Update test_surrogatemodel_ode_secir_groups.py

8a002c7

Update test_surrogatemodel_ode_secir_groups.py

392ec54

Merge branch '1210-abm-paper-logger' into 1290-add-abm-visualization-…

cb08197

…from-paper-to-memilio-plot

update

7ccdb45

update example

2132e8a

Merge branch '1290-add-abm-visualization-from-paper-to-memilio-plot' …

0c5c13a

…into 1210-abm-paper-logger

update

0fdbb15

redo

e22dd77

update

7d544bf

xsaschako added 3 commits March 9, 2026 16:49

fix comment formatting in plotAbmInfectionStates.py

9724423

update

22044c6

update

6c6879f

xsaschako requested review from DavidKerkmann and Copilot March 9, 2026 16:09

xsaschako self-assigned this Mar 9, 2026

This was linked to issues Mar 9, 2026

Add ABM visualization from paper to MEmilio-plot #1290

Open

ABM Paper: Logger #1210

Open

Copilot started reviewing on behalf of xsaschako March 9, 2026 16:12 View session

update

49d0587

Copilot AI reviewed Mar 9, 2026

View reviewed changes

xsaschako added 4 commits July 31, 2026 18:16

Merge branch 'main' into 1210-abm-paper-logger

8fc375a

update

72fcfc5

delete comment

d40ead4

format

19b306e

xsaschako mentioned this pull request Jul 31, 2026

1290 add ABM visualization from paper to memilio plot #1291

Closed

7 of 12 tasks

xsaschako added 2 commits July 31, 2026 20:38

fix test

faf0f40

update

650985f

DavidKerkmann requested changes Oct 8, 2026

View reviewed changes

xsaschako added 2 commits October 8, 2026 15:14

Merge branch 'main' into 1210-abm-paper-logger

8e9d969

xsaschako mentioned this pull request Oct 8, 2026

1406 Count subpopulations in a single pass over all persons #1595

Open

8 of 13 tasks

update comment

64998ea

xsaschako commented Oct 8, 2026

View reviewed changes

xsaschako requested a review from DavidKerkmann October 9, 2026 08:26

Conversation

xsaschako commented Mar 9, 2026 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DavidKerkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xsaschako left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xsaschako commented Mar 9, 2026 •

edited

Loading

codecov Bot commented Mar 9, 2026 •

edited

Loading