Added AgentPortrayalStyle and PropertyLayerStyle
#2786
Conversation
|
Performance benchmarks:
|
for more information, see https://pre-commit.ci
…er/mesa into portrayal-components
|
@coderabbitai full review |
|
""" WalkthroughThe changes introduce two new dataclasses, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VisualizationFunction
participant AgentPortrayalFn
participant AgentPortrayalStyle
participant PropertyLayerPortrayalFn
participant PropertyLayerStyle
User->>VisualizationFunction: Call draw_space(..., agent_portrayal, ...)
VisualizationFunction->>AgentPortrayalFn: agent_portrayal(agent)
AgentPortrayalFn-->>VisualizationFunction: AgentPortrayalStyle instance
VisualizationFunction->>AgentPortrayalStyle: Access style attributes
VisualizationFunction->>VisualizationFunction: Render agent using style
User->>VisualizationFunction: Call draw_property_layers(..., propertylayer_portrayal, ...)
VisualizationFunction->>PropertyLayerPortrayalFn: propertylayer_portrayal(property_layer)
PropertyLayerPortrayalFn-->>VisualizationFunction: PropertyLayerStyle instance
VisualizationFunction->>PropertyLayerStyle: Access style attributes
VisualizationFunction->>VisualizationFunction: Render property layer using style
Possibly related issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
mesa/visualization/components/__init__.py (1)
21-27:⚠️ Potential issue
propertylayer_portrayaltyping & docstring are out-of-date
mpl_space_drawing.draw_property_layersnow expects a callable that returns aPropertyLayerStyle, butmake_space_componentstill advertises (and type-hints) this parameter as adict. Passing a dict will raiseTypeError: 'dict' object is not callable.- propertylayer_portrayal: dict | None = None, + propertylayer_portrayal: Callable | None = None,Please adjust the docstring accordingly (
Dictionary of PropertyLayer portrayal specifications→ “Callable returning aPropertyLayerStyle”) and propagate the new type to any other interfaces (draw_space, examples, docs).mesa/visualization/mpl_space_drawing.py (3)
161-175: 🛠️ Refactor suggestion
draw_spacestill advertises dict-based portrayalThe function signature and docstring say
propertylayer_portrayal: dict | None, but the implementation now forwards this value todraw_property_layers, which calls it as a function (portrayal = propertylayer_portrayal(layer)).
Consequently, passing a dict will crash at runtime.Please switch the annotation to
Callable | None(mirroring the behaviour indraw_property_layers) and update the surrounding docstring.
251-276:⚠️ Potential issueCallable vs. Dict mismatch may raise
TypeError
draw_property_layersassumespropertylayer_portrayalis callable. If users still pass a dict (as the public API previously required), this line will fail:portrayal = propertylayer_portrayal(layer)Consider:
- Adding a compatibility shim similar to the one in
collect_agent_datathat recognises a dict and issues a deprecation warning, or- Updating all upstream signatures/guides/examples to require a callable.
Fail-fast with a clear message if the input is neither callable nor
None.
688-696:⚠️ Potential issueBoolean-mask bug in
_scatterproducesTypeError
mark_maskis currently built as a Python list, which cannot participate in element-wise&with a NumPy array (zorder_mask). At runtime this raises:
TypeError: unsupported operand type(s) for &: 'list' and 'numpy.ndarray'- mark_mask = [m == mark for m in list(marker)] + # Vectorised NumPy comparison – much faster and avoids the type error + mark_mask = marker == mark ... - logical = mark_mask & zorder_mask + logical = np.logical_and(mark_mask, zorder_mask)Add a small unit test that plots two markers with different z-orders to catch this regression in CI.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mesa/visualization/components/__init__.py(2 hunks)mesa/visualization/components/portrayal_components.py(1 hunks)mesa/visualization/mpl_space_drawing.py(16 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mesa/visualization/components/__init__.py (1)
mesa/visualization/components/portrayal_components.py (2)
AgentPortrayalStyle(17-51)PropertyLayerStyle(55-78)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
mesa/visualization/components/__init__.py (1)
21-36: 🛠️ Refactor suggestionUpdate type hints & doc-strings to the new Callable-based
propertylayer_portrayalThe function signature and docs still describe
propertylayer_portrayalas adict, but the implementation inmpl_space_drawing.draw_property_layersnow expects a callable returning aPropertyLayerStyle.Please align the public interface:
- propertylayer_portrayal: dict | None = None, + propertylayer_portrayal: Callable | None = None, ... - propertylayer_portrayal: Dictionary of PropertyLayer portrayal specifications + propertylayer_portrayal: A callable that receives a `PropertyLayer` + instance and returns a `PropertyLayerStyle` (or `None` to skip).Failing to do so misleads users and static type checkers alike.
mesa/visualization/mpl_space_drawing.py (1)
290-307: 🛠️ Refactor suggestionEdge case: generated colormap may explode if
vmin == vmaxWhen all layer values are identical, the normalisation
rgba = (data - vmin) / (vmax - vmin)will divide by zero. Guard against this by early-returning or perturbingvmax:if vmin == vmax: vmax = vmin + 1e-9This prevents RuntimeWarnings and NaNs showing up in the visualisation.
♻️ Duplicate comments (2)
mesa/visualization/components/__init__.py (1)
15-18:__all__still clobbers the pre-existing public API – extend, don’t overwriteThe PR keeps the previous behaviour that replaces
__all__with just the two new classes.
Everything that used to be publicly re-exported from this package (e.g.SpaceMatplotlib,make_space_component, …) silently disappears and will breakfrom mesa.visualization.components import …in downstream code.-__all__ = [ - "AgentPortrayalStyle", - "PropertyLayerStyle", -] +# Preserve the earlier exports (if any) and append the newcomers +try: + __all__ # type: ignore[name-defined] +except NameError: + __all__: list[str] = [] + +__all__ += [ + "AgentPortrayalStyle", + "PropertyLayerStyle", +]mesa/visualization/components/portrayal_components.py (1)
35-45: Usetyping.Anyand consider a more Pythonicupdate(**kwargs)
anyis the built-in predicate, not a type. Static analysers will complain.-from dataclasses import dataclass +from dataclasses import dataclass +from typing import Any ... - def update(self, *updates_fields: tuple[str, any]): + def update(self, *updates_fields: tuple[str, Any]):While touching this, you may want to offer the more idiomatic signature
update(**fields_to_change)to avoid the slightly awkwardstyle.update(("color", "red"))call-site.Optionally add
__slots__ = ()to both dataclasses; these objects can be created thousands of times per tick and the space saving is noticeable.
🧹 Nitpick comments (2)
mesa/visualization/components/portrayal_components.py (1)
16-34: Lightweight optimisation: add__slots__to both style dataclassesAs these style objects are instantiated per agent / per layer every frame, ditching the per-instance
__dict__can cut memory usage and speed up attribute access:@dataclass class AgentPortrayalStyle: + __slots__ = ("x", "y", "color", "marker", "size", "zorder", + "alpha", "edgecolors", "linewidths")Repeat for
PropertyLayerStyle.mesa/visualization/mpl_space_drawing.py (1)
45-59: Doc-string & param type drift incollect_agent_data
default_sizewas introduced, but:
- The doc-string still talks about “limited to size, color …” dict return values.
agent_portrayalnow must yield anAgentPortrayalStyle, but theArgs:section doesn’t mention the dataclass.Please refresh the doc-string accordingly to avoid user confusion and mismatched IDE hints.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mesa/visualization/components/__init__.py(2 hunks)mesa/visualization/components/portrayal_components.py(1 hunks)mesa/visualization/mpl_space_drawing.py(16 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mesa/visualization/components/__init__.py (1)
mesa/visualization/components/portrayal_components.py (2)
AgentPortrayalStyle(17-51)PropertyLayerStyle(55-78)
for more information, see https://pre-commit.ci
Sahil-Chhoker
added
breaking
|
@Sahil-Chhoker -- impressive work! Can you add a mesa example, showing what implementation looks like for the user? Appreciating this is a breaking change, we are going to have to do at least do a transition, where the existing approach works and agentportrayalstyle and propertylayerstyle works with a warning to transition. Unless there is a way to do a conversion so it will stay backwards compatible. As this is GSoC, can you add some cprofile information, just to see where the processing is occurring. I am curious why WolfSheep and BoidFlockers increased while BoltzMannWealth and Schelling went down. |
Sure, will do.
The agent portrayal currently works for both dict and
I will look into it. |
Thanks @Sahil-Chhoker! |
Sahil-Chhoker
added
the
trigger-benchmarks
Sahil-Chhoker
requested review from
quaquel
and removed request for
quaquel and
EwoutH
|
@EwoutH, @quaquel and @jackiekazil -- Please take a look and let me know if you have any concerns. As this is mesa-vis I would like at least one more approval before merge. Here is @Sahil-Chhoker project timeline for reference --- Timeline: |
|
I have one request: can you add unit tests to ensure we have 100% coverage on the new code? I had a quick look at the code and that looks all fine for now. We'll have to see how it integrates with the rest that you have planned. But that is fine. |
|
I'm not quite able to understand what codecov is mentioning here, am I supposed to write tests for the components |
|
Tests for the new components, not for sugarscape. |
That was weird that sugarscape had its own tests, looking through the history it looks like it is an artifact of moving from the examples repo. @Sahil-Chhoker --- please feel free to delete the tests.py in sugarscape g1mt |
|
All comments addressed so merging --- conversation continuing on at #2772 |
tpike3
added
visualisation
and removed
breaking
|
Congratulations on having this PR merged! |
|
A few things now that I think of it: since this is a breaking change, we should:
|
|
As far as I understand, the PR is not breaking because both dicts and the new class are supported. @Sahil-Chhoker correct me if I am wrong. |
|
Yes the older versions of both |
|
Sorry, I thought that it replaced is. Should have looked better. Having both supported is great for now! I would suggest disabling the warning for now, so users have one overlapping release to switch organically. After that we can start issuing deprecation warnings. |
|
Added a PR removing the deprecation warnings in PR #2797. |
* added AgentPortrayalStyle and PropertyLayerStyle * added docstrings * added coderabbit review * added usage example and fix __init__ file * added backwards compatability for dict propertylayer_portrayal * fix the network logic * null check for agent pos * added tests * delete unwanted test
Summary
Added
AgentPortrayalStyleandPropertyLayerStyle, both are returned from a callable called upon agents and layers respectively.In
AgnetPortrayalStyleone can define:In
PropertyLayerStyleone can define:color and colormap can't coexist.
Motive
Eliminating the use of dictionary in defining styles.
Usage Example
AgnetPortraylStyle:PropertyLayerStyle: