-
Notifications
You must be signed in to change notification settings - Fork 874
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
space: Implement PropertyLayer and _PropertyGrid #1898
Conversation
221b4e3
to
e4be7d8
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1898 +/- ##
==========================================
+ Coverage 79.62% 79.87% +0.24%
==========================================
Files 15 15
Lines 1124 1267 +143
Branches 244 277 +33
==========================================
+ Hits 895 1012 +117
- Misses 198 216 +18
- Partials 31 39 +8 ☔ View full report in Codecov by Sentry. |
307fcf8
to
3adf137
Compare
This PR is now feature complete! Please take a proper look at the code, it's ready for review. Next up:
|
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.
Super excited for this! Obviously still some rough edges, but I think this has very high potential.
I left some comments. One thing that got me thinking. You often define methods that potentially only affect a neighborhood. For this you always have the optional arguments only_neighborhoods, moore, include_center and radius. This somewhat predetermines what a neighborhood can be. But maybe we can define "neighborhood" just as a list of coordinates. Then we can define neighborhoods also based on conditions and only pass in the neighborhoods to these functions. Maybe that would be more general?
I also think some method names are not optimal. But I think to fully judge that we would need some toy model where we can "read" how models are written.
Anyway, again, great work!
mesa/space.py
Outdated
def select_cells_multi_properties( | ||
self, | ||
conditions: dict, | ||
only_neighborhood: bool = False, |
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.
Maybe this could be simplified to a an optional neighborhood argument that precalculated the neighborhood? And _get_neighborhood_mask only masks a neighborhood. That way we don't need all the extra arguments for the neighborhood and this could also be applied for other spaces, where neighborhoods can be defined differently
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.
Agreed, will think about how to make this modular.
If you have any suggestions, let me know!
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.
Implemented! It now takes a mask with a utility method get_neighborhood_mask()
mesa/space.py
Outdated
raise ValueError( | ||
"Condition must be a NumPy array with the same shape as the grid." | ||
) | ||
np.copyto(self.data, value, where=condition) # Conditional in-place update |
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.
cant we update the values in-place, why the copy?
mesa/space.py
Outdated
else: | ||
# Ensure condition is a boolean array of the same shape as self.data | ||
if ( | ||
not isinstance(condition, np.ndarray) |
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.
Wait, is condition a callable that returns a boolean array or already a boolean array?
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.
Excellent catch, I went a little over board there with the optimization. I think I fixed it in 5f34480.
Could you confirm that now makes sense?
mesa/space.py
Outdated
return operation(self.data) | ||
else: | ||
# NumPy ufunc case | ||
return operation(self.data) |
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.
both branches do the same, so I think we dont need the branching?
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.
Nice catch, fixed in 7b9310c.
mesa/space.py
Outdated
def move_agent_to_random_cell( | ||
self, | ||
agent: Agent, | ||
conditions: dict, |
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.
cool idea to randomly move, but only to a specified neighborhood.
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.
Thanks! I think generalizing that is indeed a good idea, I replied below.
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.
Done, you can now insert any mask. Utility methods get_neighborhood_mask()
and get_empty_mask
are provided.
All docs and tests are added. That should reviewing a bit easier.
While this PR is fully implemented and quite specific, feel also free to talk about the big picture stuff. I'm proposing a major conceptual change to the Mesa space stack, and thus discussion on all levels is appreciated. @rht Ruff doesn't allow me to specify a condition as a lambda function: condition = lambda x: x == 0 tests/test_space.py:557:9: E731 Do not assign a `lambda` expression, use a `def`
Found 7 errors.
No fixes available (7 hidden fixes can be enabled with the `--unsafe-fixes` option). Direct insertion is okay, so without using the condition variable in between. Being able to use lambda function to update the property layer is fundamental to this PR, and I like the current explicit |
Yes, I like this. Just pass a sub selection of cells, which can be a neighbourhood but also can be something else. That would be extremely flexible, you can define rooms, areas, basically anything you like.
Struggling with this, really open for suggestions! |
Great work! I'm wondering what type of indices is used for cells. In grid space the coordinate system is in Another question is that it seems to be possible to have property layers with different dimensions than that of the grid space. If this is the case, how do we align them? This also links to the coordinate system mentioned above. In mesa-geo, raster layers can have different dimensions and resolutions, and everything was taken care of through a As an example, if you have a 10x10 grid, and a 20x20 property layer, where would you put it on the grid? If, alternatively, you would prefer to make sure that property layers always have the same dimension as the grid, perhaps we could simplify the API to something like this? # Initialize a SingleGrid
grid = SingleGrid(width=10, height=10, torus=False)
# Create a PropertyLayer for a specific property (e.g., pollution level)
# and add the property layer to the grid
grid.add_property_layer(name='pollution', default_value=0.0)
# or
grid.add_property_layer(name='another_property', values=some_numpy_array_of_the_same_shape) |
Based on Corvince’s insights:
|
Thanks for your insights Wang!
Not even thought of this. 😅
Was also thinking about this for ContinuousSpace. I would say out of scope for this PR, but in general, it would be nice to support this (especially because NetLogo is ContinuousSpace + discrete patches).
Personally I wouldn’t go this way, because now property layers are completely independent. It would be nice to keep them that way. |
d496121
to
377c552
Compare
Quite productive morning so far!
@wang-boyu I added a test in 68992a0 to check the coordinate systems between agents and layers. It looks like they match. Can you check if I tested correctly and haven't forgotten an edge case? @rth I would really like to know what you think about Ruff not allowing assigned lambda functions. Code, test and docs wise, this PR is done. I will now work on some examples to show functionality. I would invite everyone to do that with me and play around with it a bit. This branch can be installed with:
|
I added 3 examples to the main PR message. I think is shows (almost) all functionality. Really curious what everybody thinks! |
Thanks for adding a test case for it. But it's not really what I'm trying to say. I made a small visualization for your forest fire model: codeimport numpy as np
import solara
import matplotlib.pyplot as plt
from matplotlib.figure import Figure
from mesa.experimental import JupyterViz
def space_drawer(model, agent_portrayal):
agents_portrayal = {
"firefighter": {"x": [], "y": [], "c": "tab:red", "marker": "o", "s": 10},
}
for agent in model.schedule.agents:
if isinstance(agent, FirefighterAgent):
agents_portrayal["firefighter"]["x"].append(agent.pos[0])
agents_portrayal["firefighter"]["y"].append(agent.pos[1])
fig, ax = plt.subplots()
im = ax.imshow(model.grid.properties["temperature"].data, cmap="spring")
# im = ax.imshow(model.grid.properties["temperature"].data, cmap="spring", origin="lower")
fig.colorbar(im, orientation="vertical")
ax.scatter(**agents_portrayal["firefighter"])
ax.set_axis_off()
solara.FigureMatplotlib(fig)
model_params = {
"N": 1,
"width": 4,
"height": 4,
}
page = JupyterViz(
ForestFireModel,
model_params,
measures=[],
name="Forest Fire",
space_drawer=space_drawer,
play_interval=1500,
)
page # noqa For simplicity I also changed your temperature_data[:] = np.random.randint(270, 370, temperature_data.shape) to temperature_data[0, 0] = 500 so that the cell at position (0, 0) has a temperature of 500, with other cells having the default temperature of 300. Now the temperature property becomes model = ForestFireModel(1, 4, 4)
model.grid.properties["temperature"].data
array([[500., 300., 300., 300.],
[300., 300., 300., 300.],
[300., 300., 300., 300.],
[300., 300., 300., 300.]]) If we plot it, it looks like which isn't what we want, because (0, 0) is supposed to be at the lower left corner. This is essentially what I was trying to explain earlier. I just realized that the same problem has been dealt with in the sugarscape demo: https://github.com/projectmesa/mesa-examples/blob/7275b172aad8905584dcffc131d9bbd96992abb7/examples/sugarscape_g1mt/app.py#L36-L38. In the demo code above I have a line commented out, that specifies However I should mention that this only makes the model looks correct. The underlying numpy array still has its upper left corner with the 500 temperature. If we go for this approach, we probably need to make sure that
Sorry about all these. I hope this helps clarifying my concern. |
@wang-boyu Thanks for your extensive reply. I now understand:
What I don't understand yet:
Edit: I start to get it better. At 0,0, the agent thinks it's in the lower left, but when asking patch 0,0, it's in the upper right. Oh man this is a head breaker. With you GIS experience, any suggestion how to move forward? |
I already hate this, but adding a translation method that flips the y coordinate of every position like this: def _translate_coords(self, position):
"""
Translates Mesa coordinates to NumPy array coordinates.
"""
x, y = position
return self.height - 1 - y, x And then using that everywhere where a position is inputed: def set_cell(self, position, value):
"""
Update a single cell's value in-place.
"""
self.data[self._translate_coords(position)] = value Would probably fix it right. Except if users start manipulating the class directly, without using the methods... I already hate this. |
Your hate is very understandable. What we could potentially do is use a numpy array with dtype=object for the agents as well. Last time I checked it was marginally slower, but would allow easier consistency. |
And then just flip all the visualisations? |
lol welcome to the world of gis... this is the exact problem I ran into when working on the urban_growth example model. I kept wondering why the results were always flipped around. Regarding this PR, I think this really is just a design decision. How would you prefer the users to use the feature? Possibly:
Regarding the first option, it is also possible to make the numpy array as a Python property without a setter. And when getting the array, we can do the flipping before returning it. But this may be unexpected to those users who are familiar with matrix indexing. We just need to be consistent that it is not (row, col) indexing, but instead (x, y) coordinate. There might be other better options that I missed. |
I thought about it for a bit, and I think this is the right approach. It's basically just a convention for visualisation. In future efforts, we might want to create a visualisation function that included the up-down flip. On a side note: We have to be really careful if we ever want to add something with directions. Because if we do, up is down and down is up.
I like this. Let me work on a implementation. We basically define it like this, right? @property
def data(self):
return self._data As a side note, I really would like feedback on the API and the variable names. This is the stuff that's difficult to change after the first release. So are there any names of methods that veel unintuitive, or could be improved? |
To be honest, I don't know if we want data as a strict property. Doing some initializations as Or should we include a setter and do some checks there? |
Yes I prefer this approach too, which is also consistent with the sugarscape example model . To recap, we assume indexing such as A side effect of this approach is the internal representation using the numpy array will be upside down. To mitigate this problem, there are again a few options which are essentially design decisions:
Regarding the APIs I don't have any preferences. My only questions is whether you would like to call it |
mesa/space.py
Outdated
) | ||
return # Agent stays in the current position | ||
|
||
new_pos = tuple(agent.random.choice(target_cells)) |
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.
This might be surprising that a cell is chosen randomly
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.
What would your alternative be, if we have multiple cells with the same highest or lowest value?
This is also something I noticed and I wonder if we should drop a concrete name altogether. I would be in favor of using more generic names for most of the functions. Anything with cell or even patch in the name implies a grid. If we later extend to 3 dimensional spaces or for networks the terminology doesn't fit anymore. I would prefer to exchange cell with value in most cases. For example instead of And In cases where value doesn't fit maybe we can use pos or position, which is still generic. Otherwise I am quite happy with this PR. Really excellent intro post, great examples. Currently on mobile so I can't to a proper code review, but I agree that having a proper API is currently more important and implementation details can also be improved later (if there are any improvements) |
I notice I’m mentally overloading a bit on this PR. My brain is running out of RAM to track all the discussions and comments. This PR now tracks 85 comments and over a month’s worth of discussion. I would highly recommend merging this experimental feature as-is. Everything works, is tested, and is documented. Making iterative, atomic modifications on each of the potential improvement. This feature is exactly for which #1909 was meant. We can break everything about it an hundred times and scrap it fully if we want to. Please use Squash and merge while merging. I don’t want 30 commits in the commit history. The PR serves as historical reference and grounding. |
@EwoutH I can take over applying the rest of the change requests. Should be done in less than an hour and then I will squash and merge. Well done! |
@EwoutH these are the remaining unaddressed reviews: |
Thanks for taking care of the comments and merging! Very glad it's in! |
Apologies if this is the incorrect place to ask this question, I am relatively new to this community and Github in general, but one issue I have encountered using this awesome feature is how to use it with the datacollector method. I'm trying to implement the sugar/spice model with pollution in Mesa (modifying the sugarscape_g1mt model from mesa-examples), and currently have the following in my datacollector definition:
Everything (as in the rest of the data collection, excluded from the code for space reasons) seems to work as intended, as in the base model, except for the pollution data; it seems that for all steps, only the final pollution property layer is being stored (i.e. in the final results dataframe, the "Pollution" column is identical across model steps). Perhaps this is just me misunderstanding how the data collector works, but I wanted to see if this was potentially just not something that the PropertyLayer class is intended to handle. I imagine there should be some way to do it (for this model, we of course expect the distribution over space of pollution to change with each step), but I'm struggling to figure it out. Any help is greatly appreciated! |
Thanks for reaching out! Can you share a minimal reproducible example? |
Yes, apologies for not doing so initially. Below is an extremely simplified version of what I am trying to do:
If you check the model_results object after running, you will see that all of the arrays in the Pollution column are exactly the same--specifically, they seem to be the final step levels of pollution in each grid cell. The pollution at step 0, at least, should be 0 in every entry, which is not the case. |
Thanks for the minimal example. I can reproduce this bug, and it's indeed unintended behavior. Could you open a new issue for this? I will investigate a bit further in the meantime. |
Thanks for looking into this, I've opened an issue and will check back for updates/to see if anything else is needed. |
Summary
This PR introduces a system for managing cell properties within Mesa's grid classes. It adds a
PropertyLayer
class and extends theSingleGrid
andMultiGrid
classes to support cell properties, enhancing the capability of Mesa for more detailed environmental modeling in simulations.Motive
The addition of cell properties brings a new dimension to agent-based models, allowing them to simulate environmental factors (e.g., terrain types, resources, pollution levels) more realistically.
From a technical perspective, cell properties reduce computational overhead by negating the need for numerous agent objects for static environmental features. Conceptually, they provide a clearer and more intuitive representation of the environment, streamlining model complexity and facilitating analysis.
This feature enhances Mesa's modeling capabilities by enabling dynamic interaction between agents and their environment. It allows for simulations where environmental conditions directly influence agent behavior, thus broadening the scope and depth of possible models.
Implementation
PropertyLayer Class
The
PropertyLayer
class represents a layer of properties associated with the cells of a grid. It includes the following functionalities:_PropertyGrid Class
The
_PropertyGrid
class extends the existing_Grid
class, adding support for managing multiplePropertyLayer
instances. Key features include:Integration with Grid Classes
The existing grid classes, such as
SingleGrid
andMultiGrid
, are derived from_PropertyGrid
. This inheritance allows them to leverage the property layer functionalities seamlessly. These enhanced grid classes enable modelers to define and manipulate cell properties directly, enriching the modeling capabilities of Mesa.Implementation Details
API examples
Here are some code examples of how these features could be utilized.
Initializing a Property Layer and Setting Cell Values
Modifying Cell Values Using Operations.
Note: If performance is important, NumPy Universal functions (ufunc) are often faster.
Conditional Operations Based on Cell Properties
Aggregating Property Values
We can perform an aggregate operation, such as calculating the sum or mean of all values in the property layer.
Complex Cell Selection Based on Condition
We can select cells that meet a complex condition, such as finding cells within a specific range of values.
Model examples
Moving an Agent to a Cell with the Highest Value
In this example, we'll move an agent to the cell with the highest value in a specified property layer.
Example 2: Moving an Agent Based on Multiple Property Conditions
Here, we move an agent to a random cell that satisfies multiple conditions based on different property layers.
Scenario: Forest Fire Simulation
In this simulation, we have a forest with varying levels of temperature, humidity, and elevation. Agents represent firefighters trying to contain a forest fire. The goal is for the firefighters to move towards areas with the highest temperature (indicative of fire) but also consider safety constraints such as avoiding extremely low humidity areas (high fire risk), inaccessible high elevation zones or extremely high temperature zones. They also have a maximum movement radius of 3 (using the neighbor mask) and won't move to a cell where already a firefighter is (using the empty mask).
Key Features of This Example