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

Try to fix issue #2437 #2549

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Try to fix issue #2437 #2549

wants to merge 3 commits into from

Conversation

LunnyRia
Copy link

Summary

This PR removes cell_coloring_property from VoronoiGrid and sets it as an argument when visualizing.

Bug / Issue

Related to Minor refactoring of VoroinoiGrid #2437.

Implementation

In voronoi.py, there are three instances where the cell_coloring_property attribute is mentioned. All of these instances should be commented out (but not deleted). By examining mpl_space_drawing.py, I found that this attribute is used to set the transparency (alpha) of polygons. Based on the last line of voronoi.py, I understand that the developer intended to set its value to 0. However, I don’t quite understand why it was defined as a str, as indicated by cell_coloring_property: str | None = None.

To enable setting the transparency of Voronoi polygons during visualization, I modified mpl_space_drawing.py. In the draw_voronoi_grid function, I added this attribute as a new parameter. GPT and Claude suggested more refined approaches for this visualization, but I want to first confirm that my modification doesn’t introduce any bugs and hear your opinions on it.

Testing

Using the .pre-commit-config.yaml file from the root folder of the Mesa project, I ran pre-commit run --all-files locally, and all checks passed.

Additional Notes

I just started learning Mesa and look forward to your suggestions. Thank you!

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 +0.3% [-1.2%, +1.7%] 🔵 -0.3% [-0.4%, -0.1%]
BoltzmannWealth large 🔵 +1.3% [+0.5%, +2.0%] 🔵 -1.0% [-2.6%, +0.2%]
Schelling small 🔵 +0.1% [-0.2%, +0.4%] 🔵 -0.5% [-0.6%, -0.4%]
Schelling large 🔵 -1.0% [-3.9%, +0.6%] 🔵 +2.3% [+1.8%, +2.8%]
WolfSheep small 🔵 -0.3% [-0.7%, +0.1%] 🔵 +0.6% [-0.4%, +1.7%]
WolfSheep large 🔵 -0.4% [-0.9%, +0.1%] 🔵 -0.9% [-2.2%, +0.6%]
BoidFlockers small 🔵 -0.6% [-1.3%, +0.0%] 🔵 -1.0% [-1.9%, +0.0%]
BoidFlockers large 🔵 -1.0% [-1.7%, -0.3%] 🔵 -0.1% [-0.7%, +0.5%]

@EwoutH EwoutH requested a review from quaquel December 14, 2024 05:55
@quaquel
Copy link
Member

quaquel commented Dec 14, 2024

Thanks for taking a stab at this issue.

  1. Can you make the title a bit more descriptive?
  2. Why don't you delete the old code?
  3. This version of the PR removes functionality: i.e., we can no longer control the alpha of the individual voroinoi cells. How difficult would it be to modify draw_voronoi_grid to accept a color and/or alpha kwarg?

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.

2 participants