Repository: projectmesa/mesa

Issue:
Removing all agents in the model
I am currently updating my teaching materials to MESA 3. One of my assignments is an evolutionary version of Axelrod's emergence of collaboration. However, this requires removing the entire population of agents at each tick, and creating the next generation. (I know it can also be implemented differently...). At the moment, there is no convenience method for removing all agents from the model. Since `model.agents` returns a weakref agentset, doing operations on this won't work. It seems we need a model level method like `model.clear_agents()`, which would call `agent.remove` on each agent. It needs to run through `agent.remove` rather than just rebuild the model._agents datastructures to ensure that agents are also removed from the experimental cell spaces.

Discussion:
github-actions[bot]: Performance benchmarks:

| Model           | Size   | Init time [95% CI]      | Run time [95% CI]       |
|-----------------|--------|-------------------------|-------------------------|
| BoltzmannWealth | small  | 🔵 +0.4% [-1.0%, +2.1%] | 🔵 -0.8% [-1.0%, -0.6%] |
| BoltzmannWealth | large  | 🔵 +2.0% [+1.1%, +2.9%] | 🔴 +6.5% [+4.4%, +8.4%] |
| Schelling       | small  | 🔴 +3.6% [+3.2%, +4.0%] | 🔵 +0.8% [+0.5%, +1.1%] |
| Schelling       | large  | 🔵 +1.0% [+0.3%, +1.7%] | 🔵 -5.0% [-7.8%, -1.5%] |
| WolfSheep       | small  | 🔵 -1.4% [-1.7%, -1.1%] | 🔵 -0.8% [-1.1%, -0.6%] |
| WolfSheep       | large  | 🔵 -2.9% [-4.0%, -2.1%] | 🟢 -6.9% [-8.8%, -5.0%] |
| BoidFlockers    | small  | 🔵 +0.5% [+0.0%, +1.0%] | 🔵 -0.1% [-0.8%, +0.5%] |
| BoidFlockers    | large  | 🔵 -0.3% [-1.1%, +0.4%] | 🔵 -1.2% [-1.9%, -0.6%] |

EwoutH: Let's make it explicit what it does and call it `remove_all_agents`.

`remove_agents` we may want to use for something else also, like removing a certain AgentSet or list of Agents, or doing it conditionally, etc.
quaquel: I made the suggested name change. 
EwoutH: Thanks!

Do we need to handle Agents being present in Spaces or other structures? (luckily they don't have schedulers anymore.) Or document something on how to handle this? And maybe test?
quaquel: This starts to feel like a round trip....

Tests were already included. I deliberately use agent.remove internally so it works for e.g., CellAgents. This is also stated in #2393. I will not bother with the existing spaces because they are a massive memory leak already and outside scope. The best solution for those is to subclass Agent.remove explicitly in your own custom agent class. 
EwoutH: > This starts to feel like a round trip....

I understand the feeling. I think there's a balance here. From my perspective, I can do a full, proper review end of day when I can take the time for it, and bundle anything immeadiatly.

Or I can jot down a quick thought with the risk that I might have another one a bit later. In this case it were two thoughts within half an hour of the PR opening.

We also do have to acknowledge that PR authors and PR reviewers have different roles and thus different responsibilities, but both have an essential role to play. I understand it can feel a bit asymmetrical because you have done most feature work last week, but I know exactly what it feel like. Part of it is avoidable, but part of it is an essential pain of pushing things through at high velocity in a library committed to a long-term stable API.

Adding something to a standardized, user facing API just come with additional effort / costs compared to doing it yourself within the model.

I'm convinced if we keep appreciating both sides of this coin, whichever we occupy in a certain PR, we'll keep improving. I can't promise it will never be without any pain (for example, I do feel #2394), because sometimes a little pain a huge long term benefits. We can't prevent everything, but we can prevent a lot and should strive for that.

So do call things out, but always assume good intentions. If we need to discuss this further (in a call / face to face) let me know.

<hr>

> Tests were already included.

I a bit of robustness would be nice here. Could you add some Agents to a custom AgentSet and Grid, and see if they are properly removed?

>  I deliberately use agent.remove internally so it works for e.g., CellAgents.

That's really nice, didn't know that!

> This is also stated in https://github.com/projectmesa/mesa/issues/2393.

In my view, PR descriptions should be readable on their own. We have [templates](https://github.com/projectmesa/mesa/tree/main/.github/PULL_REQUEST_TEMPLATE) for these, let me check if I can add them to a menu, never got to that.

They don't have to be epistels, sometimes a single line per header could be enough.

Personally, I have largely automated this process using LLMs. For example, I find if you add the issue description, the code diff (add .diff to the PR URL (https://github.com/projectmesa/mesa/pull/2394.diff)) and optionally a few other important points/thought, LLMs are amazing in formatting and generating usage examples.

<details><summary>Prompt</summary>
Write a concise PR description using the issue and diff, following the PR template.

Removing all agents in the model
I am currently updating my teaching materials to MESA 3. One of my assignments is an evolutionary version of Axelrod's emergence of collaboration. However, this requires removing the entire population of agents at each tick, and creating the next generation. (I know it can also be implemented differently...). At the moment, there is no convenience method for removing all agents from the model. Since `model.agents` returns a weakref agentset, doing operations on this won't work. It seems we need a model level method like `model.clear_agents()`, which would call `agent.remove` on each agent. It needs to run through `agent.remove` rather than just rebuild the model._agents datastructures to ensure that agents are also removed from the experimental cell spaces.

```
diff --git a/mesa/model.py b/mesa/model.py
index ac59cc1f1f9..ac6d5d3215c 100644
--- a/mesa/model.py
+++ b/mesa/model.py
@@ -276,3 +276,9 @@ def initialize_data_collector(
         )
         # Collect data for the first time during initialization.
         self.datacollector.collect(self)
+
+    def remove_all_agents(self):
+        """Remove all agents from the model."""
+        # we need to wrap keys in a list to avoid a RunTimeError: dictionary changed size during iteration
+        for agent in list(self._agents.keys()):
+            agent.remove()
diff --git a/tests/test_model.py b/tests/test_model.py
index 7c343e46357..ff1fe297b6a 100644
--- a/tests/test_model.py
+++ b/tests/test_model.py
@@ -93,3 +93,18 @@ class Sheep(Agent):
     assert model.agents_by_type[Wolf] == AgentSet([wolf], model)
     assert model.agents_by_type[Sheep] == AgentSet([sheep], model)
     assert len(model.agents_by_type) == 2
+
+
+def test_agent_remove():
+    """Test removing all agents from the model."""
+
+    class TestAgent(Agent):
+        pass
+
+    model = Model()
+    for _ in range(100):
+        TestAgent(model)
+    assert len(model.agents) == 100
+
+    model.remove_all_agents()
+    assert len(model.agents) == 0
```

PR template

```
## Summary
<!-- Provide a concise summary of the feature and its purpose. -->

## Motive
<!-- Explain the reasoning behind this feature. Include details on the problem it addresses or the enhancement it provides. -->

## Implementation
<!-- Describe how the feature was implemented. Include details on the approach taken, important decisions made, and code changes. -->

## Usage Examples
<!-- Provide code snippets or examples demonstrating how to use the new feature. Highlight key scenarios where this feature will be beneficial.

If you're modifying the visualisation, add before/after screenshots. -->

## Additional Notes
<!-- Add any additional information that may be relevant for the reviewers, such as potential side effects, dependencies, or related work. -->
```
</details>

<details><summary>Response</summary>
Here's a concise PR description following the provided template:

## Summary
Add a `remove_all_agents()` method to the Model class to facilitate removing all agents from a model in a single operation.

## Motive
This feature addresses the need to efficiently remove all agents from a model, particularly useful in scenarios like evolutionary simulations where the entire population needs to be replaced each tick. Currently, there's no convenient way to perform this operation, especially considering the weakref nature of `model.agents`.

## Implementation
- Added `remove_all_agents()` method to the `Model` class in `mesa/model.py`.
- The method iterates through all agents using `self._agents.keys()` wrapped in a list to avoid runtime errors due to dictionary size changes.
- Each agent is removed using the `agent.remove()` method to ensure proper cleanup, including removal from experimental cell spaces.
- Added a unit test in `tests/test_model.py` to verify the functionality of the new method.

## Usage Examples
```python
model = MyModel()
# ... add agents and run simulation ...
model.remove_all_agents()  # Removes all agents from the model
assert len(model.agents) == 0
```

## Additional Notes
This method ensures that agents are properly removed using their `remove()` method, maintaining consistency with other agent removal operations in Mesa.
</details>
Of course you have to review and where necessary edit these, and never blindly copy, but I find they are often 90%+ of the case good, and often I only edit a few words or a add a sentence somewhere. And writing them yourself also has some charm, so sometimes I also do that.

Good PR descriptions are helpful for reviewers, but also historical documentation and especially useful in a public-facing library, where users might want to know why a feature is designed like the way it is.

> I will not bother with the existing spaces because they are a massive memory leak already and outside scope.

Okay, in this specific case I can get behind this, but maybe add a line of documentation that Agents are removed from the cell space automatically, but not from the current spaces.

> The best solution for those is to subclass Agent.remove explicitly in your own custom agent class.

I was also thinking in that direction. Would a simple test case that shows this be possible?
quaquel: >  A bit of robustness would be nice here. Could you add some Agents to a custom AgentSet and Grid, and see if they are properly removed?

I don't understand what you are proposing. This adds a method to the model class so why test other non-Model class related things? That is more of a form of integration testing. Moreover, mesa.space.Grid and its subclasses won't work because its not covered in Agent.remove (and cannot be covered there because agents don't know about mesa.space classes. 

> Okay, in this specific case I can get behind this, but maybe add a line of documentation that Agents are removed from the cell space automatically, but not from the current spaces.

> I was also thinking in that direction. Would a simple test case that shows this be possible?

No. What would make more sense is improving the documentation of `Agent.remove`. If we expand this documentation you introduce potential unwanted dependencies in the docs between the agent class and the model class. Therefore adding tests in test_model for testing `agent.remove` also appears strange to me. 
EwoutH: From my view the difference is when removing an Agent in a loop or one by one, you often have code around it to handle other removals, like the space. With `remove_all_agents`, users might think, "oh, that's convenient, a proper build in function!" and then that's not anymore the case.

In my opinion there should be something somewhere for people to find to resolve their error when inevitably they think remove all agents from a model that has a Space. It doesn't need to handle it perfectly all the time be default, but a piece of documentation or testing should be there. That's the cost of doing business in a public API. 

Once we have deprecated the spaces formally, this will be a different story. And `remove_all_agents` could also be just a function of a custom user model, since their really isn't that much added value to add it to Mesa if it doesn't handle supported (maintenance or not) edge cases properly.

Of course any maintainers might have different opinions about any of this, and I'm happy to follow a majority.
quaquel: > From my view the difference is when removing an Agent in a loop or one by one, you often have code around it to handle other removals, like the space. With remove_all_agents, users might think, "oh, that's convenient, a proper build in function!" and then that's not anymore the case.

Fair enough. I have added something on this in `Agent.remove` _and_ `Model.remove_all_agents`. 

Patch:
diff --git a/mesa/agent.py b/mesa/agent.py
index 18454164..200dab6f 100644
--- a/mesa/agent.py
+++ b/mesa/agent.py
@@ -67,7 +67,13 @@ class Agent:
         self.model.register_agent(self)
 
     def remove(self) -> None:
-        """Remove and delete the agent from the model."""
+        """Remove and delete the agent from the model.
+
+        Notes:
+            If you need to do additional cleanup when removing an agent by for example removing
+            it from a space, consider extending this method in your own agent class.
+
+        """
         with contextlib.suppress(KeyError):
             self.model.deregister_agent(self)
 
diff --git a/mesa/model.py b/mesa/model.py
index ac59cc1f..f9c0e2e4 100644
--- a/mesa/model.py
+++ b/mesa/model.py
@@ -276,3 +276,16 @@ class Model:
         )
         # Collect data for the first time during initialization.
         self.datacollector.collect(self)
+
+    def remove_all_agents(self):
+        """Remove all agents from the model.
+
+        Notes:
+            This method calls agent.remove for all agents in the model. If you need to remove agents from
+            e.g., a SingleGrid, you can either explicitly implement your own agent.remove method or clean this up
+            near where you are calling this method.
+
+        """
+        # we need to wrap keys in a list to avoid a RunTimeError: dictionary changed size during iteration
+        for agent in list(self._agents.keys()):
+            agent.remove()


Test patch:
diff --git a/tests/test_model.py b/tests/test_model.py
index 7c343e46..ff1fe297 100644
--- a/tests/test_model.py
+++ b/tests/test_model.py
@@ -93,3 +93,18 @@ def test_agents_by_type():
     assert model.agents_by_type[Wolf] == AgentSet([wolf], model)
     assert model.agents_by_type[Sheep] == AgentSet([sheep], model)
     assert len(model.agents_by_type) == 2
+
+
+def test_agent_remove():
+    """Test removing all agents from the model."""
+
+    class TestAgent(Agent):
+        pass
+
+    model = Model()
+    for _ in range(100):
+        TestAgent(model)
+    assert len(model.agents) == 100
+
+    model.remove_all_agents()
+    assert len(model.agents) == 0