Make connections in experiment cell spaces named

[kurone02]

The proposed solution is to create a new Connection class that stores the connection information of each Cell.

Inside a Connection class, there are three main attributes:

1. naming: A dictionary mapping a str name to an int index.
2. reverse_naming: A dictionary mapping an int index to a str name.
3. connections: A list of Cell containing the connecting cells.

Currently, Connection implements two methods for adding and deleting connections.

I have added additional test cases for OrthogonalMooreGrid similar to the example in #2060.

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
Schelling	small	🔴 +49.5% [+48.9%, +50.1%]	🔴 +7.5% [+7.2%, +7.9%]
Schelling	large	🔴 +46.3% [+45.6%, +47.0%]	🔴 +11.3% [+9.8%, +12.9%]
WolfSheep	small	🔴 +22.7% [+21.3%, +24.1%]	🔴 +3.5% [+3.3%, +3.6%]
WolfSheep	large	🔴 +24.1% [+22.9%, +25.4%]	🔵 +3.8% [+2.6%, +4.9%]
BoidFlockers	small	🔵 +1.0% [+0.3%, +1.7%]	🔵 -0.6% [-1.3%, +0.0%]
BoidFlockers	large	🔵 +0.4% [-0.3%, +1.1%]	🔵 +0.1% [-0.2%, +0.4%]

[quaquel]

Thanks for this. It looks very good already.

Just a few quick questions

1. Currently naming works for Moore/Neumann 2D grids. Why not add 2D HexGrids to this as well?
2. The current implementation has naming as optional by defaulting to None. I am not sure whether this is the desired behavior. I had been thinking about this myself and thought of defaulting to the relative indexing tuple (e.g. (-1, 1) for top left) if no meaningful name can be specified. This scales across all grids and to ND.
3. Do you know what explains the performance loss for Schelling and small WolfSheep?
4. Is the dedicated Connections class really needed? It seems used here to have two-way mapping from names to relative indices and back again, but do we need the reverse naming?

[kurone02]

Thank you for the response.

1. Currently naming works for Moore/Neumann 2D grids. Why not add 2D HexGrids to this as well?

I forgot there is a HexGrid 😅, I'll add the names.

2. The current implementation has naming as optional by defaulting to None. I am not sure whether this is the desired behavior. I had been thinking about this myself and thought of defaulting to the relative indexing tuple (e.g. (-1, 1) for top left) if no meaningful name can be specified. This scales across all grids and to ND.

So there will be the default name for each connection, right? Should the type of the default name be tuple or str? What about the case of a Network space?

3. Do you know what explains the performance loss for Schelling and small WolfSheep?

I ran the benchmarks on my local PC and the performance loss is not significant. Here is the benchmark result.

Model	Size	Init time [95% CI]	Run time [95% CI]
Schelling	small	🔵 -2.6% [-3.5%, -1.6%]	🔵 +0.0% [-0.9%, +0.9%]
Schelling	large	🔵 +0.6% [-1.5%, +3.0%]	🔵 +2.0% [-3.5%, +7.9%]
WolfSheep	small	🔵 -1.5% [-2.9%, -0.1%]	🔵 -1.3% [-2.4%, -0.2%]
WolfSheep	large	🔵 +3.0% [+0.1%, +6.1%]	🔵 +6.2% [+1.8%, +10.3%]
BoidFlockers	small	🔴 +8.8% [+4.6%, +12.9%]	🔵 +7.7% [+0.4%, +15.5%]
BoidFlockers	large	🔵 -1.5% [-7.8%, +4.9%]	🔵 +1.1% [-4.8%, +6.8%]

Could you re-trigger the performance benchmarks by adding the label trigger-benchmarks?

4. Is the dedicated Connections class really needed?

It's actually not needed, we can move the implementation to the CellCollection class. I'll refactor if it is needed.

It seems used here to have two-way mapping from names to relative indices and back again, but do we need the reverse naming?

It is necessary if we want to support removing the connection by its index (so that we don't have to iterate the naming dict to find the corresponding name). If we don't need to support removing by index then reverse_naming is not needed.

[EwoutH]

Thanks for this effort, and sorry to let you waiting.

While I think named connections are useful, it seems like quite a lot of code for what in my head is a relatively simple feature. But maybe it's needed for this to work properly and I'm underestimating the complexity. Could you expand a bit on this?

Also, @quaquel, what's your take on this?

[kurone02]

Thanks for this effort, and sorry to let you waiting.

While I think named connections are useful, it seems like quite a lot of code for what in my head is a relatively simple feature. But maybe it's needed for this to work properly and I'm underestimating the complexity. Could you expand a bit on this?

Also, @quaquel, what's your take on this?

Hello! Thanks for the response.

I wanted to separate the feature's implementation so that we wouldn't have to change too much in other places (we just need to use append and remove similar to how we were using list)

In the Connection class:

• __init__: For initializing the list of connections, along with its names
• __len__: Returns the number of connections in the list (to support len(conn))
• __getitem__: Returns the connection given its name or id (to support [] operator: conn["top left"])
• __contains__: To check if the given cell is in the list of connections based on the name or id (to support in operator: "top left" in conn)
• append: For adding a new cell to the connection list
• remove: Remove a connection from the list of connections

The complexity in adding and removing the connections mostly comes from dealing with multiple types of input because I don't know the specific requirements so I tried to cover all cases that I can think of. We can remove some of the code to make it simpler.

[EwoutH]

@quaquel I would like your opinion on this before I merge this.