Repository: projectmesa/mesa
Pull request #2007 (2024-01-26 12:26:36)

Fix AgentSet inplace shuffle (and thus RandomActivation), add tests
Adds a test that checks if the `RandomActivation` doesn't trigger agents in a sequential order. This test now fails, because as noted in #2006, it currently does do it in sequential order.

In theory this could give false positives (a test passing when it shouldn't, but that chance is around ~0.1^18).

github-actions[bot]: Performance benchmarks:

| Model        | Size   | Init time [95% CI]     | Run time [95% CI]      |
|--------------|--------|------------------------|------------------------|
| Schelling    | small  | 🔵 -1.3% [-1.5%, -1.1%] | 🔵 -0.5% [-0.6%, -0.3%] |
| Schelling    | large  | 🔵 -1.4% [-1.5%, -1.3%] | 🔵 -0.2% [-1.1%, +0.7%] |
| WolfSheep    | small  | 🔵 -0.2% [-0.5%, +0.2%] | 🔵 -0.2% [-0.3%, -0.1%] |
| WolfSheep    | large  | 🔵 +0.7% [-0.0%, +1.3%] | 🔵 -0.4% [-1.4%, +0.4%] |
| BoidFlockers | small  | 🔵 -0.2% [-0.6%, +0.2%] | 🔵 -0.4% [-1.0%, +0.3%] |
| BoidFlockers | large  | 🔵 +0.1% [-0.3%, +0.5%] | 🔵 -0.2% [-1.0%, +0.7%] |

quaquel: If you change `do_each` to

```python
    def do_each(self, method, shuffle=False):
        if shuffle:
            self._agents.shuffle(inplace=True)
        self._agents.do(method)
```

it works correctly. I am going to check RandomActivationByType as well.

Do I have write access to this?
quaquel: I also think this bug fix will actually produce a speedup because we avoid a needless copy operation of the AgentSet.

EwoutH: Thanks! For clarity, we should document the difference between `agents`, `_agents` and `agents_` somewhere
EwoutH: @quaquel could you add unittests for `shuffle`? And maybe we should check coverage of the AgentSet again at some point (not this PR).
quaquel: do you mean for AgentSet.shuffle?
quaquel: Can you trigger the benchmarks? I am curious to see what this fix has done with those given that it removes an unnecessary copy operation.
rht: /rerun-benchmarks
EwoutH: Looks good! Benchmarks depend on #2002.

Can’t approve this PR, but looks good to me.

Patch:
diff --git a/mesa/agent.py b/mesa/agent.py
index fcf643d3..6155284b 100644
--- a/mesa/agent.py
+++ b/mesa/agent.py
@@ -54,6 +54,7 @@ class Agent:
         except AttributeError:
             # model super has not been called
             self.model.agents_ = defaultdict(dict)
+            self.model.agents_[type(self)][self] = None
             self.model.agentset_experimental_warning_given = False
 
             warnings.warn(
diff --git a/mesa/datacollection.py b/mesa/datacollection.py
index 88f1863f..3fcd205c 100644
--- a/mesa/datacollection.py
+++ b/mesa/datacollection.py
@@ -14,8 +14,7 @@ name.
 
 When the collect() method is called, each model-level function is called, with
 the model as the argument, and the results associated with the relevant
-variable. Then the agent-level functions are called on each agent in the model
-scheduler.
+variable. Then the agent-level functions are called on each agent.
 
 Additionally, other objects can write directly to tables by passing in an
 appropriate dictionary object for a table row.
@@ -30,8 +29,7 @@ The DataCollector then stores the data it collects in dictionaries:
 Finally, DataCollector can create a pandas DataFrame from each collection.
 
 The default DataCollector here makes several assumptions:
-    * The model has a schedule object called 'schedule'
-    * The schedule has an agent list called agents
+    * The model has an agent list called agents
     * For collecting agent-level variables, agents must have a unique_id
 """
 import contextlib
@@ -67,7 +65,7 @@ class DataCollector:
 
         Model reporters can take four types of arguments:
         1. Lambda function:
-           {"agent_count": lambda m: m.schedule.get_agent_count()}
+           {"agent_count": lambda m: len(m.agents)}
         2. Method of a class/instance:
            {"agent_count": self.get_agent_count} # self here is a class instance
            {"agent_count": Model.get_agent_count} # Model here is a class
@@ -180,11 +178,14 @@ class DataCollector:
         rep_funcs = self.agent_reporters.values()
 
         def get_reports(agent):
-            _prefix = (agent.model.schedule.steps, agent.unique_id)
+            _prefix = (agent.model._steps, agent.unique_id)
             reports = tuple(rep(agent) for rep in rep_funcs)
             return _prefix + reports
 
-        agent_records = map(get_reports, model.schedule.agents)
+        agent_records = map(
+            get_reports,
+            model.schedule.agents if hasattr(model, "schedule") else model.agents,
+        )
         return agent_records
 
     def collect(self, model):
@@ -207,7 +208,7 @@ class DataCollector:
 
         if self.agent_reporters:
             agent_records = self._record_agents(model)
-            self._agent_records[model.schedule.steps] = list(agent_records)
+            self._agent_records[model._steps] = list(agent_records)
 
     def add_table_row(self, table_name, row, ignore_missing=False):
         """Add a row dictionary to a specific table.
diff --git a/mesa/experimental/jupyter_viz.py b/mesa/experimental/jupyter_viz.py
index 7a586239..eb20ca5f 100644
--- a/mesa/experimental/jupyter_viz.py
+++ b/mesa/experimental/jupyter_viz.py
@@ -181,7 +181,7 @@ def ModelController(model, play_interval, current_step, reset_counter):
     def do_step():
         model.step()
         previous_step.value = current_step.value
-        current_step.value += 1
+        current_step.value = model._steps
 
     def do_play():
         model.running = True
diff --git a/mesa/model.py b/mesa/model.py
index 1bd9b3a7..b2b12340 100644
--- a/mesa/model.py
+++ b/mesa/model.py
@@ -13,11 +13,13 @@ import warnings
 from collections import defaultdict
 
 # mypy
-from typing import Any
+from typing import Any, Union
 
 from mesa.agent import Agent, AgentSet
 from mesa.datacollection import DataCollector
 
+TimeT = Union[float, int]
+
 
 class Model:
     """Base class for models in the Mesa ABM library.
@@ -68,6 +70,9 @@ class Model:
         self.current_id = 0
         self.agents_: defaultdict[type, dict] = defaultdict(dict)
 
+        self._steps: int = 0
+        self._time: TimeT = 0  # the model's clock
+
         # Warning flags for current experimental features. These make sure a warning is only printed once per model.
         self.agentset_experimental_warning_given = False
 
@@ -112,6 +117,11 @@ class Model:
     def step(self) -> None:
         """A single step. Fill in here."""
 
+    def _advance_time(self, deltat: TimeT = 1):
+        """Increment the model's steps counter and clock."""
+        self._steps += 1
+        self._time += deltat
+
     def next_id(self) -> int:
         """Return the next unique ID for agents, increment current_id"""
         self.current_id += 1
diff --git a/mesa/time.py b/mesa/time.py
index 87a68949..50d564be 100644
--- a/mesa/time.py
+++ b/mesa/time.py
@@ -73,6 +73,8 @@ class BaseScheduler:
         self.model = model
         self.steps = 0
         self.time: TimeT = 0
+        self._original_step = self.step
+        self.step = self._wrapped_step
 
         if agents is None:
             agents = []
@@ -115,6 +117,11 @@ class BaseScheduler:
         self.steps += 1
         self.time += 1
 
+    def _wrapped_step(self):
+        """Wrapper for the step method to include time and step updating."""
+        self._original_step()
+        self.model._advance_time()
+
     def get_agent_count(self) -> int:
         """Returns the current number of agents in the queue."""
         return len(self._agents)
@@ -143,8 +150,8 @@ class BaseScheduler:
 
     def do_each(self, method, shuffle=False):
         if shuffle:
-            self.agents.shuffle(inplace=True)
-        self.agents.do(method)
+            self._agents.shuffle(inplace=True)
+        self._agents.do(method)
 
 
 class RandomActivation(BaseScheduler):


Test patch:
diff --git a/tests/test_agent.py b/tests/test_agent.py
index 5861038d..60a0eec1 100644
--- a/tests/test_agent.py
+++ b/tests/test_agent.py
@@ -251,3 +251,16 @@ def test_agentset_select_by_type():
     # Test with no type specified (should select all agents)
     all_agents = agentset.select()
     assert len(all_agents) == len(mixed_agents)
+
+
+def test_agentset_shuffle():
+    model = Model()
+    test_agents = [TestAgent(model.next_id(), model) for _ in range(12)]
+
+    agentset = AgentSet(test_agents, model=model)
+    agentset = agentset.shuffle()
+    assert not all(a1 == a2 for a1, a2 in zip(test_agents, agentset))
+
+    agentset = AgentSet(test_agents, model=model)
+    agentset.shuffle(inplace=True)
+    assert not all(a1 == a2 for a1, a2 in zip(test_agents, agentset))
diff --git a/tests/test_time.py b/tests/test_time.py
index 3cfc2f35..53967d1a 100644
--- a/tests/test_time.py
+++ b/tests/test_time.py
@@ -224,6 +224,28 @@ class TestRandomActivation(TestCase):
         agent_ids = {agent.unique_id for agent in model.agents}
         assert all(entry in agent_ids for entry in keys)
 
+    def test_not_sequential(self):
+        model = MockModel(activation=RANDOM)
+        # Create 10 agents
+        for _ in range(10):
+            model.schedule.add(MockAgent(model.next_id(), model))
+        # Run 3 steps
+        for _ in range(3):
+            model.step()
+        # Filter out non-integer elements from the log
+        filtered_log = [item for item in model.log if isinstance(item, int)]
+
+        # Check that there are no 18 consecutive agents id's in the filtered log
+        total_agents = 10
+        assert not any(
+            all(
+                (filtered_log[(i + j) % total_agents] - filtered_log[i]) % total_agents
+                == j % total_agents
+                for j in range(18)
+            )
+            for i in range(len(filtered_log))
+        ), f"Agents are activated sequentially:\n{filtered_log}"
+
 
 class TestSimultaneousActivation(TestCase):
     """