Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make cell connections public and named #2296

Merged
merged 9 commits into from
Sep 18, 2024
Merged

Make cell connections public and named #2296

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6dd3081
make connections public and named
Corvince Sep 15, 2024
82ddc05
add names to grid
Corvince Sep 15, 2024
2a9f3b4
add names to network and voronoi
Corvince Sep 15, 2024
05790e1
update tests to use public connections
Corvince Sep 15, 2024
5bf9471
update dict-key handling
Corvince Sep 15, 2024
64bb4f7
Change connection name to be coordinate-like (tuple)
Corvince Sep 16, 2024
bb4e9df
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 18, 2024
884f0f8
rename connection name to key and add to docstring
Corvince Sep 18, 2024
ed421e8
Merge branches 'cellspace-named-connections' and 'cellspace-named-con…
Corvince Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions mesa/experimental/cell_space/cell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
from mesa.experimental.cell_space.cell_collection import CellCollection

if TYPE_CHECKING:
from mesa.agent import Agent
from mesa.experimental.cell_space.cell_agent import CellAgent

Coordinate = tuple[int, ...]


class Cell:
"""The cell represents a position in a discrete space.
Expand All @@ -26,7 +29,7 @@ class Cell:

__slots__ = [
"coordinate",
"_connections",
"connections",
"agents",
"capacity",
"properties",
Expand All @@ -44,7 +47,7 @@ class Cell:

def __init__(
self,
coordinate: tuple[int, ...],
coordinate: Coordinate,
capacity: float | None = None,
random: Random | None = None,
) -> None:
Expand All @@ -58,20 +61,24 @@ def __init__(
"""
super().__init__()
self.coordinate = coordinate
self._connections: list[Cell] = [] # TODO: change to CellCollection?
self.agents = [] # TODO:: change to AgentSet or weakrefs? (neither is very performant, )
self.connections: dict[Coordinate, Cell] = {}
self.agents: list[
Agent
] = [] # TODO:: change to AgentSet or weakrefs? (neither is very performant, )
self.capacity = capacity
self.properties: dict[str, object] = {}
self.properties: dict[Coordinate, object] = {}
self.random = random

def connect(self, other: Cell) -> None:
def connect(self, other: Cell, name: Coordinate | None = None) -> None:
"""Connects this cell to another cell.

Args:
other (Cell): other cell to connect to

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please complete the docstring. Name is not defined as one of the arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, because I rebased the PR and not merged main into it, my precommit hooks did not fire locally, so I didn't catch this. Next time I am going to merge again.

Also note that this is indeed an error and happily catched by our CI :) pre-commit.ci

"""
self._connections.append(other)
if name is None:
name = other.coordinate
self.connections[name] = other

def disconnect(self, other: Cell) -> None:
"""Disconnects this cell from another cell.
Expand All @@ -80,7 +87,9 @@ def disconnect(self, other: Cell) -> None:
other (Cell): other cell to remove from connections

"""
self._connections.remove(other)
keys_to_remove = [k for k, v in self.connections.items() if v == other]
for key in keys_to_remove:
del self.connections[key]
quaquel marked this conversation as resolved.
Show resolved Hide resolved

def add_agent(self, agent: CellAgent) -> None:
"""Adds an agent to the cell.
Expand Down Expand Up @@ -123,30 +132,34 @@ def __repr__(self): # noqa

# FIXME: Revisit caching strategy on methods
@cache # noqa: B019
def neighborhood(self, radius=1, include_center=False):
def neighborhood(self, radius: int = 1, include_center: bool = False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to add a docstring.

Also, and not perse necessary for this PR, but the name of this method has been tripping me up. The name suggests that it is an attribute, so perhaps at some point we should change this to something like get_neighborhood

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, there is some revisiting to be done here. I think at one point the idea was that this basically is an attribute/property for the default case, which is just a CellCollection of the the connections. Maybe we could actually have something like

def get_neighborhood(self, radius: int = 1, include_center: bool = False):
     ...

@property
def neighborhood(self):
    return self.get_neighborhood()

So for the "normal" neighborhood you could do cell.neighborhood and for everything more complex cell.get_neighborhood(...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this makes good sense. The most common case is indeed getting the direct neighborhood and it would be good if that is just a property. Naming-wise, what about direct_neighborhood and get_neighborhood? It would be nice if the names were a bit more distinctive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes more distinctive names sounds good. I'll have to think about direct_neighborhood cause I think its a bit verbose. But lets leave that and adding a docstring to a different PR

"""Returns a list of all neighboring cells."""
return CellCollection(
return CellCollection[Cell](
self._neighborhood(radius=radius, include_center=include_center),
random=self.random,
)

# FIXME: Revisit caching strategy on methods
@cache # noqa: B019
def _neighborhood(self, radius=1, include_center=False):
def _neighborhood(
self, radius: int = 1, include_center: bool = False
) -> dict[Cell, list[Agent]]:
# if radius == 0:
# return {self: self.agents}
if radius < 1:
raise ValueError("radius must be larger than one")
if radius == 1:
neighborhood = {neighbor: neighbor.agents for neighbor in self._connections}
neighborhood = {
neighbor: neighbor.agents for neighbor in self.connections.values()
}
if not include_center:
return neighborhood
else:
neighborhood[self] = self.agents
return neighborhood
else:
neighborhood = {}
for neighbor in self._connections:
neighborhood: dict[Cell, list[Agent]] = {}
for neighbor in self.connections.values():
neighborhood.update(
neighbor._neighborhood(radius - 1, include_center=True)
)
Expand Down
4 changes: 2 additions & 2 deletions mesa/experimental/cell_space/discrete_space.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ def all_cells(self):
def __iter__(self): # noqa
return iter(self._cells.values())

def __getitem__(self, key): # noqa
def __getitem__(self, key: tuple[int, ...]) -> T: # noqa: D105
return self._cells[key]

@property
def empties(self) -> CellCollection:
def empties(self) -> CellCollection[T]:
"""Return all empty in spaces."""
return self.all_cells.select(lambda cell: cell.is_empty)

Expand Down
10 changes: 4 additions & 6 deletions mesa/experimental/cell_space/grid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
T = TypeVar("T", bound=Cell)


class Grid(DiscreteSpace, Generic[T]):
class Grid(DiscreteSpace[T], Generic[T]):
"""Base class for all grid classes.

Attributes:
Expand Down Expand Up @@ -100,7 +100,7 @@ def _connect_single_cell_nd(self, cell: T, offsets: list[tuple[int, ...]]) -> No
if self.torus:
n_coord = tuple(nc % d for nc, d in zip(n_coord, self.dimensions))
if all(0 <= nc < d for nc, d in zip(n_coord, self.dimensions)):
cell.connect(self._cells[n_coord])
cell.connect(self._cells[n_coord], d_coord)

def _connect_single_cell_2d(self, cell: T, offsets: list[tuple[int, int]]) -> None:
i, j = cell.coordinate
Expand All @@ -111,7 +111,7 @@ def _connect_single_cell_2d(self, cell: T, offsets: list[tuple[int, int]]) -> No
if self.torus:
ni, nj = ni % height, nj % width
if 0 <= ni < height and 0 <= nj < width:
cell.connect(self._cells[ni, nj])
cell.connect(self._cells[ni, nj], (di, dj))


class OrthogonalMooreGrid(Grid[T]):
Expand All @@ -133,7 +133,6 @@ def _connect_cells_2d(self) -> None:
( 1, -1), ( 1, 0), ( 1, 1),
]
# fmt: on
height, width = self.dimensions

for cell in self.all_cells:
self._connect_single_cell_2d(cell, offsets)
Expand Down Expand Up @@ -165,13 +164,12 @@ def _connect_cells_2d(self) -> None:
( 1, 0),
]
# fmt: on
height, width = self.dimensions

for cell in self.all_cells:
self._connect_single_cell_2d(cell, offsets)

def _connect_cells_nd(self) -> None:
offsets = []
offsets: list[tuple[int, ...]] = []
dimensions = len(self.dimensions)
for dim in range(dimensions):
for delta in [
Expand Down
6 changes: 3 additions & 3 deletions mesa/experimental/cell_space/network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from mesa.experimental.cell_space.discrete_space import DiscreteSpace


class Network(DiscreteSpace):
class Network(DiscreteSpace[Cell]):
"""A networked discrete space."""

def __init__(
Expand Down Expand Up @@ -37,6 +37,6 @@ def __init__(
for cell in self.all_cells:
self._connect_single_cell(cell)

def _connect_single_cell(self, cell):
def _connect_single_cell(self, cell: Cell):
for node_id in self.G.neighbors(cell.coordinate):
cell.connect(self._cells[node_id])
cell.connect(self._cells[node_id], node_id)
11 changes: 6 additions & 5 deletions mesa/experimental/cell_space/voronoi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ def _connect_cells(self) -> None:

for point in self.triangulation.export_triangles():
for i, j in combinations(point, 2):
self._cells[i].connect(self._cells[j])
self._cells[j].connect(self._cells[i])
self._cells[i].connect(self._cells[j], (i, j))
quaquel marked this conversation as resolved.
Show resolved Hide resolved
self._cells[j].connect(self._cells[i], (j, i))

def _validate_parameters(self) -> None:
if self.capacity is not None and not isinstance(self.capacity, float | int):
Expand All @@ -233,9 +233,10 @@ def _validate_parameters(self) -> None:

def _get_voronoi_regions(self) -> tuple:
if self.voronoi_coordinates is None or self.regions is None:
self.voronoi_coordinates, self.regions = (
self.triangulation.export_voronoi_regions()
)
(
self.voronoi_coordinates,
self.regions,
) = self.triangulation.export_voronoi_regions()
return self.voronoi_coordinates, self.regions

@staticmethod
Expand Down
Loading
Loading