Encapsulate cell movement in properties #2333

quaquel · 2024-09-28T19:06:02Z

This PR encapsulates agent movement between cells using a HasCell mixin with property descriptors. It replaces the explicit move_to method with cell attribute assignment.

Motive

The goal is to simplify the API for agent movement in cell-based spaces and lay the groundwork for more flexible agent behaviors. This approach allows movement to be handled through simple attribute assignment rather than explicit method calls.

Implementation

Added a HasCell mixin class that implements cell assignment logic using property descriptors
Updated CellAgent to use the HasCell mixin
Removed the move_to method from CellAgent
Updated CellAgent.remove to handle cell removal when an agent is removed
Modified example models and tests to use the new cell assignment approach

Usage Examples

Previous approach:

agent.move_to(new_cell)

New approach:

agent.cell = new_cell

In the Schelling model:

# Old
self.move_to(self.model.grid.select_random_empty_cell())

# New  
self.cell = self.model.grid.select_random_empty_cell()

Python Constructs and Patterns Used

Property Descriptors: The HasCell mixin uses the @property decorator and a setter to create a managed attribute for the cell. This allows for custom logic to be executed when the cell is accessed or modified.
Mixins: The HasCell class is designed as a mixin, allowing its functionality to be easily combined with other classes through multiple inheritance.

Additional Notes

This PR lays the foundation for more advanced movement behaviors to be implemented in Generalize CellAgent #2292
There is a small performance impact on some models, particularly Wolf-Sheep, which may need further optimization
The move_to method may be reintroduced with enhanced functionality in a future PR

Original description

This PR encapsulates the addition to and removal from a cell of an agent in a HasCell mixin. It updates CellAgent to use this mixin, and adds an override of agent.remove to ensure agents remove themselves from the cell they are occupying when they are removed from the model. The main implication of this PR is that CellAgent.move_to is now redundant. I have thus removed it. In #2292, we might bring it back but with more powerful behavior.

outdated based on discussion below

This PR is for discussion in relation to #2292. It shows how it might be possible to use composition for cell-related movement by encapsulating all the movement behavior into a descriptor. So instead of having a special Agent.move_to method, you can do Agent.cell = new_cell. You can also use whatever name you want instead of cell.

An additional benefit that I had not realized at first is that it cleans up the API a bit more as well. A user does not have to interact with the Cell class directly. She only needs to assign the Cell instance to whatever attribute is used for the descriptor.

for more information, see https://pre-commit.ci

EwoutH · 2024-09-29T09:58:48Z

I think I like this, it makes it much more explicit what's happening. It's like modifying pos directly, but for the new spaces.

One thing I'm curious about if cell has now a special status. How does the space know what Agent variable to lookup / modify? What is there are multiple spaces?

I will review in more detail tomorrow!

Corvince · 2024-09-29T10:18:35Z

Ha! Given enough time I think @quaquel will turn every mesa behaviour into a descriptor 😂

No, I really like this and this somehow put me over the edge to finally try to fully grok descriptors.

Let me write in more detail when I have more time, but I think using just a property with a setter might be better

quaquel · 2024-09-29T14:45:10Z

One thing I'm curious about if cell has now a special status. How does the space know what Agent variable to lookup / modify? What is there are multiple spaces?

A space does not need to know anything about Agents. A space has cells. A cell can have agents. Agents and cells need to be aware of each other. A space is primarily a container for cells, nothing more. Multiple spaces should not be a problem. You just assign another cell to the attribute, and you're done.

No, I really like this and this somehow put me over the edge to finally try to fully grok descriptors.

The shortest summary is "reusable properties".

Let me write in more detail when I have more time, but I think using just a property with a setter might be better

I think you need a getter because the agent needs to know its cell for e.g., a random walk, but curious to see what more you have to say in detail.

Given the fact that both @Corvince and @EwoutH like this idea, I'll try to update the tests asap to make sure this is fully covered again.

EwoutH · 2024-09-29T15:00:40Z

(I just looked at it from my phone, still need to do a thorough review)

for more information, see https://pre-commit.ci

Corvince · 2024-09-29T18:40:10Z

I think you need a getter because the agent needs to know its cell for e.g., a random walk, but curious to see what more you have to say in detail.

My main objection would be that it might make some things easier if we have a clear "cell" definition. That is with a reusable descriptor we have the advantage of allowing several "cell" descriptor from several spaces. In practice I think this isn't very common.

In the case of a "normal" property we could have the following:

class HasCell:
    _cell: Cell | None = None

    @property
    def cell(self):
        return self._cell

    @cell.setter
    def cell(self, cell: Cell):
        ... # Your logic from CellDescriptor.__set__

And then we can have separate movement mixin

class CellMovement:
    def move_to(self, cell: Cell):
         self.cell = cell

    ... # More movement functions

And finally the CellAgent:

class CellAgent(Agent, HasCell, CellMovement):
    ....

In this case its unambigious and easy. We know we have to assign to "cell". If the cell descriptor can be assigned to any name, how would we know the right name?

Further advantages are that one could theoretically overwrite the cell.setter function. Also I think more people know properties in comparison to descriptors (although the former is just a higher level abstraction).

quaquel · 2024-09-29T19:00:12Z

In this case its unambigious and easy. We know we have to assign to "cell". If the cell descriptor can be assigned to any name, how would we know the right name?

Not sure I fully understand your point, but a descriptor has the __set_name__ magic method. This magic method is called during class creation with the owning class and name of the attribute to which the descriptor is assigned. So, with a descriptor, you can always know the name of the attribute.

Also, the __set__ method in the descriptor would make a move_to method completely redundant.

I like your name HasCell as a possible name for the descriptor. It is probably better than my current uninspiring CellDescriptor

Corvince · 2024-09-29T19:08:00Z

Sorry if I wasn't clear. The movement mixin was just an example (although I would like to have the clearer move_to API, even if redundant. But this is irrelevant for my point here). The point is that for any mixin or rather anything wanting to operate on the cell property it needs to know the name. Of course the user knows the name, but our framework doesn't. So in the example move_to method the line self.cell = cell would not work if the name is not cell, but something else.

quaquel · 2024-09-29T19:11:15Z

Sorry if I wasn't clear. The movement mixin was just an example (although I would like to have the clearer move_to API, even if redundant. But this is irrelevant for my point here). The point is that for any mixin or rather anything wanting to operate on the cell property it needs to know the name. Of course the user knows the name, but our framework doesn't. So in the example move_to method the line self.cell = cell would not work if the name is not cell, but something else.

Ok, that makes sense. If we want to add any additional functionality like the MoveTo example, then yes we need to lock down the name of the attribute to which the current cell is assigned.

quaquel · 2024-09-30T06:03:47Z

A quick follow-up to @Corvince point about the naming of the attribute. There is, in fact, already a case like this. It is up to the user to actively remove an agent from the cell before calling Agent.remove. Moving this behavior into CellAgent.remove would make sense, but this method needs to know the attribute to which Cell is assigned. So the descriptor idea, which leaves the name of the attribute to the user, is less than ideal.

This leaves the other idea: instead of having a move_to method solve everything via attribute assignment: self.cell = self.cell.neighborhood.select_random_cell(), which can be done with properties as shown by the HasCell mixin shown by @Corvince . I do like the clean API that results from solving cell movement this way and slightly prefer it over the current move_to method.

Of course this still leaves room for more sophisticated movement methods that for example use a heading or something.

EwoutH · 2024-09-30T06:47:40Z

One thing I would find interesting is how this would work with multiple grids. For example, I currently have a model in which Agents move both in fine grained areas (postal code “cells”) and which are part of larger scale municipalities. So here you have like a nested spaces with two resolutions.

Another case would be to completely orthogonal spaces, for example a space representing some physical representation and another representing social, economic or other relations (maybe a cell space isn’t suitable for that).

CC @wang-boyu (I’m especially curious how you see this (and the whole cell space) integrating with Mesa-Geo in the long term)

quaquel · 2024-09-30T06:54:46Z

One thing I would find interesting is how this would work with multiple grids.

I think this question is only tangentially related to the point of this PR/discussion.
Do we want to support that out of the box with MESA? I personally don't think so.

EwoutH · 2024-09-30T07:01:38Z

Just making sure we don’t box ourselves in / prohibit a use case unintentionally.

Corvince · 2024-09-30T13:46:48Z

Yes, I see this as a perfect base case for the ideas discussed in #2292. After this is merged I would update #2292 to incorporate the named coordinates and create a "Movement" mixin class. Then CellAgent can become just a class inherting from Agent, HasCell and Movement without any MRO related headaches (because of those, only Agent has an init method). I think this will come out nicely, but once implemented we can see how it plays out by adapting some examples.

for more information, see https://pre-commit.ci

github-actions · 2024-09-30T15:11:34Z

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🔵 +0.5% [-0.4%, +1.3%]	🔵 -0.2% [-0.2%, -0.1%]
BoltzmannWealth	large	🔵 +0.4% [-0.2%, +1.1%]	🔵 +2.1% [+1.3%, +3.0%]
Schelling	small	🔵 +1.9% [+1.5%, +2.3%]	🔵 +3.5% [+2.7%, +4.3%]
Schelling	large	🔵 +2.3% [+1.2%, +3.5%]	🔵 +7.0% [-0.2%, +14.2%]
WolfSheep	small	🔵 +2.5% [+1.8%, +3.2%]	🔴 +13.4% [+9.5%, +17.5%]
WolfSheep	large	🔵 -1.1% [-2.9%, +0.6%]	🔴 +11.1% [+6.8%, +15.4%]
BoidFlockers	small	🔵 +1.4% [+1.0%, +1.7%]	🔵 +0.1% [-0.5%, +0.7%]
BoidFlockers	large	🔵 +1.6% [+0.9%, +2.2%]	🔵 +0.3% [-0.1%, +0.7%]

github-actions · 2024-09-30T15:16:35Z

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🔵 -0.2% [-1.0%, +0.5%]	🔵 +0.3% [+0.2%, +0.4%]
BoltzmannWealth	large	🔵 -0.6% [-1.2%, -0.1%]	🔵 +0.1% [-0.4%, +0.7%]
Schelling	small	🔵 +1.4% [+0.9%, +1.8%]	🔵 +1.7% [+1.4%, +2.0%]
Schelling	large	🔵 +0.9% [+0.4%, +1.4%]	🔵 +1.5% [-0.0%, +2.9%]
WolfSheep	small	🔵 +1.8% [+1.4%, +2.1%]	🔴 +12.1% [+7.8%, +16.6%]
WolfSheep	large	🔵 +0.8% [-0.4%, +2.3%]	🔴 +17.8% [+15.2%, +20.4%]
BoidFlockers	small	🔵 -2.0% [-2.3%, -1.7%]	🔵 -0.7% [-1.5%, +0.1%]
BoidFlockers	large	🔵 -1.6% [-2.1%, -1.1%]	🔵 -0.6% [-1.2%, -0.1%]

Corvince · 2024-09-30T15:33:47Z

There are 6 ruff errors otherwise this looks good to go from my side (but a lot of things are based on my ideas so I would like another reviewer to approve)

rht · 2024-09-30T16:48:56Z

Disclaimer: I haven't read #2292. I'm not fully sure why having a move_to method makes it not compose-able, but from UX perspective, I prefer for it to be not removed. It's a lot easier to search and understand the codebase if there is an actual move_to verb, and having to grep for self.cell = self.cell.* for an agent movement (this is a relatively more complex expression regexp vs just a keyword move_to), making it as a too Mesa-specific quirk and not as modeling in general.

While @quaquel 's stance on accommodating LLM can be found at #2237 (comment), I find that making it less likely for LLM to fall into a gotcha / to help it to reason better actually doubles as to help human reasoners as well.

Why can't we have both move_to and while having the descriptor under the hood?

quaquel · 2024-09-30T17:04:13Z

Why can't we have both move_to and while having the descriptor under the hood?

The idea is that move_to will be made more powerful via #2292. So yes we'll likely end up with both. The key thing here is that the logic of adding and removing agents from cells is encapsulated in the attribute assignment rather than via a separate method.

rht · 2024-09-30T18:55:23Z

benchmarks/Schelling/schelling.py

@@ -31,7 +42,7 @@ def step(self):

        # If unhappy, move:
        if similar < self.homophily:
-            self.move_to(self.model.grid.select_random_empty_cell())
+            self.cell = self.model.grid.select_random_empty_cell()


If we have both, then why is this not self.move_to?

please read the discussion both here and in #2292.

This doesn't sound inclusive to me. This was a genuine question, yet I was told to comb through the wall of text to get my answer. The 1 approval from a third position to merge is wearing me down, when a feedback from someone else not biased with the design choice was asked.

I am sorry for the short answer and the merge, but move_to is added back in via #2292.

I see, then you have my post-merge approval.

EwoutH

Thanks for working on this. This looks really robust, and it looks like it's more robust than previous movement methods.

I left a few thoughts when going through the diff.

EwoutH · 2024-09-30T20:07:32Z

benchmarks/WolfSheep/wolf_sheep.py

    def step(self):
        """One step of the agent."""
-        self.random_move()
+        self.cell = self.cell.neighborhood.select_random_cell()


Nice to have this both an one-liner and in a single location!

Yes I like the shortness of it now. Where in mesa 2.1 or so, you needed a dedicated random walker class (see here to now having it as a oneliner.

EwoutH · 2024-09-30T20:08:48Z

benchmarks/WolfSheep/wolf_sheep.py

        super().__init__(model)
-        self._fully_grown = fully_grown
+        self._fully_grown = True if countdown == 0 else False  # Noqa: SIM210


An comment explaining this might be useful.

Oh I might better have left that change out. Basically, there was a fixme statement in the docstring to go from 2 arguments (countdown value and whether or not being fully grown) to only 1. If countdown is 0, you are fully grown, otherwise you are not.

mesa/experimental/cell_space/cell_agent.py

EwoutH · 2024-09-30T20:13:02Z

tests/test_cell_space.py

+    assert agent not in cell2.agents
+
+    agent.remove()
+    assert agent not in model._all_agents


Completely unrelated, doesn't agent not in model.agents work? If so, we might want to implementent some construct so that it does.

In this case I explicitly am testing for the hard reference because if that is gone, I know that the object can be garbage collected.

The syntax in general is indeed valid.

EwoutH · 2024-09-30T20:16:08Z

mesa/experimental/cell_space/cell_agent.py

+            cell.add_agent(self)
+
+
+class CellAgent(Agent, HasCell):


Now that this Agent is moving, this name might get confusing with an Agent that sits in a fixed place, and the whole grid is covered with such (like the patches in NetLogo). For now fine, but maybe something we should keep in mind as the cell space develops.

Let's pick up that point in #2292

quaquel added 2 commits September 28, 2024 21:01

move cell movement into a descriptor

5870dac

Update cell_agent.py

f6e4f08

quaquel added the discuss label Sep 28, 2024

quaquel requested review from Corvince and EwoutH September 28, 2024 19:06

pre-commit-ci bot and others added 2 commits September 28, 2024 19:06

[pre-commit.ci] auto fixes from pre-commit.com hooks

bc81ff9

for more information, see https://pre-commit.ci

benchmark related fixes

680320c

quaquel added the trigger-benchmarks Special label that triggers the benchmarking CI label Sep 28, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

6355e65

for more information, see https://pre-commit.ci

quaquel removed the trigger-benchmarks Special label that triggers the benchmarking CI label Sep 28, 2024

This comment was marked as outdated.

Sign in to view

minor tweak

22a2cce

quaquel added trigger-benchmarks Special label that triggers the benchmarking CI and removed trigger-benchmarks Special label that triggers the benchmarking CI labels Sep 29, 2024

This comment was marked as outdated.

Sign in to view

quaquel and others added 3 commits September 29, 2024 19:12

minor cleaning

6883135

[pre-commit.ci] auto fixes from pre-commit.com hooks

b687d08

for more information, see https://pre-commit.ci

Update schelling.py

4f7e69a

quaquel and others added 6 commits September 30, 2024 16:36

Merge remote-tracking branch 'upstream/main' into cell

a6ce51e

move to mixin

064b934

[pre-commit.ci] auto fixes from pre-commit.com hooks

0d2eccc

for more information, see https://pre-commit.ci

Update cell_agent.py

7f79d27

add tests

5d67c8d

[pre-commit.ci] auto fixes from pre-commit.com hooks

4c8c4ea

for more information, see https://pre-commit.ci

quaquel added trigger-benchmarks Special label that triggers the benchmarking CI feature Release notes label enhancement Release notes label and removed discuss trigger-benchmarks Special label that triggers the benchmarking CI labels Sep 30, 2024

This comment was marked as duplicate.

Sign in to view

ruff related fixes

b921ea5

Corvince approved these changes Sep 30, 2024

View reviewed changes

rht reviewed Sep 30, 2024

View reviewed changes

Corvince mentioned this pull request Sep 30, 2024

Generalize CellAgent #2292

Merged

quaquel merged commit e5ce58a into projectmesa:main Sep 30, 2024
11 of 12 checks passed

quaquel changed the title ~~Move cell movement into a descriptor~~ Encapsulate cell movement in properties Sep 30, 2024

EwoutH reviewed Sep 30, 2024

View reviewed changes

EwoutH added experimental Release notes label and removed feature Release notes label labels Sep 30, 2024

quaquel mentioned this pull request Oct 12, 2024

Moving examples to use the new discrete spaces projectmesa/mesa-examples#198

Merged

quaquel deleted the cell branch November 4, 2024 19:36

EwoutH mentioned this pull request Nov 25, 2024

Tracking issue: Stabilizing the Cell Space #2519

Open

22 tasks

Encapsulate cell movement in properties #2333

Encapsulate cell movement in properties #2333

Conversation

quaquel commented Sep 28, 2024 • edited by EwoutH Loading

Motive

Implementation

Usage Examples

Python Constructs and Patterns Used

Additional Notes

Original description

outdated based on discussion below

This comment was marked as outdated.

This comment was marked as outdated.

EwoutH commented Sep 29, 2024

Corvince commented Sep 29, 2024

quaquel commented Sep 29, 2024 • edited Loading

EwoutH commented Sep 29, 2024

Corvince commented Sep 29, 2024

quaquel commented Sep 29, 2024

Corvince commented Sep 29, 2024

quaquel commented Sep 29, 2024

quaquel commented Sep 30, 2024

EwoutH commented Sep 30, 2024

quaquel commented Sep 30, 2024

EwoutH commented Sep 30, 2024

Corvince commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

This comment was marked as duplicate.

Corvince commented Sep 30, 2024

rht commented Sep 30, 2024

quaquel commented Sep 30, 2024

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

EwoutH 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

quaquel commented Sep 28, 2024 •

edited by EwoutH

Loading

quaquel commented Sep 29, 2024 •

edited

Loading