Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1515 +/- ##
=======================================
Coverage 97.38% 97.38%
=======================================
Files 188 188
Lines 16621 16621
=======================================
Hits 16186 16186
Misses 435 435 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DavidKerkmann
left a comment
DavidKerkmann
left a comment
There was a problem hiding this comment.
There are a couple of generalized questions that we need to tackle now that we are bringing python bindings to the simulation level.
| template <class T> | ||
| concept HasSampleFunction = requires(T t) { | ||
| { t.get_sample(std::declval<RandomNumberGenerator&>()) } -> std::convertible_to<ScalarType>; | ||
| { t.get_sample(std::declval<abm::PersonalRandomNumberGenerator&>()) } -> std::convertible_to<ScalarType>; |
There was a problem hiding this comment.
The abstract parameter disctribution is in utils/ and should remain model-independent. An abm::PersonalRandomNumberGenerator is only known when there is knowledge about the ABM..
Is this required at this point? Requiring an abm::PRNG makes this unusable for other models.
| model.parameters.AgeGroupGotoSchool[AgeGroup(age_group)] = False | ||
| model.parameters.AgeGroupGotoWork[AgeGroup(age_group)] = False |
There was a problem hiding this comment.
As the default is false, these line could be removed, unless you have an intention to explicitly write them down.
There was a problem hiding this comment.
true, removed also from the cpp version
| for age in range(num_age_groups): | ||
| model.parameters.InfectionProtectionFactor[abm.ProtectionType.GenericVaccine, AgeGroup( | ||
| age), abm.VirusVariant.Wildtype] = mio.TimeSeriesFunctor( | ||
| [[0, 0.0], [14, 0.67], [180, 0.4]]) | ||
|
|
||
| model.parameters.SeverityProtectionFactor[abm.ProtectionType.GenericVaccine, AgeGroup( | ||
| age), abm.VirusVariant.Wildtype] = mio.TimeSeriesFunctor( | ||
| [[0, 0.0], [14, 0.85], [180, 0.7]]) |
There was a problem hiding this comment.
This part is not in the .cpp file. We should probably stay as close as possible to the .cpp example. If we want to add this, then we should also add it in the .cpp example.
There was a problem hiding this comment.
In the cpp version this is divided into the abm_minimal and the abm_vaccination example. Agree that we should probably stay close, so either merge them in the cpp version or split it here. Do you have a favorite solution?
| # Seed infections | ||
|
|
||
| infection_distribution = [0.5, 0.3, 0.05, 0.05, 0.05, 0.05, 0.0, 0.0] | ||
| rng = np.random.default_rng() |
There was a problem hiding this comment.
With the availability of the MEmilio RNG / Personal RNG we should use that one here instead of the numpy rng. The discrete distribution would be suitable.
| .def_property_readonly("is_in_quarantine", &mio::abm::Person::is_in_quarantine); | ||
| .def_property_readonly("is_in_quarantine", &mio::abm::Person::is_in_quarantine) | ||
| .def_property_readonly("id", &mio::abm::Person::get_id) | ||
| .def("add_new_infection", |
There was a problem hiding this comment.
I see that this looked different at some point. Normally, as far as I can tell, we always bind the functions 1:1, meaning that we use the same arguments as in the cpp code. Here, the model is passed instead of the rng, and then the function itself creates the PRNG. However, then we could go a step further and also remove the age for example, as it could also be inferred directly from the person.
In general, we should decide how we want to go about these things. In a way, we lose flexibility if the user intends to use a different rng for some reason here. However, I think it also simplifies the use and perhaps we could bind functions for easier use in general.
There was a problem hiding this comment.
added the bindings for the prng and infections. Maybe we can discuss if we want to provide simplified functionality for python at a later time.
| .def( | ||
| "advance", | ||
| [](mio::abm::Simulation<>& sim, mio::abm::TimePoint tmax) { | ||
| sim.advance(tmax); | ||
| }, | ||
| py::arg("tmax")) | ||
| .def( | ||
| "advance", | ||
| [](mio::abm::Simulation<>& sim, mio::abm::TimePoint tmax, | ||
| mio::History<mio::abm::TimeSeriesWriter, mio::abm::LogInfectionState>& history) { | ||
| sim.advance(tmax, history); | ||
| }, | ||
| py::arg("tmax"), py::arg("history")) |
There was a problem hiding this comment.
This only allows for two specific advance calls. A general functionality with any history object would be preferred (in conjuction with the binding of the History).
There was a problem hiding this comment.
Added some functionality. Let's discuss if that is our prefereable solution
| { | ||
| bind_class<mio::AbstractParameterDistribution, EnablePickling::Never>(m, name.c_str()) | ||
| .def(py::init<>()) | ||
| .def(py::init<mio::ParameterDistributionLogNormal>(), py::arg("dist")) |
There was a problem hiding this comment.
This should be generic for any distribution (see templated definition).
There was a problem hiding this comment.
Pull request overview
Adds/extends pybind11 bindings to enable running a minimal ABM workflow from Python, including additional utility bindings needed by the ABM parameterization and logging.
Changes:
- Expose additional core utility types to Python (e.g., RNG/distributions, time-series functor, abstract parameter distributions, lognormal distribution).
- Extend ABM bindings to support richer parameter arrays, histories/loggers, and additional model/simulation helpers.
- Add a Python minimal ABM example script demonstrating the new bindings.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| pycode/memilio-simulation/memilio/simulation/bindings/utils/time_series.cpp | Adds a Python constructor for TimeSeries from a table. |
| pycode/memilio-simulation/memilio/simulation/bindings/utils/random_number_generator.h | Introduces RNG + discrete distribution bindings. |
| pycode/memilio-simulation/memilio/simulation/bindings/utils/parameter_distributions.h | Declares lognormal distribution binding entry point. |
| pycode/memilio-simulation/memilio/simulation/bindings/utils/parameter_distributions.cpp | Implements ParameterDistributionLogNormal binding. |
| pycode/memilio-simulation/memilio/simulation/bindings/utils/index.h | Exposes Index.get() to Python. |
| pycode/memilio-simulation/memilio/simulation/bindings/utils/custom_index_array.h | Minor comment formatting fix. |
| pycode/memilio-simulation/memilio/simulation/bindings/utils/abstract_parameter_distribution.h | Adds binding for AbstractParameterDistribution. |
| pycode/memilio-simulation/memilio/simulation/bindings/simulation.cpp | Wires new utility bindings into the main _simulation module. |
| pycode/memilio-simulation/memilio/simulation/bindings/pybind_util.h | Fixes Range.__iter__ to return an iterator object. |
| pycode/memilio-simulation/memilio/simulation/bindings/models/abm.cpp | Extends ABM bindings (parameters, histories, model/simulation helpers). |
| pycode/memilio-simulation/memilio/simulation/bindings/math/time_series_functor.h | Adds TimeSeriesFunctor Python binding. |
| pycode/memilio-simulation/memilio/simulation/bindings/geography/geolocation.cpp | Adds GeographicalLocation binding (currently included from .cpp). |
| pycode/examples/simulation/abm_minimal_example.py | New Python example using the new ABM bindings. |
| cpp/memilio/utils/abstract_parameter_distribution.h | Introduces a C++20 concept constraint for distribution implementations. |
| cpp/examples/abm_minimal.cpp | Small tweak to the C++ minimal ABM example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* | ||
| * Copyright (C) 2020-2026 MEmilio | ||
| * | ||
| * Authors: Carlotta Gerstein | ||
| * |
There was a problem hiding this comment.
This binding header defines non-templated functions but has no include guard / #pragma once. If it is included twice (directly or indirectly) it will cause redefinition errors. Add an include guard and consider moving non-inline function definitions into a .cpp (or mark them inline) to avoid ODR issues.
| .def_property_readonly("key", &mio::RandomNumberGenerator::get_key) | ||
| .def_property("counter", &mio::RandomNumberGenerator::get_counter, &mio::RandomNumberGenerator::set_counter) |
There was a problem hiding this comment.
RandomNumberGenerator::get_key() returns mio::Key<uint64_t> and get_counter() returns mio::Counter<uint64_t>, but neither Key nor Counter are bound to Python (they are TypeSafe wrappers with explicit conversion). Exposing them as properties will make rng.key/rng.counter unusable and set_counter impossible to call. Bind these wrapper types (incl. .get()), or expose key/counter as plain uint64_t in the binding.
| .def_property_readonly("key", &mio::RandomNumberGenerator::get_key) | |
| .def_property("counter", &mio::RandomNumberGenerator::get_counter, &mio::RandomNumberGenerator::set_counter) | |
| .def_property_readonly("key", [](const mio::RandomNumberGenerator& self) { | |
| return self.get_key().get(); | |
| }) | |
| .def_property( | |
| "counter", [](const mio::RandomNumberGenerator& self) { | |
| return self.get_counter().get(); | |
| }, | |
| [](mio::RandomNumberGenerator& self, uint64_t counter) { | |
| self.set_counter(mio::Counter<uint64_t>(counter)); | |
| }) |
| .def( | ||
| "add_new_infection", | ||
| [](mio::abm::Person& self, mio::abm::Infection infection) { | ||
| self.add_new_infection(std::move(infection)); | ||
| }, | ||
| py::return_value_policy::reference_internal) |
There was a problem hiding this comment.
Person::add_new_infection is void in C++, but this binding passes py::return_value_policy::reference_internal and returns nothing from the lambda. Remove the return policy (or change the binding to return something meaningful if that’s intended) to avoid misleading API and potential pybind issues.
| .def( | |
| "add_new_infection", | |
| [](mio::abm::Person& self, mio::abm::Infection infection) { | |
| self.add_new_infection(std::move(infection)); | |
| }, | |
| py::return_value_policy::reference_internal) | |
| .def("add_new_infection", [](mio::abm::Person& self, mio::abm::Infection infection) { | |
| self.add_new_infection(std::move(infection)); | |
| }) |
| bind_timeserieswriter_history<mio::abm::LogInfectionState>(m, "TimeSeriesWriterLogInfectionStateHistory"); | ||
| bind_datawriter_history<mio::abm::LogLocationInformation>(m, "DataWriterLogLocationInformationHistory"); | ||
| bind_datawriter_history<mio::abm::LogPersonInformation>(m, "DataWriterLogPersonInformationHistory"); | ||
| bind_datawriter_history<mio::abm::LogDataForMobility>(m, "DataWriterLogDataForMobilityHistory"); |
There was a problem hiding this comment.
The ABM bindings add several new Python-facing APIs (e.g., history wrapper classes and additional Simulation.advance(...) overloads that accept different History types, plus RNG access/seeding helpers). tests/test_abm.py exists but doesn’t cover these new behaviors—please add unit tests that exercise at least one history type with Simulation.advance(tmax, history) and validate the logged output shape/content.
| pymio::bind_random_number_generator(m, "RandomNumberGenerator"); | ||
| pymio::bind_discrete_distribution(m, "DiscreteDistribution"); | ||
|
|
||
| pymio::bind_time_series_functor(m, "TimeSeriesFunctor"); | ||
|
|
There was a problem hiding this comment.
RandomNumberGenerator, DiscreteDistribution, TimeSeriesFunctor, and GeographicalLocation are newly bound here, but there are no tests exercising them. Please add focused tests (e.g., sampling advances the RNG counter, TimeSeriesFunctor evaluates expected values, GeographicalLocation stores coordinates) to catch binding issues early.
| self.add_new_infection(std::move(infection)); | ||
| }, | ||
| py::return_value_policy::reference_internal) | ||
| .def("add_new_vaccination", &mio::abm::Person::add_new_vaccination, py::return_value_policy::reference_internal) |
There was a problem hiding this comment.
Person::add_new_vaccination is void in C++, so specifying py::return_value_policy::reference_internal here is misleading. Remove the return policy (or adjust the binding to return a value) to keep the Python API consistent and avoid pybind warnings.
| .def("add_new_vaccination", &mio::abm::Person::add_new_vaccination, py::return_value_policy::reference_internal) | |
| .def("add_new_vaccination", &mio::abm::Person::add_new_vaccination) |
|
|
||
| auto sim_cls = pymio::bind_class<mio::abm::Simulation<>, pymio::EnablePickling::Never>(m, "Simulation") | ||
| .def(py::init<mio::abm::TimePoint, size_t>()) | ||
| .def(py::init([](mio::abm::TimePoint t, mio::abm::Model& model) { |
There was a problem hiding this comment.
This constructor binding takes Model& and then std::move(model) into Simulation(TimePoint, Model&&). That will move-from the original Python Model instance, leaving the Python object in a valid-but-unspecified (effectively broken) state. Prefer taking the model by value (copy) and moving the local copy, or redesign the binding to transfer ownership explicitly (e.g., via std::unique_ptr<Model>).
| .def(py::init([](mio::abm::TimePoint t, mio::abm::Model& model) { | |
| .def(py::init([](mio::abm::TimePoint t, mio::abm::Model model) { |
| { | ||
| bind_class<mio::TimeSeries<double>, EnablePickling::Required>(m, name.c_str()) | ||
| .def(py::init<Eigen::Index>(), py::arg("num_elements")) | ||
| .def(py::init<std::vector<std::vector<double>>>(), py::arg("table")) |
There was a problem hiding this comment.
The new TimeSeries(table) Python constructor forwards directly to mio::TimeSeries(std::vector<std::vector<...>>) which requires a non-empty table with equal row sizes. For invalid input (e.g. empty list), the C++ constructor hits assertions / may access table.front() and can crash the Python process in release builds. Wrap this constructor in a lambda that validates the table shape and raises py::value_error instead of relying on assert.
| .def(py::init<std::vector<std::vector<double>>>(), py::arg("table")) | |
| .def(py::init([](const std::vector<std::vector<double>>& table) { | |
| if (table.empty()) { | |
| throw py::value_error("TimeSeries table must not be empty."); | |
| } | |
| const auto row_size = table.front().size(); | |
| if (row_size == 0) { | |
| throw py::value_error("TimeSeries table rows must not be empty."); | |
| } | |
| for (const auto& row : table) { | |
| if (row.size() != row_size) { | |
| throw py::value_error("TimeSeries table must have equal row sizes."); | |
| } | |
| } | |
| return mio::TimeSeries<double>(table); | |
| }), | |
| py::arg("table")) |
| void bind_abstract_parameter_distribution(py::module_& m, std::string const& name) | ||
| { | ||
| bind_class<mio::AbstractParameterDistribution, EnablePickling::Never>(m, name.c_str()) | ||
| .def(py::init<>()) |
There was a problem hiding this comment.
The binding exposes AbstractParameterDistribution() default construction to Python. In C++, the default-constructed object calls exit(...) when get(...) is invoked, which would terminate the Python process. Consider removing the default constructor from the Python API (or wrapping get to raise a Python exception when no distribution is set) to avoid hard process exits from Python code.
| .def(py::init<>()) |
| pymio::bind_parameter_distribution(m, "ParameterDistribution"); | ||
| pymio::bind_parameter_distribution_normal(m, "ParameterDistributionNormal"); | ||
| pymio::bind_parameter_distribution_lognormal(m, "ParameterDistributionLogNormal"); | ||
| pymio::bind_parameter_distribution_uniform(m, "ParameterDistributionUniform"); | ||
| pymio::bind_uncertain_value(m, "UncertainValue"); |
There was a problem hiding this comment.
This module now exposes new/extended Python APIs (e.g., ParameterDistributionLogNormal and AbstractParameterDistribution, plus additional bindings later in the file). There are existing Python unit tests under pycode/memilio-simulation/tests/, but these new bindings are not covered—please add tests that exercise the new entry points to prevent regressions.
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
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)
Closes #1503