646 Use graph model with abms #1065

jubicker · 2024-07-09T08:35:32Z

Changes and Information

Create simulation graph nodes and mobility edges for ABM
Add world id to world and location and include id in functions where necesarry
Add activeness vector for persons to world
Add tests and example for abm graph model
The performance has slightly decreased (5-10%), because an additional check in some functions is now necessary whether a person is active in the current world (e.g. in migration and interaction function). For more information about performance, see the comment below

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

codecov · 2024-07-09T08:56:50Z

Codecov Report

Attention: Patch coverage is 79.61165% with 42 lines in your changes missing coverage. Please review.

Project coverage is 95.87%. Comparing base (d59fbd7) to head (1f85548).
Report is 2 commits behind head on main.

Files	Patch %	Lines
cpp/models/abm/world.h	62.79%	16 Missing ⚠️
cpp/models/graph_abm/graph_abm_mobility.h	77.41%	14 Missing ⚠️
cpp/models/graph_abm/mobility_rules.cpp	56.25%	7 Missing ⚠️
cpp/models/abm/world.cpp	89.13%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1065      +/-   ##
==========================================
- Coverage   96.08%   95.87%   -0.22%     
==========================================
  Files         131      133       +2     
  Lines       11048    11188     +140     
==========================================
+ Hits        10616    10726     +110     
- Misses        432      462      +30

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

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

jubicker · 2024-07-24T09:05:55Z

Performance Measures:
The following are the mean results of 10 executions of the abm benchmark for a single thread (OMP_NUM_THREADS=1)

Main:

---------------------------------------------------------------------------
Benchmark                                 Time             CPU
---------------------------------------------------------------------------
abm_benchmark/abm_benchmark_50k        3178,6 ms      3177,1 ms
abm_benchmark/abm_benchmark_100k      7128,8 ms      7125,8 ms
abm_benchmark/abm_benchmark_200k      14771 ms       14766,3 ms

Graph_ABM:

---------------------------------------------------------------------------
Benchmark                                 Time             CPU
---------------------------------------------------------------------------
abm_benchmark/abm_benchmark_50k        3537,5 ms      3535,5 ms
abm_benchmark/abm_benchmark_100k      7611 ms         7608 ms
abm_benchmark/abm_benchmark_200k      15674,8 ms    15669,6 ms

So 5-10% performance loss.

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

reneSchm

I have not entirely gone through the graph_abm and tests directories yet, so take this as a preliminary review.

reneSchm · 2024-07-29T11:29:01Z

cpp/examples/graph_abm.cpp

+//Logger
+struct Logger : mio::LogAlways {


A slightly more descriptive name would be neat. It is fine as long as this stays in the example.

reneSchm · 2024-07-29T11:31:56Z

cpp/examples/graph_abm.cpp

+            location_information.push_back(std::make_tuple(loc.get_world_id(), loc.get_type(), loc.get_id(),
+                                                           sim.get_world().get_number_persons(loc.get_id()),
+                                                           persons_per_infection_state));


If this causes too many reallocations, you could reserve #worlds * #locations in world 0 vector entries.

reneSchm · 2024-07-29T11:50:02Z

cpp/memilio/mobility/graph_simulation.h

+          class edge_f = void (*)(Timepoint, Timespan, typename Graph::EdgeProperty&, typename Graph::NodeProperty&,
+                                  typename Graph::NodeProperty&),
+          class node_f = void (*)(Timepoint, Timespan, typename Graph::NodeProperty&)>


Since we store these functions, we should preferably use std::functions instead of function pointers. I don't think this will cause too much overhead, and it allows using e.g. a lambda as node function.

reneSchm · 2024-07-29T11:58:14Z

cpp/memilio/mobility/graph_simulation.h

+    using GraphSimulationBase<Graph, Timepoint, Timespan, edge_f, node_f>::GraphSimulationBase;
+    using Base = GraphSimulationBase<Graph, Timepoint, Timespan, edge_f, node_f>;


Suggested change

using GraphSimulationBase<Graph, Timepoint, Timespan, edge_f, node_f>::GraphSimulationBase;

using Base = GraphSimulationBase<Graph, Timepoint, Timespan, edge_f, node_f>;

using Base = GraphSimulationBase<Graph, Timepoint, Timespan, edge_f, node_f>;

using Base::GraphSimulationBase;

to avoid rewriting all template params, or even using Base::Base;.

reneSchm · 2024-07-30T06:50:35Z

cpp/memilio/mobility/metapopulation_mobility_instant.h

+GraphSimulation<Graph<SimulationNode<Sim>, MigrationEdge<FP>>, FP, FP,
+                void (*)(double, double, mio::MigrationEdge<>&, mio::SimulationNode<Sim>&, mio::SimulationNode<Sim>&),
+                void (*)(double, double, mio::SimulationNode<Sim>&)>


Probably should be an std::function as well

reneSchm · 2024-07-30T12:10:35Z

cpp/models/abm/world.h

+    /**
+     * Get activeness status of all persons in the world.
+     * @return Activeness vector
+     */
+    std::vector<bool>& get_activeness_statuses()
+    {
+        return m_activeness_statuses;
+    }


I think something like set_person_active(PersonId person_id, bool status) would be better than exposing a private member.

reneSchm · 2024-07-30T12:41:12Z

cpp/models/abm/world.h

+    /**
+     * @brief Invalidate local population and exposure rate cache.
+     */
+    void invalidate_cache()
+    {
+        m_are_exposure_caches_valid       = false;
+        m_is_local_population_cache_valid = false;
+    }


I am pretty certain this function needs to go. Whatever call causes the need to invalidate caches should already invalidate them.

reneSchm · 2024-07-30T12:44:59Z

cpp/models/abm/world.h

+    /**
+     * @brief Flip activeness status of a person in the world.
+     * @param[in] person_id PersonId of Person whose activeness status is fipped.
+     */
+    void change_activeness(PersonId person_id)
+    {
+        m_activeness_statuses[person_id.get()] = !m_activeness_statuses[person_id.get()];
+    }


This function requires already knowing the activeness state of a person. I prefer using the "set_person_active" function I propose above.

reneSchm · 2024-07-30T12:48:35Z

cpp/models/abm/world.h

+    /**
+     * @brief Copy the persons from another World to this World. 
+     * If the persons are at a location in this world they are activated, otherwise they are deactivated.
+     * If necessary the person ids are changed such that they correspond to the index in this world's m_persons vector.
+     * @param[in] other The World the Person%s are copied from.
+     */
+    void copy_persons_from_other_world(const World& other)


I would prefer adding an (optional) world_id to World::add_person and make this an external function using add_person.

reneSchm · 2024-07-30T12:51:36Z

cpp/models/abm/world.h

+    /**
+    * @brief Set the Person%s of the World.
+    * @param[in] persons The Person%s of the World.
+    */
+    void set_persons(std::vector<Person>& persons)


Since this too behaves very similar to for (p : persons) add_person(p) I would prefer making this an external function.
Maybe we should consider adding a builder class for World or even the entire World-Graph.

reneSchm · 2024-07-31T10:01:51Z

cpp/models/graph_abm/graph_abm_mobility.h

+                    //change person's location in all nodes
+                    mio::abm::migrate(person_n1, target_location);
+                    mio::abm::migrate(person_n2, target_location);
+                    // invalidate both worlds' cache
+                    node_to.get_simulation().get_world().invalidate_cache();
+                    node_from.get_simulation().get_world().invalidate_cache();


I see. We could implement a World::migrate that takes IDs, similar to World::interact. This could also take care of the caches.

reneSchm · 2024-07-31T10:12:13Z

cpp/models/graph_abm/graph_abm_mobility.h

+                auto& target_world    = (target_world_id == node_from.get_simulation().get_world().get_id())
+                                            ? node_from.get_simulation().get_world()
+                                            : node_to.get_simulation().get_world();


When the ternary condition is true, then why do we not stop here? The person won't travel along the edge, and just stay in its world - so this should be covered by World::migration, shouldn't it?

reneSchm · 2024-07-31T10:26:49Z

cpp/models/graph_abm/graph_abm_mobility.h

+                    mio::log_error("Person with index {} has two assigned locations of the same type.",
+                                   person_n1.get_id().get());


This error message seems to be misleading. An instance of Person cannot have two assigned locations of the same type, since they are stored in a vector with type as its index. So I am not sure what can actually trigger this error, maybe setting the initial location as an unassigned location?

jubicker · 2024-07-31T11:15:16Z

Some more Benchmarks:

I got the following results for 5 executions of the abm benchmarks for one single thread. For every benchmark I added inactive agents and compared the results for num_inactive_agents=0, num_agents, 2x num_agents, 3x num_agents.

For 50k agents:

---------------------------------------------------------------------------
Benchmark                        Time         Deviation from no inactive
---------------------------------------------------------------------------
abm_benchmark_50k_0_inactive     3568,8 ms     0%
abm_benchmark_50k_1_inactive     4072,6 ms     14%
abm_benchmark_50k_2_inactive     4584 ms       28%
abm_benchmark_50k_3_inactive     5093,8 ms     43%

For 100k agents:

---------------------------------------------------------------------------
Benchmark                        Time         Deviation from no inactive
---------------------------------------------------------------------------
abm_benchmark_100k_0_inactive     7707,2 ms     0%
abm_benchmark_100k_1_inactive     8705 ms       13%
abm_benchmark_100k_2_inactive     9699,6 ms     26%
abm_benchmark_100k_3_inactive     10720,2 ms    39%

For 200k agents:

---------------------------------------------------------------------------
Benchmark                        Time         Deviation from no inactive
---------------------------------------------------------------------------
abm_benchmark_200k_0_inactive     15850 ms     0%
abm_benchmark_200k_1_inactive     17898,6 ms   13%
abm_benchmark_200k_2_inactive     19949,2 ms   26%
abm_benchmark_200k_3_inactive     22033,6 ms   39%

xsaschako

i'm way more confident that this is okay after i went through it :-).

Just one comment before i do a full review:
Look out for a way to handle a default "all persons are active" so that maybe there is a way that this doesn't affect the performance in a normal abm at all :-)

add ABM simulation node and edge

f272d9c

jubicker linked an issue Jul 9, 2024 that may be closed by this pull request

Use graph model with abms #646

Open

2 tasks

jubicker added 20 commits July 11, 2024 16:01

graph abm implementation

1f29e1c

remove warning surpression for boost

5e91137

merge main

22a2a60

add CMakeList

4368cb5

CMake List Bug fix

81df1a8

fix include

5b2c64a

bug fix

414c3f4

test

4e2a733

test apply_mobility

8f1d7cb

extend apply_mobility test

7400802

error in abm_minimal example

b314715

test for apply mobility

dbbd068

finx clang bug

196275f

abm minimal bug fix

a5a4e88

Merge branch '646-use-graph-model-with-abms-v2' of https://github.com…

3367fa6

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

graph_abm example

8cce390

graph abm example

2fc9026

merge main

ef4c0b1

fix merge conflicts

0ece6be

fix test

b773193

jubicker added 4 commits July 25, 2024 10:22

graph abm example

b4d44d5

remove unnecessary comment

ae95301

Merge branch '646-use-graph-model-with-abms-v2' of https://github.com…

5ebd62f

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

fix bug

e22009b

jubicker requested a review from reneSchm July 25, 2024 08:42

delete dbugging comments

11c3ae8

Merge branch '646-use-graph-model-with-abms-v2' of https://github.com…

1f85548

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

reneSchm requested changes Jul 30, 2024

View reviewed changes

benchmark inactive persons

fdad4da

reneSchm reviewed Jul 31, 2024

View reviewed changes

xsaschako reviewed Jul 31, 2024

View reviewed changes

jubicker removed a link to an issue Aug 1, 2024

Use graph model with abms #646

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

646 Use graph model with abms #1065

646 Use graph model with abms #1065

jubicker commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

jubicker commented Jul 24, 2024 •

edited

Loading

reneSchm left a comment

reneSchm Jul 29, 2024

reneSchm Jul 29, 2024

reneSchm Jul 29, 2024

reneSchm Jul 29, 2024

reneSchm Jul 30, 2024

reneSchm Jul 30, 2024

reneSchm Jul 30, 2024

reneSchm Jul 30, 2024

reneSchm Jul 30, 2024

reneSchm Jul 30, 2024

reneSchm Jul 31, 2024

reneSchm Jul 31, 2024

reneSchm Jul 31, 2024

jubicker commented Jul 31, 2024 •

edited

Loading

xsaschako left a comment

		using GraphSimulationBase<Graph, Timepoint, Timespan, edge_f, node_f>::GraphSimulationBase;
		using Base = GraphSimulationBase<Graph, Timepoint, Timespan, edge_f, node_f>;

		mio::log_error("Person with index {} has two assigned locations of the same type.",
		person_n1.get_id().get());

		//Logger
		struct Logger : mio::LogAlways {

646 Use graph model with abms #1065

Are you sure you want to change the base?

646 Use graph model with abms #1065

Conversation

jubicker commented Jul 9, 2024 • edited Loading

Changes and Information

Merge Request - Guideline Checklist

Checks by code author

Checks by code reviewer(s)

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

jubicker commented Jul 24, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubicker commented Jul 31, 2024 • edited Loading

xsaschako left a comment

Choose a reason for hiding this comment

jubicker commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

jubicker commented Jul 24, 2024 •

edited

Loading

jubicker commented Jul 31, 2024 •

edited

Loading