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

Fix: Property layer visualization for HexGrid #2646

Merged

Conversation

Sahil-Chhoker
Copy link
Contributor

Summary

Property layers were not properly visualized for HexGrids, and this PR aims to resolve that issue. It utilizes the same coordinate system as the draw_hex_grid function and generates a PolyCollection that overlays color values on the grid.

Bug / Issue

fixes #2433

Before

Screenshot 2025-01-26 203400

After

Screenshot 2025-01-26 203521

Additional Notes:

I am not entirely sure if this was the best approach to address the issue. If there is a more effective or elegant solution, I would greatly appreciate your recommendations.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 -2.3% [-3.4%, -1.2%] 🔵 -1.8% [-2.3%, -1.3%]
BoltzmannWealth large 🔵 -0.0% [-0.5%, +0.5%] 🔵 -0.3% [-0.8%, +0.4%]
Schelling small 🔵 -0.6% [-1.0%, -0.2%] 🔵 -0.3% [-0.4%, -0.2%]
Schelling large 🔵 -0.0% [-0.6%, +0.4%] 🔵 +2.9% [+1.6%, +4.1%]
WolfSheep small 🔵 +0.0% [-0.4%, +0.5%] 🔵 +0.6% [+0.5%, +0.8%]
WolfSheep large 🔵 +1.0% [+0.5%, +1.5%] 🔵 +2.3% [+0.5%, +4.4%]
BoidFlockers small 🔵 -1.1% [-1.9%, -0.3%] 🔵 -0.1% [-0.4%, +0.1%]
BoidFlockers large 🔵 +0.7% [+0.2%, +1.2%] 🔵 -0.1% [-0.5%, +0.2%]

@quaquel
Copy link
Member

quaquel commented Jan 26, 2025

Thanks for this. The example looks great. I am taking a look at the code now.

Copy link
Member

@quaquel quaquel left a comment

Choose a reason for hiding this comment

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

Just a few questions and ideas. Looks very good already.

@Sahil-Chhoker
Copy link
Contributor Author

Are you sure it's fine as it is? I had a few questions about it:

  1. Regarding the repetitive part, I think I’ve got an answer for now. I’ll get back to you as early as tomorrow.
  2. I'm hardcoding the value of zorder to -1. Is this the correct approach?

@quaquel
Copy link
Member

quaquel commented Jan 26, 2025

I gave it a first quick pass and focussed on the high-level stuff. The zorder one is a good question (and actually interacts with #2642). Ideally (but beyond this pr), zorder should become controllable for property layers to explicitly control layering.

@Sahil-Chhoker
Copy link
Contributor Author

Thank you for the review! I have added the suggested changes. Please let me know if you have any additional recommendations to further improve the code quality.

Copy link
Member

@quaquel quaquel left a comment

Choose a reason for hiding this comment

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

looks good, just a few remaining clarifications

mesa/visualization/mpl_space_drawing.py Outdated Show resolved Hide resolved
mesa/visualization/mpl_space_drawing.py Outdated Show resolved Hide resolved
mesa/visualization/mpl_space_drawing.py Outdated Show resolved Hide resolved
@quaquel
Copy link
Member

quaquel commented Jan 28, 2025

I think this is ready to be merged. @Sahil-Chhoker do you have any outstanding doubts? If not, I'll merge this later today.

@Sahil-Chhoker
Copy link
Contributor Author

Thanks @quaquel, I think I'm fine.

@quaquel quaquel merged commit efed03e into projectmesa:main Jan 28, 2025
11 checks passed
@Sahil-Chhoker
Copy link
Contributor Author

@quaquel, I ran into a bug while testing this feature again with the lru_cache functionality.
The function call hexagons = _get_hexmesh(width, height) won't display the grid. I don't know exactly what might be causing this, but it can be fixed by either of these two options:

  1. hexagons = _get_hexmesh(width, height, size=1.0)
  2. Removing lru_cache.

I am very sorry for pushing code without properly testing it.

@quaquel
Copy link
Member

quaquel commented Jan 28, 2025

can you share a minimum example with the bug? I might be able to help diagnose it.

@Sahil-Chhoker
Copy link
Contributor Author

Sorry for the late response, here is my example I was testing the code with:

from mesa.experimental.cell_space.grid import HexGrid
from mesa import Model
import math
from mesa.experimental.cell_space import CellAgent
from mesa.visualization import SolaraViz, make_space_component
from mesa.experimental.cell_space.property_layer import PropertyLayer
import numpy as np

class myAgent(CellAgent):
    def __init__(self, model, cell=None):
        super().__init__(model)
        self.cell = cell
    
 
class myModel(Model): 
    def __init__(self, seed=None, width=10,  height=15 , n_agents=4):
        super().__init__(seed=seed) 
        self.width = width
        self.height = height 
        self.num_agents = n_agents
        self.grid = HexGrid( 
            (self.width, self.height), capacity=math.inf, torus=False, random=self.random
        )
 
        self.test_layer = PropertyLayer("test", (self.width, self.height), default_value=1, dtype=int)
        self.test_layer.data = np.random.randint(2, 10, (self.width, self.height))
        self.grid.add_property_layer(self.test_layer)
        myAgent(self, self.grid.all_cells.cells[17])


def portrayal(agent):
    if agent is None:
        return None
    
    return {
        "color": "tab:orange",
        "size": 70,
    }


def post_process(ax):
    ax.set_aspect("equal")
    ax.set_xticks([])
    ax.set_yticks([])
    ax.get_figure().set_size_inches(8, 8) 

property_layer_portrayal = { 
        "test": { 
            "colormap": "Blues",
            "alpha": 0.7,
            "vmin": 0, 
            "vmax": 10,
        }
    }


model_params = { 
    "seed": {
        "type": "InputText",
        "value": 42,
        "label": "Random Seed",
    },
    "n_agents": {
        "type": "SliderInt",
        "value": 4,
        "min": 1,
        "max": 10,
        "step": 1,
        "label": "Number of Agents",
    },
    "width": 6,
    "height": 5,
}

model = myModel()

space_component = make_space_component(
    agent_portrayal=portrayal, draw_grid=True, post_process=post_process, propertylayer_portrayal=property_layer_portrayal
)

page = SolaraViz(
    model,
    components=[space_component],
    model_params=model_params,
    name="Test Model",
)
page

@Sahil-Chhoker
Copy link
Contributor Author

@quaquel, While testing the code mentioned above, I discovered another bug: whenever the model's step count is increased, the hex grid simply disappears. After diagnosing the issue, I found that lru_cache was once again the culprit. I'm not sure why it’s causing so many problems, but I would recommend removing it from the code entirely.

@quaquel
Copy link
Member

quaquel commented Jan 28, 2025

What happens if you use a normal cache instant of lru_cache?

@Sahil-Chhoker
Copy link
Contributor Author

What happens if you use a normal cache instant of lru_cache?

cache behaves like lru_cache, but it stores results infinitely, unlike lru_cache, which deletes the least recently used values based on its max_size. Ultimately, what I mean is that the results were the same and buggy.

@quaquel
Copy link
Member

quaquel commented Jan 28, 2025

I think I know what's going on. _get_hexmesh is a generator rather than a function. Why did you make it a generator?

@Sahil-Chhoker
Copy link
Contributor Author

Sahil-Chhoker commented Jan 28, 2025

I thought it was cleaner than storing results in a list and returning them. Function does work as intended. Should I change it into a function instead?

@quaquel
Copy link
Member

quaquel commented Jan 28, 2025

It depends on what we prefer. In case of a normal function. we can memoize the return allowing us to effectively generate the hexmesh vertices only once.

@Sahil-Chhoker
Copy link
Contributor Author

Now that I have this knowledge that generators don't go well with caching (thanks to you), I think we should change this to a function.

@quaquel
Copy link
Member

quaquel commented Jan 28, 2025

Can you put in a new PR which does this? Otherwise, I'll pick up tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grid and property layer for hexgrid
2 participants