Move experimental cell spaces to normal

[quaquel]

this is WIP. See elements conversation.

TODO

• move experimental.cell_space to mesa.spaces
• fix imports and test
• move benchmarks to use new spaces
• move examples to use new spaces
• add space specific benchmarks to assess scaling of various methods
• Make propery grids work work with gridspaces.
• resolve Agent and CellAgent (see Generalize CellAgent #2292 for a dedicated discussion on this)
• Update old continuous space in light of CellAgent.move_to resolution
• Deprecate/remove all old spaces

identified issues

1. Cell.neighborhood is currently a method. This is a bad name for a method so either the name should be changed or it should become a property. In Make cell connections public and named #2296, it was suggested to do both. Cell.neighborhood would give you the direct neighborhood (so size of 1), while Cell.get_neighborhood can then be used for larger radii. See Have a dedicated neighborhood property and a get_neighborhood method on Cell #2309.
2. The experimental spaces have a separate class for Moore and von Neumann grids. Do we want to keep this, and more broadly can we built some convenience API on top of this. See @EwoutH comment and the resulting conversation
3. grids take an optional random kwarg, this typically should be set to self.random when instanciating a discrete space within a model to properly pass the rng as used by the model.
4. we use a position tuple rather than seperate x,y coordinates and don't have convenience width and height properties on Grid.
5. Backport API updates of AgentSet to CellCollection were relevant. See Update to CellCollection.select #2307.

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🔴 +78.8% [+77.5%, +79.9%]	🟢 -41.9% [-42.2%, -41.7%]
BoltzmannWealth	large	🔴 +93.0% [+91.7%, +94.4%]	🟢 -29.7% [-32.9%, -25.4%]
Schelling	small	🔵 +0.4% [+0.2%, +0.6%]	🔵 +0.7% [+0.5%, +1.0%]
Schelling	large	🔵 +0.1% [-0.3%, +0.7%]	🔵 +0.1% [-0.8%, +1.1%]
WolfSheep	small	🔵 +0.2% [-0.2%, +0.7%]	🔵 +1.3% [+1.1%, +1.6%]
WolfSheep	large	🔵 -0.5% [-1.0%, -0.1%]	🔵 +1.0% [-0.3%, +2.4%]
BoidFlockers	small	🔵 -2.8% [-3.2%, -2.3%]	🔵 +0.6% [-0.1%, +1.3%]
BoidFlockers	large	🔵 -3.1% [-3.8%, -2.4%]	🔵 +1.3% [+0.5%, +2.2%]

[quaquel]

Note that wolf-sheep and Schelling already used the new experimental grid spaces. The pattern shown for Boltzman is in line with what we saw while developing grid spaces. There is a small absolute increase in init times but a 20%-50% reduction in run times.

[EwoutH]

Don't have a solution just yet, bit on first glance I don't directly like OrthogonalMooreGrid and OrthogonalVonNeumannGrid.

Maybe OrthogonalGrid with an required (so no default) diagonal_connections boolean keyword argument might work. That would also make it easier to experiment with it: Instead of needing to import another space, you switch a keyword argument.

[quaquel]

We debated this endlessly with the original PR. I don't want to redo that debate here. Let's first try and move all this code from experimental and take it from there in future PRs

[EwoutH]

The problem is that once it's stable we can't as easily do things like this.

One part of stabilizing an API is re-evaluating properties of it. I know that will add some work, but if we're not doing that, then why did we make it experimental at all?

[Corvince]

Still you should start by revisiting the original PR.

Regarding experimentation, I don't see a lot of problem changing spaces.OrthogonalMooreGrid to spaces.OrthogonalVonNeumannGrid If you import the spaces, no need to change imports.

[quaquel]

To be clear, I am happy to debate the API, but there is enough stuff going on in this PR that follows from resolving all the implications that follow from moving this from experimental to stable. I feel that adding the actual API to that long list adds further confusion.

Moreover, we had good reasons in the original PR for the split and like @Corvince, I don't see a major practical difference between changing imports or a boolean.

[quaquel]

1. Moore is used as a label in the current version of the space but with a boolean. So we don't add new concepts to MESA.
2. Knowing Moore vs von Neumann neighborhoods in my view is basic ABM knowledge that a user should just have. It sits in the same category of knowing what a torus is.
3. Explaining the difference between Moore and von Neumann is presently already covered in the docstring. We might expand this a bit more so a user does not need to google it.

[EwoutH]

Moore is used as a label in the current version of the space but with a boolean.

So why then at the least not keep it this way? Would make migrating also easier.

[EwoutH]

You normally subclass when you have either:

• Different (conflicting) attributes
• Different methods, with some absent in other

With diagonal/notdiagonal this is not the case, as far as I understand. They have the same methods, return the same outputs (a collection of cells) and have the same attributes (neighbours, etc.). The only thing that's different is a single function implementation (afaik), which can easily be covered by an if-else.

So why do I like keyword arguments (in this case)? Because they are more scalable. If you add another two booleans (on how many agents are allowed, another neighbour configuration, etc.), you can have 8 configuration options. With subclasses, you need to have 8 subclasses.

Here's another idea: Why not have a keyword argument neighbour_definition. Can be "Moore", "VonNeuman", but also something else, like a custom map. Could help a lot when we want to implement:

• Proposal: Formal neighborhood definition for grids #1900

It just feels like subclassing limits us significantly for future extension, and isn't the best practice in this case.

---

I'm not going to die on this hill (in this PR). Move forward with it without this if you want, I can follow up.

[quaquel]

I am happy to explore this further, but indeed, preferably separately.

When to subclass is a deeply contested topic, judging by the various StackOverflow questions about this that have been closed as being too opinionated. We indeed use it here for different implementations of how cells are wired up inside the grid.

But I would not say that the fact that the only difference is the wiring up method implies we should (note not could, because that is indeed possible) reduce this all to a list of options for some neighbour_definition keyword argument and have a long case match statement to cover all possible neighborhood definitions. From a classic OO point of view, a Moore grid, a von Neuman grid, a network, a hexgrid, or a voroinoi grid, are all discrete grids. Subclassing them while abiding by a common interface is, in fact, good design. The motivation for subclassing in this reasoning is that their behavior is different.

[Corvince]

Just a note on the API (again, outside the scope of this PR): The use of subclassed to explicitly donate which space is used does not prohibit a possibly more user-friendly API. We could also think about a create_space() function with completely different arguments that return our spaces, without users needing to instantiate them themselves.

[EwoutH]

A key question is how to merge Agent and CellAgent.

While this feels the wrong way around, it seems that an Agent is an CellAgent that can move around. So maybe Agent should inherit from CellAgent?

[quaquel]

While this feels the wrong way around, it seems that an Agent is an CellAgent that can move around. So maybe Agent should inherit from CellAgent?

1. I like having some kind of move method in the Agent. It is part of the behavior of an agent that it can move.
2. The issue is that the implementation of some kind of Agent.move_to is dependent on the underlying space within which an agent is moving.
3. Still open, there are Agent spatial methods from GaelLucero #2149 and ABM language #1354, both are about move methods in continuous spaces.

So, ideally, we come up with a single solution that resolves all of this. I agree with #2149 that having a move_to method that is just always there in a basic agent class might be preferable over having Agent, CellAgent (for discrete spaces), and ContinuousAgent (for continuous spaces).

[EwoutH]

2. The issue is that the implementation of some kind of Agent.move_to is dependent on the underlying space within which an agent is moving.

Maybe it helps if the Agent has direct acces to the grid it's on, like with Agent.grid. Or require Agent.model.grid to be defined when using move(). Or having move take a grid as input (which can have an convention like model.grid as default).

[quaquel]

2. The issue is that the implementation of some kind of Agent.move_to is dependent on the underlying space within which an agent is moving.

Maybe it helps if the Agent has direct acces to the grid it's on, like with Agent.grid. Or require Agent.model.grid to be defined when using move(). Or having move take a grid as input (which can have an convention like model.grid as default).

Yes something like that seems a good way forward. Just quickly testing some API idea, you might get something like the following:

class Agent:

    def __init__(self, model, space: SpaceBaseClass = None):
        ...
        if isinstance(space, ContinouosSpace)
            # "activate" continuous space move methods
        elif isinstance(space, Discrete space) 
            # "activate" discrete space move methods

You could even go one step further and in __new__ instantiate either a CellAgent or ContinousAgent as the base class (see second answer on this StackOverflow). Their only difference would be the available move methods. This would mean that by default, a user would not need to worry about which agent to subclass and she can just subclass Agent.

Note that I prefer space over grid, because space is the more generic term here.

[EwoutH]

I thought about this, and I'm against stabilizing for now. The main reason is that we just haven't had the chance to iterate on this properly, and that makes it just too early to stabilize.

There's also only a very small penalty in having this in the experimental space. It's just a convention, and a reminder for users that this can change. Furthermore, we can also stabilize with 3.1 or 3.2.

A few examples:

• We're just now figuring out how to how the relation between a cell agent and a regular agent is
• We're still discussing the 4-, 8- and custom neighbour relations
  • See also Proposal: Formal neighborhood definition for grids #1900
  • On another thoughts: Isn't the Vonoroi Tesllation space not just a very special case of cells having neighbours? Can we integrate that in a more general definition?
    Add Voronoi tessellation space #1895
• PropertyLayers aren't connected yet

I really like there's renewed focus on the spaces. But let's do all those things in the experimental space, and once we're content, and have a solid strategy about replacing the current spaces, we can move.

[quaquel]

I thought about this, and I'm against stabilizing for now. The main reason is that we just haven't had the chance to iterate on this properly, and that makes it just too early to stabilize.

I am somewhat surprised by this. Would any of the listed discussions have started if not for this PR? This PR intended to kickstart that kind of discussion and renew our focus on spaces. Moreover, as long as users don't get at least a deprecation warning on any of the existing discrete spaces, how many will start looking, using, and providing feedback on the new spaces?

Put different: I am not saying this PR should be merged ASAP. Rather it is a WIP to stimulate iterating and refining the spaces. But I would personally be in favor of merging this as part of 3.0, or at a minimum deprecate the old stuff.

[EwoutH]

Would any of the listed discussions have started if not for this PR?

And that's a good thing, but also indicates stuff is moving a lot, still. Wouldn't have moving over the examples (and benchmarks) over have had the same effect? (and I'm a big proponent of moving over examples and benchmarks!)

We don't have to decide this now. If it looks and feels stable before 3.0 feature cutoff we can stabilize it.

My initial thought it we can't deprecate something before we have a stable replacement. But let me think about it.

(also this is my current view and of course that can change)

Edit: I suggest this mainly to make sure we can keep the spaces moving quickly, not to slow it down. In the experimental directory we can move way quicker. And once we stabilize, which we can do in a minor release, we can just let .experimental.spaces be an alias for .spaces.

It's just flipping a switch.