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.

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

experimental: Integrate PropertyLayers into cell space #2319

Merged
merged 9 commits into from
Oct 1, 2024

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Sep 23, 2024

Integrate the PropertyLayers into the cell space. Initially only in the DiscreteSpace, and thereby the Grids.

Since we're in the experimental space, this PR is ready for review as-is.

In future PRs:

  • Add a more comprehensive select function
    def select_cells(
  • Integrate PropertyLayer into CellCollection
    • Thinking about how to do this fast. For larger collections, keeping a mask is probably most performant, but for smaller collections, maybe just iterate though the cells.

Integrate the PropertyLayers into the cell space. Initially only in the DiscreteSpace, and thereby the Grids.
@EwoutH EwoutH added feature Release notes label experimental Release notes label labels Sep 23, 2024
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 +0.5% [-0.3%, +1.2%] 🔵 -0.1% [-0.2%, +0.0%]
BoltzmannWealth large 🔵 -0.5% [-0.9%, -0.2%] 🔵 +0.6% [+0.3%, +0.9%]
Schelling small 🔵 +1.4% [+1.1%, +1.6%] 🔵 -0.3% [-0.5%, -0.1%]
Schelling large 🔵 +1.3% [+0.8%, +1.8%] 🔵 +0.4% [-0.1%, +0.9%]
WolfSheep small 🔵 +1.7% [+1.5%, +1.9%] 🔵 +1.8% [+1.7%, +2.0%]
WolfSheep large 🔵 +0.9% [+0.2%, +1.5%] 🔵 +1.1% [+0.7%, +1.5%]
BoidFlockers small 🔵 -2.9% [-3.2%, -2.5%] 🔵 -2.3% [-2.9%, -1.8%]
BoidFlockers large 🟢 -3.6% [-4.1%, -3.0%] 🔵 -2.6% [-2.9%, -2.2%]

@quaquel
Copy link
Member

quaquel commented Sep 23, 2024

It's remarkably straightforward. Good to see.

@EwoutH
Copy link
Member Author

EwoutH commented Sep 24, 2024

If there are any other remarks please let me know.

@Corvince
Copy link
Contributor

It's remarkably straightforward. Good to see.

I can second that. When I first skimmed this PR I thought its not ready yet, because its not a lot of code. Would have expected more work to be needed. Great to see! One thing I need more time to think about is how the individual cell interacts with the layer. Right now a reference to the whole Property layer ist passed. But Cells also have a "properties" attribute. I would have imagined that inside this properties dictionary there is just a "view" to the propertylayers individual coordinate. Does that make sense?

@quaquel
Copy link
Member

quaquel commented Sep 24, 2024

One last small request: 4 lines are currently not covered by the tests. Can you make sure we have 100% coverage on this? Its a small thing to fix now, but it adds up towards the future.

@EwoutH
Copy link
Member Author

EwoutH commented Sep 24, 2024

Agreed, feel free to add them, otherwise I will pick it up later this week.

Thereby you lose control over how you're adding the values to the cells.
@EwoutH
Copy link
Member Author

EwoutH commented Sep 27, 2024

Updated the tests, and removed the possibility to initialize grids with propertylayers, because that doesn't give you the control if you want those layers added to the cells or not.

I'm struggeling a bit with how - on a conceptual level - we want Cell poperties and property_layers to interact / coexist / complement each other. Functionally it's clear, poperties are for individual cell properties (also other than numbers) and property_layers are for high-performance properties that are shared though a whole space.

Input is welcome.

@quaquel
Copy link
Member

quaquel commented Sep 27, 2024

I'm struggeling a bit with how - on a conceptual level - we want Cell poperties and property_layers to interact / coexist / complement each other. Functionally it's clear, poperties are for individual cell properties (also other than numbers) and property_layers are for high-performance properties that are shared though a whole space.

I don't understand the point, but also in the code, I am a bit lost. In my understanding, property layers are define at the space level and individual cells can only access their values in those same property layers. However it seems cell now has its own property layer dict?

@@ -69,6 +71,7 @@ def __init__(
self.capacity: int = capacity
self.properties: dict[Coordinate, object] = {}
self.random = random
self.property_layers: dict[str, PropertyLayer] = {}
Copy link
Member

@quaquel quaquel Sep 27, 2024

Choose a reason for hiding this comment

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

sorry I don't understand why this is here. should this not be linked to the property layers defined at the grid level?

Copy link
Member Author

@EwoutH EwoutH Sep 27, 2024

Choose a reason for hiding this comment

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

It also is, but it might be useful to be able to access your value in a PropertyLayer directly from the cell.

A few lines below you can see that a Cell can do get_property and set_property.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that and agree with it. I now also looked at add_property_layer and start to understand what is going on.

  1. I suggest making this self._property_layers so it's not declared as part of the public API. My concern is that one might add a layer via cell directly which would then not exist on the grid.
  2. I think overall, a more elegant solution is possible here (but that can be a separate PR). My idea would be to add on the fly Descriptors to cells for each property layer. This would make it possible to de cell.property_layer_name, which would return the value for this cell in the named property layer. Another benefit would be that we don't have code by default in cell that is only functional in OrthogonalGrids.

Copy link
Contributor

Choose a reason for hiding this comment

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

On 2) my high-level idea would be that we use the existing property dictionary of the cell and add to it an entry with the name of the property layer, so cell.properties[property_layer_name]. getting the value should be easy, but I don't know how that would work for setting the value. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of an immediate solution other than to subclass dict and have a dedicated PropertiesDict for all this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was thinking about that. It might be great as an API, but can also create naming conflicts.

Unfortunately, I'm really making a sprint on my thesis now, so not much conceptual-thought capacity to work on this.

Copy link
Member

Choose a reason for hiding this comment

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

With the one minor change (to a private attribute), I think we can merge this for now. I might have some time over the weekend to play around with more descriptor fun 😄 and see what I can do.

Copy link
Member Author

@EwoutH EwoutH Sep 30, 2024

Choose a reason for hiding this comment

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

Go ahead in making the change and merging!

It's all experimental, we can go YOLO.

@quaquel quaquel merged commit 038c9c2 into projectmesa:main Oct 1, 2024
11 of 12 checks passed
@quaquel quaquel deleted the propertylayer_cell_space branch October 1, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Release notes label feature Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants