A cleaner Grid implementation

[Corvince]

Introduction

This PR improves the several Grid classes, both in terms of performance and a cleaner API. Since I came to develop this PR for an improved performance I will start to describe it, but I think by now more value lies in the improved API.
But first of all a big thank you for @rht for providing type hints for the space module. This helped a lot in figuring out the weakpoints of the current implementation.

Performance

I'll start by demonstrating the current state for running the Schelling example. If you create a jupyter notebook inside examples/schelling with the following content you should see a similar result

from model import Schelling

%%prun
model = Schelling(30, 30)

while model.running and model.schedule.steps < 100:
    model.step()

This will result in roughly the following output:

         6220779 function calls in 6.221 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
   647100    1.471    0.000    2.700    0.000 space.py:139(iter_neighborhood)
   534723    0.867    0.000    4.272    0.000 space.py:307(<genexpr>)
  1150400    0.748    0.000    0.748    0.000 space.py:287(out_of_bounds)
     4474    0.594    0.000    0.594    0.000 {built-in method builtins.sorted}
   579674    0.482    0.000    0.585    0.000 space.py:361(is_cell_empty)
    71900    0.421    0.000    5.271    0.000 model.py:24(step)
   575200    0.357    0.000    0.736    0.000 space.py:277(torus_adj)

The result shows that most of the time is spent in iter_neighborhood and also quite some time in out_of_bounds (called twice from within iter_neighborhood, which is a bug) and is_cell_empty and torus_adj (again mostly from within iter_neighborhood). This means that this function is the main entry point for any speed-ups. However, the function itself doesn't do a lot actually. For a cell (1, 1) it just calculates the neighboring cells [(0, 1), (1, 0), (1, 2), (2, 1)]. While it does have to jump through a few hoops to work for different neighborhoods and handle tori, the algorithm is fairly straight forward. This means any performance improvements (of which I tried several) are borderline premature optimization, but since the function is called more than 600,000 times in the example above it does add up. However, I just recently realized how to improve performance for good: caching. In hindsight it is quite obvious that the neighborhoods never change, so we don't have to calculate it every time, but just once and then store it in a dict with calling parameters as keys.

This change reduces the model runtime from the above 6.2 seconds to 2.9 seconds, so more than a 2x gain. And it leads directly to the next change of this PR

get_ vs iter_

Currently all space methods are implemented as Iterators named iter_* with an associated get_* method that just wraps the Iterator into a list. I always thought this was mostly done for performance reasons, since iterators are evaluated lazily and one could "abort" the iteration if a model doesn't need to iterate over all values. However, since we can't cache iterators this performance advantage is broken for iter_neighborhood vs get_neighborhood. Therefore I would propose deprecating all iter_ methods to have a simplified API that only consists of get_ methods. If you really need to iterate you could still always write something like this:

neighbors = grid.get_neighborhood(pos)
for cell in neighbors:
  content = grid[cell]
  if content:  # if you want to filter empty cells
    ...  # whatever you want to do

or even create you own iterator in a single line:

iter_neighbors = (grid[neighbor] for neighbor in grid.get_neighborhood(pos) if grid[neighbor])

Please discuss any advantages you see in having iter_* methods.

the get_cell_list_contents method

Ok this is the function that caused me the most confusion while working on this PR. What I thought this function does: Provide it with a list of cells and you get a list of their contents. Turns out: not quite. For example, considering SingleGrid: The content is either None or Agent. However, get_cell_list_contents(cell_list) will always return a list of only Agents. This is more useful most of the times, but wasn't clear to me from the beginning. More confusingly get_cell_list_contents for MultiGrid will chain together all Agents. So if you provide 2 cells and you get a list of 2 agents back you don't know where they come from (either both from the first, both from the second or one and one). I propose a new method get_contents(cell_list) that always returns a list of the same length as the input cell_list. Additionally provide a new method get_agents(cell_list) that returns all agents within the cell_list (that is, the same function as get_cell_list_contents but hopefully less confusing).

Breaking API change

So far the changes I introduced with this PR include some deprecations, but are backwards compatible. However I also propose one backward incompatible change. Currently for a cell with pos = (x, y) it is possible to get the contents of that cell either by using grid[x][y] or with grid.get_cell_list_contents(pos). I propose to remove the former in favor of grid[pos]. This negates the need for the outer and inner APIs to unpack the pos parameter just to access the contents. Additionally grid[x][y] already looks like an attribute access, but is actually just grid[x] followed by [y]. This ties the API to the implementation (a list of lists), which I don't think is a good style and also prevents alternative grids with the same API.

This is also the only thing I changed in the tests. Everything else passes without any modifications.

Code changes

The following changes only regard the implementation and not the API.

1. Instead of SingleGrid and MultiGrid inheriting from Grid, MultiGrid is now the base class with the default content type of a list. SingleGrid is a subclass of MultiGrid that prevents more than one agent per cell and Grid is a special type of SingleGrid (see Issues with the Grid implementation #808 for my opinion on why Grid should be removed from mesa).
2. I removed the private methods _place_agent and _remove_agent methods. I think it was rather confusing to have some work inside place_agent and some work within place_agent.
3. Introduced a private method _get(x, y) that always returns the internal list representation (since grid[pos] returns an Optional[Agent] for SingleGrid and Grid). This is used extensively inside various methods and allows to unpack within calling this function (with which I mean it is called as self._get(*pos))

Status

I am publishing this PR now as a draft for the following reasons:

1. Some docstrings need a bit more updating
2. Tests and examples have to be updated.

However, I don't want to do these things until this PR gets some approval and make the work worthwhile.

[codecov]

Codecov Report

Merging #815 into master will decrease coverage by 0.46%.
The diff coverage is 86.44%.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
- Coverage   84.71%   84.25%   -0.47%     
==========================================
  Files          17       17              
  Lines        1034     1054      +20     
  Branches      169      171       +2     
==========================================
+ Hits          876      888      +12     
- Misses        127      133       +6     
- Partials       31       33       +2     

Impacted Files	Coverage Δ
mesa/space.py	91.56% <86.44%> (-2.11%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb56278...75ae348. Read the comment docs.

[rht]

Shouldn't this be y, x?

[Corvince]

Yes and no: It should be y, x to not introduce a change, but I think the current [y][x] is a bug: Earlier (line 83) (x, y) is explicitly passed as a pos parameter to citizen

[rht]

In that case, the example needs to be fixed, separate from this PR. This PR will take a long time to be merged.

[Corvince]

Theoretically I would agree, but since this is just an example code and there is no behavioral change to the model itself I would say this change should be okay. If you and others insist on keeping it with (y, x) I don't mind changing it back at all, but honestly I currently don't have the time nor the desire to create a separate PR and rebase this one just to have a "clean" solution.
I agree that this PR will take a long time to be merged (if ever), but I certainly don't think this is the cause

[ReblochonMasque]

Back in the day, I proposed to change x, y with col, row in order to prevent certain confusion in the grid addressing. What made sense 4 or 5 years ago could still be implemented, with great benefit.

[rht]

Pinging @jackiekazil, @tpike3, @wang-boyu for before the 1.0 release. Just wondering if we should switch to self.grid[x][y] = agent instead of self.grid[y][x] = agent in 1.0 ASAP. The convention affects the examples only.

[wang-boyu]

Back in the day, I proposed to change x, y with col, row in order to prevent certain confusion in the grid addressing. What made sense 4 or 5 years ago could still be implemented, with great benefit.

I would prefer this - use variables (x, y) for coordinates, and [row, col] for matrix/grid indexing (so we don't need to change too much code hopefully). We can highlight the relations between these variables in documentations.

If needed, we could perhaps also provide utility functions to do such conversion, e.g., xy_2_rowcol(), or with some better function name.

[rht]

It's a quick fix. I have made #1366.

[rht]

For reference, Agents.jl defines the first index as the first dimension, second index as the second dimension, ... up to N. This is more general, and we shouldn't be bogged down to a 2D-specific representation detail such as col/row. Think Einstein notation for tensor.

[wang-boyu]

Oh sorry I think I misunderstood your question. My previous comment applies only to the upcoming RasterLayer in Mesa-Geo, not Mesa's grid space, since we're not viewing the grid space as matrix here.

It would definitely make sense to use self.grid[x][y] in all examples to make it consistent.

[rht]

This should be y, x too.

[Corvince]

Note: This is only done for the time being, otherwise travis will crash because the warnings are triggered too many times (since the examples are not updated yet)

[Corvince]

I'll leave this open to discuss any of the changes, but I have now reconsidered my approach: It is probably changing way too much at the same time. I will split the changes into more digestible PRs

[jackiekazil]

@Corvince - I need a quick read through of this. I am excited abou these changes, but I agree that smaller PRs would be better and easier to push through. Looking forward to them!

[Tortar]

@Corvince I think that one of the best change in this PR is the removal of the Grid class, which seems just to be a partial SingleGrid with no real benefit, it can just creates bugs because it doesn't handle positioning of agents well, I think I will create a PR on this for Mesa 2.0, is everybody ok with this?