-
Notifications
You must be signed in to change notification settings - Fork 880
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
Conversation
Integrate the PropertyLayers into the cell space. Initially only in the DiscreteSpace, and thereby the Grids.
Performance benchmarks:
|
It's remarkably straightforward. Good to see. |
instead of the methods, directly access them.
If there are any other remarks please let me know. |
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? |
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. |
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.
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 Input is welcome. |
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? |
mesa/experimental/cell_space/cell.py
Outdated
@@ -69,6 +71,7 @@ def __init__( | |||
self.capacity: int = capacity | |||
self.properties: dict[Coordinate, object] = {} | |||
self.random = random | |||
self.property_layers: dict[str, PropertyLayer] = {} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
- 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. - 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
mesa/mesa/space.py
Line 872 in 4c82989