Make cell connections public and named

[Corvince]

This PR changes the formerly private attribute _connections from the Cell class to be public and to be no longer a list, but a dictionary that includes a name for each connections. For Grids this defaults to the relative direction ( (0, 1), (-1, -1), etc.). For Network this is the node name of the "other" cell.

This allows more powerful and direct agent movements, because you no longer need to query the connection to see where it leads or be dependent on the order in which directions were created. +

This PR also includes some type improvements

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🟢 -4.2% [-5.0%, -3.4%]	🔵 +0.0% [-0.0%, +0.1%]
BoltzmannWealth	large	🔵 -7.3% [-22.1%, +0.5%]	🔵 -0.9% [-4.3%, +3.2%]
Schelling	small	🔴 +107.6% [+107.2%, +108.1%]	🔴 +7.2% [+7.0%, +7.4%]
Schelling	large	🔴 +102.8% [+101.8%, +103.8%]	🔵 +5.7% [+2.8%, +8.9%]
WolfSheep	small	🔴 +41.8% [+40.1%, +43.4%]	🔴 +9.5% [+9.2%, +9.9%]
WolfSheep	large	🔴 +42.6% [+41.5%, +43.6%]	🔴 +7.4% [+6.3%, +8.4%]
BoidFlockers	small	🔵 +3.3% [+2.7%, +3.8%]	🔵 -0.5% [-1.3%, +0.4%]
BoidFlockers	large	🔴 +5.2% [+4.7%, +5.7%]	🔵 -0.6% [-1.1%, -0.1%]

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🔵 -0.2% [-1.0%, +0.6%]	🔵 -1.4% [-1.5%, -1.2%]
BoltzmannWealth	large	🔵 +0.6% [+0.4%, +0.8%]	🔵 +2.7% [-1.3%, +7.2%]
Schelling	small	🔴 +96.3% [+95.8%, +96.9%]	🔴 +5.9% [+5.6%, +6.1%]
Schelling	large	🔴 +88.9% [+88.2%, +89.9%]	🔴 +5.0% [+4.4%, +5.6%]
WolfSheep	small	🔴 +36.1% [+34.4%, +37.6%]	🔴 +8.0% [+7.7%, +8.3%]
WolfSheep	large	🔴 +37.3% [+36.5%, +38.2%]	🔴 +9.4% [+7.9%, +11.0%]
BoidFlockers	small	🔵 -1.2% [-1.7%, -0.6%]	🔵 -0.7% [-1.5%, +0.2%]
BoidFlockers	large	🔵 -0.8% [-1.4%, -0.2%]	🔵 -0.3% [-0.9%, +0.2%]

[quaquel]

I like this PR. I am just wondering whether using a string as key in connections is the way to go (and in voronoi it is not done correctly as far as I can tell).

Any idea about the performance benchmarks? Slower inits make sense because the data structure is a bit more involved to built up. I fail to see why runs are slower?

[Corvince]

The initial idea was to make the connection names completely arbitrary, so users could call them for example "up", "north", "top", etc., whatever seems most appropriate in the specific context.

But probably it makes sense to have that abstraction somewhere else. Relative directions seem to make the most sense. That's unambiguous for grids, for network it kind of makes sense (the direction of node xy) and for voronoi I have to think about it.

So I am probably going to change it to type tuple

[EwoutH]

Interesting PR, coordinates seem more scalable then names.

Probably related:

• Proposal: Formal neighborhood definition for grids #1900

Something like a Neighbours class/object might be useful.

[quaquel]

But probably it makes sense to have that abstraction somewhere else. Relative directions seem to make the most sense. That's unambiguous for grids, for network it kind of makes sense (the direction of node xy) and for voronoi I have to think about it.

I guess Voronoi is a network with some interesting properties (e.g., you can lay it out in 2d without any crossing edges).

[Corvince]

rebased and ready to be reviewed again

[quaquel]

A few really minor things that if addressed would be nice. Good stuff.

[quaquel]

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

[Corvince]

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

[quaquel]

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

[Corvince]

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(...)

[quaquel]

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.

[Corvince]

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