Repository: projectmesa/mesa
Pull request #1568 (2022-12-24 14:53:04)

Make the internal grid and empties_built in Grid class private
The internal grid in the Grid class should be made private as was done in #815, the internal mechanic of the class should not be used by the user (if he doesn't really want to do so) and we should be free to change it as we want if needed.

Something should be changed in the tests folder also, but I'd like to finish the PR when it gets some approval :-)

rht: Yeah, I agree with this proposal.

And in general, it is indeed more effective if you sketch a draft PR and wait for agreement, so that your effort is not wasted.
Tortar: will do that more often!
codecov[bot]: # [Codecov](https://codecov.io/gh/projectmesa/mesa/pull/1568?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) Report
Base: **82.13**% // Head: **82.13**% // No change to project coverage :thumbsup:
> Coverage data is based on head [(`5b4d349`)](https://codecov.io/gh/projectmesa/mesa/pull/1568?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) compared to base [(`b36fce7`)](https://codecov.io/gh/projectmesa/mesa/commit/b36fce7845f97c0296dd188d63bb61adba6827c7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa).
> Patch coverage: 83.33% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main    #1568   +/-   ##
=======================================
  Coverage   82.13%   82.13%           
=======================================
  Files          18       18           
  Lines        1388     1388           
  Branches      272      272           
=======================================
  Hits         1140     1140           
  Misses        204      204           
  Partials       44       44           
```


| [Impacted Files](https://codecov.io/gh/projectmesa/mesa/pull/1568?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) | Coverage Δ | |
|---|---|---|
| [mesa/space.py](https://codecov.io/gh/projectmesa/mesa/pull/1568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa#diff-bWVzYS9zcGFjZS5weQ==) | `91.39% <83.33%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/projectmesa/mesa/pull/1568?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa).

Tortar: Everything should be done now, I realized that also `empties_built` shouldn't be public so I put all together in this PR, hope this is welcomed!

Patch:
diff --git a/mesa/space.py b/mesa/space.py
index 279e876a..aea5ff62 100644
--- a/mesa/space.py
+++ b/mesa/space.py
@@ -89,7 +89,6 @@ class Grid:
     Properties:
         width, height: The grid's width and height.
         torus: Boolean which determines whether to treat the grid as a torus.
-        grid: Internal list-of-lists which holds the grid cells themselves.
     """
 
     def __init__(self, width: int, height: int, torus: bool) -> None:
@@ -104,15 +103,16 @@ class Grid:
         self.torus = torus
         self.num_cells = height * width
 
-        self.grid: list[list[GridContent]]
-        self.grid = [
+        # Internal list-of-lists which holds the grid cells themselves
+        self._grid: list[list[GridContent]]
+        self._grid = [
             [self.default_val() for _ in range(self.height)] for _ in range(self.width)
         ]
 
         # Flag to check if the empties set has been created. Better than initializing
         # _empties as set() because in this case it would become impossible to discern
         # if the set hasn't still being built or if it has become empty after creation.
-        self.empties_built = False
+        self._empties_built = False
 
         # Neighborhood Cache
         self._neighborhood_cache: dict[Any, list[Coordinate]] = dict()
@@ -124,7 +124,7 @@ class Grid:
 
     @property
     def empties(self) -> set:
-        if not self.empties_built:
+        if not self._empties_built:
             self.build_empties()
         return self._empties
 
@@ -135,7 +135,7 @@ class Grid:
                 itertools.product(range(self.width), range(self.height)),
             )
         )
-        self.empties_built = True
+        self._empties_built = True
 
     @overload
     def __getitem__(self, index: int) -> list[GridContent]:
@@ -159,11 +159,11 @@ class Grid:
 
         if isinstance(index, int):
             # grid[x]
-            return self.grid[index]
+            return self._grid[index]
         elif isinstance(index[0], tuple):
             # grid[(x1, y1), (x2, y2), ...]
             index = cast(Sequence[Coordinate], index)
-            return [self.grid[x][y] for x, y in map(self.torus_adj, index)]
+            return [self._grid[x][y] for x, y in map(self.torus_adj, index)]
 
         x, y = index
         x_int, y_int = is_integer(x), is_integer(y)
@@ -172,32 +172,32 @@ class Grid:
             # grid[x, y]
             index = cast(Coordinate, index)
             x, y = self.torus_adj(index)
-            return self.grid[x][y]
+            return self._grid[x][y]
         elif x_int:
             # grid[x, :]
             x, _ = self.torus_adj((x, 0))
             y = cast(slice, y)
-            return self.grid[x][y]
+            return self._grid[x][y]
         elif y_int:
             # grid[:, y]
             _, y = self.torus_adj((0, y))
             x = cast(slice, x)
-            return [rows[y] for rows in self.grid[x]]
+            return [rows[y] for rows in self._grid[x]]
         else:
             # grid[:, :]
             x, y = (cast(slice, x), cast(slice, y))
-            return [cell for rows in self.grid[x] for cell in rows[y]]
+            return [cell for rows in self._grid[x] for cell in rows[y]]
 
     def __iter__(self) -> Iterator[GridContent]:
         """Create an iterator that chains the rows of the grid together
         as if it is one list:"""
-        return itertools.chain(*self.grid)
+        return itertools.chain(*self._grid)
 
     def coord_iter(self) -> Iterator[tuple[GridContent, int, int]]:
         """An iterator that returns coordinates as well as cell contents."""
         for row in range(self.width):
             for col in range(self.height):
-                yield self.grid[row][col], row, col  # agent, x, y
+                yield self._grid[row][col], row, col  # agent, x, y
 
     def neighbor_iter(self, pos: Coordinate, moore: bool = True) -> Iterator[Agent]:
         """Iterate over position neighbors.
@@ -407,7 +407,7 @@ class Grid:
             An iterator of the contents of the cells identified in cell_list
         """
         # iter_cell_list_contents returns only non-empty contents.
-        return (self.grid[x][y] for x, y in cell_list if self.grid[x][y])
+        return (self._grid[x][y] for x, y in cell_list if self._grid[x][y])
 
     @accept_tuple_argument
     def get_cell_list_contents(self, cell_list: Iterable[Coordinate]) -> list[Agent]:
@@ -438,8 +438,8 @@ class Grid:
     def place_agent(self, agent: Agent, pos: Coordinate) -> None:
         """Place the agent at the specified location, and set its pos variable."""
         x, y = pos
-        self.grid[x][y] = agent
-        if self.empties_built:
+        self._grid[x][y] = agent
+        if self._empties_built:
             self._empties.discard(pos)
         agent.pos = pos
 
@@ -448,8 +448,8 @@ class Grid:
         if (pos := agent.pos) is None:
             return
         x, y = pos
-        self.grid[x][y] = self.default_val()
-        if self.empties_built:
+        self._grid[x][y] = self.default_val()
+        if self._empties_built:
             self._empties.add(pos)
         agent.pos = None
 
@@ -476,7 +476,7 @@ class Grid:
     def is_cell_empty(self, pos: Coordinate) -> bool:
         """Returns a bool of the contents of a cell."""
         x, y = pos
-        return self.grid[x][y] == self.default_val()
+        return self._grid[x][y] == self.default_val()
 
     def move_to_empty(
         self, agent: Agent, cutoff: float = 0.998, num_agents: int | None = None
@@ -594,11 +594,8 @@ class MultiGrid(Grid):
 
     Properties:
         width, height: The grid's width and height.
-
         torus: Boolean which determines whether to treat the grid as a torus.
 
-        grid: Internal list-of-lists which holds the grid cells themselves.
-
     Methods:
         get_neighbors: Returns the objects surrounding a given cell.
     """
@@ -613,18 +610,18 @@ class MultiGrid(Grid):
     def place_agent(self, agent: Agent, pos: Coordinate) -> None:
         """Place the agent at the specified location, and set its pos variable."""
         x, y = pos
-        if agent.pos is None or agent not in self.grid[x][y]:
-            self.grid[x][y].append(agent)
+        if agent.pos is None or agent not in self._grid[x][y]:
+            self._grid[x][y].append(agent)
             agent.pos = pos
-            if self.empties_built:
+            if self._empties_built:
                 self._empties.discard(pos)
 
     def remove_agent(self, agent: Agent) -> None:
         """Remove the agent from the given location and set its pos attribute to None."""
         pos = agent.pos
         x, y = pos
-        self.grid[x][y].remove(agent)
-        if self.empties_built and self.is_cell_empty(pos):
+        self._grid[x][y].remove(agent)
+        if self._empties_built and self.is_cell_empty(pos):
             self._empties.add(pos)
         agent.pos = None
 
@@ -1048,7 +1045,7 @@ class NetworkGrid:
             )
             if not include_center:
                 del neighbors_with_distance[node_id]
-            neighbors = list(neighbors_with_distance.keys())
+            neighbors = sorted(neighbors_with_distance.keys())
         return neighbors
 
     def move_agent(self, agent: Agent, node_id: int) -> None:


Test patch:
diff --git a/tests/test_grid.py b/tests/test_grid.py
index 007aa6c4..d5734a85 100644
--- a/tests/test_grid.py
+++ b/tests/test_grid.py
@@ -168,7 +168,7 @@ class TestBaseGrid(unittest.TestCase):
         x, y = agent.pos
         self.grid.remove_agent(agent)
         assert agent.pos is None
-        assert self.grid.grid[x][y] is None
+        assert self.grid[x][y] is None
 
     def test_swap_pos(self):
 
@@ -510,7 +510,7 @@ class TestIndexing:
     # Create a grid where the content of each coordinate is a tuple of its coordinates
     grid = Grid(3, 5, True)
     for _, x, y in grid.coord_iter():
-        grid.grid[x][y] = (x, y)
+        grid._grid[x][y] = (x, y)
 
     def test_int(self):
         assert self.grid[0][0] == (0, 0)
diff --git a/tests/test_space.py b/tests/test_space.py
index 37a6f691..ba04e412 100644
--- a/tests/test_space.py
+++ b/tests/test_space.py
@@ -302,10 +302,10 @@ class TestSingleGrid(unittest.TestCase):
         for i, pos in enumerate(TEST_AGENTS_GRID):
             a = self.agents[i]
             assert a.pos == pos
-            assert self.space.grid[pos[0]][pos[1]] == a
+            assert self.space[pos[0]][pos[1]] == a
             self.space.remove_agent(a)
             assert a.pos is None
-            assert self.space.grid[pos[0]][pos[1]] is None
+            assert self.space[pos[0]][pos[1]] is None
 
     def test_empty_cells(self):
         if self.space.exists_empty_cells():
@@ -325,12 +325,12 @@ class TestSingleGrid(unittest.TestCase):
         _agent = self.agents[agent_number]
 
         assert _agent.pos == initial_pos
-        assert self.space.grid[initial_pos[0]][initial_pos[1]] == _agent
-        assert self.space.grid[final_pos[0]][final_pos[1]] is None
+        assert self.space[initial_pos[0]][initial_pos[1]] == _agent
+        assert self.space[final_pos[0]][final_pos[1]] is None
         self.space.move_agent(_agent, final_pos)
         assert _agent.pos == final_pos
-        assert self.space.grid[initial_pos[0]][initial_pos[1]] is None
-        assert self.space.grid[final_pos[0]][final_pos[1]] == _agent
+        assert self.space[initial_pos[0]][initial_pos[1]] is None
+        assert self.space[final_pos[0]][final_pos[1]] == _agent
 
 
 class TestSingleNetworkGrid(unittest.TestCase):