Repository: projectmesa/mesa
Pull request #2399 (2024-10-22 13:27:07)

fixes weakref bug in shuffe_do
if one agent removes another agent, it could still be activated by shuffle_do. This adds a unit test to spot the bug and contains the fix.

# longer explanation
shuffle_do used `agents = list(self._agents.keys())`. This returns a list of hardrefs to all agents. So if `agents[0]` removes `agents[1]`, it will be removed from the model, but it is still activated in `shuffle_do`. we had the same bug before in shuffle in january. 

github-actions[bot]: Performance benchmarks:

| Model           | Size   | Init time [95% CI]       | Run time [95% CI]          |
|-----------------|--------|--------------------------|----------------------------|
| BoltzmannWealth | small  | 🔴 +9.5% [+8.3%, +10.8%] | 🔵 -0.1% [-0.3%, +0.1%]    |
| BoltzmannWealth | large  | 🔵 -0.3% [-0.7%, +0.2%]  | 🔴 +5.2% [+4.8%, +5.7%]    |
| Schelling       | small  | 🔵 +1.4% [+1.0%, +1.7%]  | 🔵 -0.8% [-1.2%, -0.6%]    |
| Schelling       | large  | 🔵 +1.3% [+0.3%, +2.5%]  | 🔴 +7.9% [+6.4%, +9.3%]    |
| WolfSheep       | small  | 🔵 +2.0% [+1.8%, +2.3%]  | 🔵 +1.0% [+0.8%, +1.3%]    |
| WolfSheep       | large  | 🔴 +5.8% [+4.5%, +6.9%]  | 🔴 +21.6% [+19.3%, +23.6%] |
| BoidFlockers    | small  | 🔵 +3.2% [+2.8%, +3.7%]  | 🔵 +1.0% [+0.2%, +1.7%]    |
| BoidFlockers    | large  | 🔵 +4.3% [+2.8%, +5.9%]  | 🔵 +2.3% [+1.3%, +3.3%]    |

github-actions[bot]: Performance benchmarks:

| Model           | Size   | Init time [95% CI]      | Run time [95% CI]        |
|-----------------|--------|-------------------------|--------------------------|
| BoltzmannWealth | small  | 🔵 +1.0% [-0.0%, +2.1%] | 🔵 +0.3% [+0.2%, +0.5%]  |
| BoltzmannWealth | large  | 🔵 +0.4% [+0.0%, +0.9%] | 🔵 +3.5% [+2.5%, +4.4%]  |
| Schelling       | small  | 🔵 +0.5% [+0.1%, +0.9%] | 🔵 +0.1% [-0.1%, +0.4%]  |
| Schelling       | large  | 🔵 -0.1% [-1.1%, +0.9%] | 🔵 +0.4% [-0.7%, +1.7%]  |
| WolfSheep       | small  | 🔵 +0.2% [-0.2%, +0.5%] | 🔵 +1.9% [+1.6%, +2.1%]  |
| WolfSheep       | large  | 🔵 +1.8% [+0.6%, +3.0%] | 🔴 +9.0% [+6.9%, +11.1%] |
| BoidFlockers    | small  | 🔵 +0.7% [+0.3%, +1.1%] | 🔵 -0.1% [-0.7%, +0.6%]  |
| BoidFlockers    | large  | 🔵 +0.1% [-0.5%, +0.7%] | 🔵 -0.0% [-0.8%, +0.8%]  |

EwoutH: Right, this happens because an Agent can get removed within a step.

Have you done any benchmarks by any chance?
quaquel: > Right, this happens because an Agent can get removed within a step.

Yes exactly. I ran into it with a wolf sheep version where I simply used self.agents.shuffle_do, rather than activate by class. The test no captures the problem.

> Have you done any benchmarks by any chance?

Not beyond the default mesa benchmarks. Feel free to rerun your original tests. My hunch is that the performance after the fix will be marginally slower but not much than before the bugfix. The delta with chaining should however still persist because  you iterate over the agents only once instead of twice. 



Patch:
diff --git a/mesa/agent.py b/mesa/agent.py
index 200dab6f..868acf25 100644
--- a/mesa/agent.py
+++ b/mesa/agent.py
@@ -294,15 +294,17 @@ class AgentSet(MutableSet, Sequence):
 
         It's a fast, optimized version of calling shuffle() followed by do().
         """
-        agents = list(self._agents.keys())
-        self.random.shuffle(agents)
+        weakrefs = list(self._agents.keyrefs())
+        self.random.shuffle(weakrefs)
 
         if isinstance(method, str):
-            for agent in agents:
-                getattr(agent, method)(*args, **kwargs)
+            for ref in weakrefs:
+                if (agent := ref()) is not None:
+                    getattr(agent, method)(*args, **kwargs)
         else:
-            for agent in agents:
-                method(agent, *args, **kwargs)
+            for ref in weakrefs:
+                if (agent := ref()) is not None:
+                    method(agent, *args, **kwargs)
 
         return self
 


Test patch:
diff --git a/tests/test_agent.py b/tests/test_agent.py
index aa5ff935..f14c755e 100644
--- a/tests/test_agent.py
+++ b/tests/test_agent.py
@@ -472,6 +472,29 @@ def test_agentset_shuffle_do():
         original_order != shuffled_order
     ), "The order should be different after shuffle_do"
 
+    class AgentWithRemove(Agent):
+        def __init__(self, model):
+            super().__init__(model)
+            self.is_alive = True
+
+        def remove(self):
+            super().remove()
+            self.is_alive = False
+
+        def step(self):
+            if not self.is_alive:
+                raise Exception
+
+            agent_to_remove = self.random.choice(self.model.agents)
+
+            if agent_to_remove is not self:
+                agent_to_remove.remove()
+
+    model = Model(seed=32)
+    for _ in range(100):
+        AgentWithRemove(model)
+    model.agents.shuffle_do("step")
+
 
 def test_agentset_get_attribute():
     """Test AgentSet.get for attributes."""