Conversation
…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
…th comparison plots
…arguments for better flexibility
…from-paper-to-memilio-plot
…into 1210-abm-paper-logger
There was a problem hiding this comment.
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 unusedLogInfectionStatePerAgeGroup) logger structs incommon_abm_loggers.h - Added
m_t_prevtracking toSimulationclass and integrated the new detailed logger intoResultSimulation - Updated
abm_parameter_study.cppexample to collect and save both aggregated and detailed results in separate subdirectories - Added new Python plotting module
plotAbmInfectionStates.pywith 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.
| mio::TimeSeries<double> get_result_detailed() const | ||
| { | ||
| return std::get<0>(history_detailed.get_log()); | ||
| } |
There was a problem hiding this comment.
The new LogInfectionPerLocationTypePerAgeGroup logger and get_result_detailed() method in ResultSimulation have no unit tests. The existing test_abm_simulation.cpp tests LogInfectionState and ResultSimulation::get_result() but does not test the new detailed result. Given the comprehensive testing of other loggers in test_abm_simulation.cpp, the new logger and the get_result_detailed() function should be covered as well, particularly verifying correct detection of new infection events and their assignment to location types.
| self.assertEqual(mock_figure.call_count, 2, | ||
| "figure should be called twice") | ||
| # Verify first call is with the figure name | ||
| mock_figure.assert_any_call('Infection_location_types') | ||
| # Verify second call is without arguments (for autofmt_xdate) | ||
| mock_figure.assert_any_call() |
There was a problem hiding this comment.
The test asserts mock_figure.call_count == 2 (line 75) and mock_figure.assert_any_call() (line 80), claiming plt.figure() is called twice (once with a name and once without arguments for autofmt_xdate). However, plot_infections_loc_types_average only calls plt.figure('Infection_location_types') once (line 142 in the source). The _format_x_axis helper calls plt.gcf(), not plt.figure(). This assertion will fail because mock_figure.call_count will be 1, not 2.
| /** | ||
| * @brief Return the detailed simulation result aggregated by infection states. | ||
| */ | ||
| mio::TimeSeries<double> get_result_detailed() const | ||
| { | ||
| return std::get<0>(history_detailed.get_log()); | ||
| } |
There was a problem hiding this comment.
The docstring for get_result_detailed() is incorrectly indented (extra leading spaces before /**) and its content says "aggregated by infection states" which is inaccurate — the detailed result is aggregated by location type per age group (using LogInfectionPerLocationTypePerAgeGroup), not by infection states.
| parser.add_argument("--90percentile", action="store_true", | ||
| help="If set, plot 90% percentile as well") | ||
| args = parser.parse_args() | ||
|
|
||
| if not args.path_to_infection_states and not args.path_to_loc_types: | ||
| print("Please provide a path to infection states or location types results.") | ||
| return | ||
|
|
||
| if args.path_to_infection_states: | ||
| plot_infection_states_results( | ||
| path_to_infection_states=args.path_to_infection_states, | ||
| start_date=args.start_date, | ||
| colormap=args.colormap, | ||
| xtick_step=args.xtick_step, | ||
| show90=True | ||
| ) |
There was a problem hiding this comment.
The --90percentile CLI argument (line 378) has two problems: (1) Argument names starting with a digit are not officially supported by Python's argparse. The attribute access would be args._90percentile (note the leading underscore added by argparse), not args.90percentile. (2) Even if accessed correctly, the flag value is completely ignored — show90 is hardcoded to True on line 392 regardless of whether the user passes --90percentile. The --90percentile flag should be renamed (e.g., --show-90percentile) and its value should be passed to plot_infection_states_results as show90=getattr(args, '_90percentile').
| # The used Loggers are: | ||
| # LogInfectionStatePerAgeGroup | ||
| # LogInfectionPerLocationTypePerAgeGroup | ||
| # The output of the loggers of several runs is stored in HDF5 files using mio::save_results in mio/io/result_io.h, see abm_history_object.cpp. |
There was a problem hiding this comment.
The module-level comment on lines 37-40 states that the used loggers are LogInfectionStatePerAgeGroup and LogInfectionPerLocationTypePerAgeGroup. However, result_simulation.h uses LogInfectionState (not LogInfectionStatePerAgeGroup) for the history member. The comment should be corrected to reference LogInfectionState instead of LogInfectionStatePerAgeGroup.
| mio::TimeSeries<double> get_result() const | ||
| { | ||
| return get<0>(history.get_log()); | ||
| return std::get<0>(history.get_log()); | ||
| } |
There was a problem hiding this comment.
The get_result() function's return type was changed from const mio::TimeSeries<double>& to mio::TimeSeries<double> (returning by value). This causes an unnecessary copy of the entire TimeSeries object on every call, including the two calls in abm_parameter_study.cpp lines 215 and 220 (sim.get_result() is called twice). Since TimeSeries can be large (many time points × many elements), this is a performance regression. The original const& return was intentional and efficient; changing it to return by value should be justified or reverted.
| xtick_step=150): | ||
| """ Plots rolling sum of new infections per 24 hours location type for the median run. | ||
|
|
||
| @param[in] base_path Path to results directory. |
There was a problem hiding this comment.
The docstring for plot_infections_loc_types_average lists @param[in] base_path Path to results directory. on line 130, but the actual parameter name is path_to_loc_types. This inconsistency makes the docstring misleading.
| Simulation(TimePoint t0, Model&& model) | ||
| : m_model(std::move(model)) | ||
| , m_t(t0) | ||
| , m_t_prev(t0-hours(1)) |
There was a problem hiding this comment.
The m_t_prev member is initialized to t0 - hours(1) in the constructor. The Simulation::advance() function logs the initial system state before entering the loop by calling history.log(*this). At this initial log call, LogInfectionPerLocationTypePerAgeGroup uses prev_time = m_t_prev = t0 - hours(1) and curr_time = t0. For any person who starts in the Exposed state at t0, get_infection_state(prev_time) will return Susceptible (since prev_time is before their infection course starts), and get_infection_state(curr_time) returns Exposed. This causes all initially Exposed persons to be incorrectly counted as "new infections" in the very first log entry, even though they were set up as initially infected — not newly infected at that time step.
| struct LogInfectionStatePerAgeGroup : mio::LogAlways { | ||
| using Type = std::pair<mio::abm::TimePoint, Eigen::VectorXd>; | ||
| /** | ||
| * @brief Log the TimeSeries of the number of Person%s in an #InfectionState. | ||
| * @param[in] sim The simulation of the abm. | ||
| * @return A pair of the TimePoint and the TimeSeries of the number of Person%s in an #InfectionState. | ||
| */ | ||
| static Type log(const mio::abm::Simulation<>& sim) | ||
| { | ||
|
|
||
| Eigen::VectorXd sum = Eigen::VectorXd::Zero( | ||
| Eigen::Index((size_t)mio::abm::InfectionState::Count * sim.get_model().parameters.get_num_groups())); | ||
| const auto curr_time = sim.get_time(); | ||
| const auto persons = sim.get_model().get_persons(); | ||
|
|
||
|
|
||
| for (auto i = size_t(0); i < persons.size(); ++i) { | ||
| auto& p = persons[i]; | ||
| auto index = (((size_t)(mio::abm::InfectionState::Count)) * ((uint32_t)p.get_age().get())) + | ||
| ((uint32_t)p.get_infection_state(curr_time)); | ||
| // PRAGMA_OMP(atomic) | ||
| sum[index] += 1; | ||
| } | ||
| return std::make_pair(curr_time, sum); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The LogInfectionStatePerAgeGroup struct is defined in common_abm_loggers.h but is not used anywhere in the codebase — result_simulation.h uses LogInfectionState and LogInfectionPerLocationTypePerAgeGroup, and there are no other usages. Additionally, there is a commented-out PRAGMA_OMP(atomic) on line 212 that is dead code. These should either be removed or actually used.
| } | ||
|
|
||
| const std::string result_directory_standard = mio::path_join(result_dir, "standart_results"); | ||
| if (!mio::create_directory(result_directory_standard)) { |
There was a problem hiding this comment.
The directory name "standart_results" contains a typo: it should be "standard_results". This typo will appear in the output directory path visible to users.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1507 +/- ##
==========================================
- Coverage 97.39% 97.35% -0.04%
==========================================
Files 188 188
Lines 16539 16561 +22
==========================================
+ Hits 16108 16123 +15
- Misses 431 438 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
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
Checks by code reviewer(s)