Refactor SpaceRenderer API to separate setup and draw methods #2893
Conversation
for more information, see https://pre-commit.ci
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe visualization rendering API is refactored to use a chainable setup/draw pattern with internal state management. Parameter names are standardized across space drawers ( Changes
Sequence Diagram(s)sequenceDiagram
participant SV as Solara Viz
participant SR as SpaceRenderer
participant SD as Space Drawer
Note over SV,SD: Previous Flow (Parameter Passing)
SV->>SR: draw_structure(**space_kwargs)
SR->>SD: draw_matplotlib(**space_kwargs)
SD->>SD: Apply kwargs
Note over SV,SD: New Flow (State-Based Setup & Draw)
SV->>SR: setup_structure(**draw_space_kwargs)
Note over SR: Store draw_space_kwargs internally
SR-->>SV: return self (for chaining)
SV->>SR: setup_agents(agent_portrayal, **kwargs)
Note over SR: Store agent_portrayal & draw_agent_kwargs
SR-->>SV: return self (for chaining)
SV->>SR: render()
Note over SR: Use stored state
SR->>SD: draw_matplotlib() [no args]
Note over SD: Use default behavior
SD-->>SR: rendered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mesa/visualization/space_renderer.py (1)
350-361: Critical bug: References to non-existent attributes.Lines 355 and 357 reference
self.space_kwargsandself.agent_kwargs, but these attributes don't exist. The correct names areself.draw_space_kwargsandself.draw_agent_kwargs.Apply this diff to fix the bug:
if self.space_mesh: - structure = self.draw_structure(**self.space_kwargs) + structure = self.draw_structure() if self.agent_mesh: - agents = self.draw_agents(self.agent_portrayal, **self.agent_kwargs) + agents = self.draw_agents() if self.propertylayer_mesh: - prop_base, prop_cbar = self.draw_propertylayer( - self.propertylayer_portrayal - ) + prop_base, prop_cbar = self.draw_propertylayer()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mesa/visualization/solara_viz.py(2 hunks)mesa/visualization/space_drawers.py(21 hunks)mesa/visualization/space_renderer.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sahil-Chhoker
Repo: projectmesa/mesa PR: 2803
File: mesa/visualization/backends/altair_backend.py:312-553
Timestamp: 2025-06-20T16:44:20.677Z
Learning: In mesa/visualization/backends/altair_backend.py, the author prefers to keep the draw_propertylayer method intact despite its complexity, as they believe breaking it into smaller functions would divert attention from the main logic flow. The method is well-documented and follows a clear sequence of operations for property layer rendering.
📚 Learning: 2025-06-20T16:44:20.677Z
Learnt from: Sahil-Chhoker
Repo: projectmesa/mesa PR: 2803
File: mesa/visualization/backends/altair_backend.py:312-553
Timestamp: 2025-06-20T16:44:20.677Z
Learning: In mesa/visualization/backends/altair_backend.py, the author prefers to keep the draw_propertylayer method intact despite its complexity, as they believe breaking it into smaller functions would divert attention from the main logic flow. The method is well-documented and follows a clear sequence of operations for property layer rendering.
Applied to files:
mesa/visualization/space_renderer.pymesa/visualization/solara_viz.pymesa/visualization/space_drawers.py
🪛 GitHub Actions: build
mesa/visualization/space_renderer.py
[error] 1-1: SpaceRenderer API mismatch: draw_structure() does not accept grid_color as a keyword, causing failures in Boltzmann model visualization tests.
[error] 1-1: SpaceRenderer API mismatch: draw_agents() does not accept agent_portrayal as a keyword, causing failures in Conways and EpStein visualization tests.
[error] 1-1: SpaceRenderer API mismatch: render() does not accept agent_portrayal as a keyword, causing failures in Schelling, Boid Flockers, and Sugarscape tests.
[error] 1-1: SpaceRenderer API mismatch: draw_structure() does not accept node_kwargs as a keyword, causing failures in Virus on Network visualization.
[error] 1-1: SpaceRenderer API mismatch: draw_agents() does not accept agent_portrayal as a keyword, causing failures in EpStein Civil Violence and Wolf-Sheep tests.
[error] 1-1: SpaceRenderer API mismatch: draw_propertylayer() is invoked with a positional argument in tests, but the method signature does not expect it, causing failure in no-property-layers test.
🔇 Additional comments (8)
mesa/visualization/space_drawers.py (2)
85-90: LGTM - Parameter naming improved.The parameter rename from
space_kwargstodraw_space_kwargsmakes the API more explicit and clear.
126-133: LGTM - Consistent parameter naming.The parameter rename from
chart_kwargstodraw_chart_kwargsis consistent with the matplotlib path changes.mesa/visualization/space_renderer.py (5)
7-7: LGTM - Future annotations import.Good practice to enable forward references for type hints.
73-77: Good: Internal state for chainable API.The addition of internal state variables enables the chainable setup/draw pattern, which improves the API ergonomics.
173-186: LGTM - Chainable setup method.The
setup_structure()method follows the builder pattern by returningselffor method chaining.
187-202: LGTM - Chainable setup method for agents.The
setup_agents()method properly stores both the portrayal function and drawing kwargs.
203-217: LGTM - Chainable setup method for property layers.The
setup_propertylayer()method completes the setup API trio.mesa/visualization/solara_viz.py (1)
273-277: Rewrite the review comment: The draw methods are called without setup_* methods, and without ensuring the renderer has configured portrayal functions. This will causecollect_agent_datato crash when trying to callNoneas theagent_portrayalfunction.Either:
- Call
setup_agents(),setup_structure(), andsetup_propertylayer()with appropriate portrayal functions before drawing, or- Provide default portrayal functions (as done in
make_altair_space), or- Document that the renderer must have portrayal pre-configured before being passed to
SpaceRendererComponentVerify the intended API pattern and ensure portrayal is properly initialized.
| def draw_structure(self): | ||
| """Draw the space structure. | ||
| Returns: | ||
| The visual representation of the space structure. | ||
| """ | ||
| self.space_mesh = self.backend_renderer.draw_structure(**self.draw_space_kwargs) | ||
| return self.space_mesh | ||
|
|
There was a problem hiding this comment.
Breaking change: draw_structure() signature changed.
The method signature changed from draw_structure(**kwargs) to draw_structure() with no parameters. This is a breaking change causing pipeline failures. Consider maintaining backwards compatibility by accepting optional kwargs and issuing a deprecation warning.
Example backwards-compatible approach:
def draw_structure(self, **kwargs):
"""Draw the space structure.
Args:
**kwargs: (Deprecated) Additional keyword arguments for drawing.
Use setup_structure() instead.
Returns:
The visual representation of the space structure.
"""
if kwargs:
warnings.warn(
"Passing kwargs to draw_structure() is deprecated. "
"Use setup_structure(**kwargs) before calling draw_structure().",
DeprecationWarning,
stacklevel=2,
)
self.draw_space_kwargs.update(kwargs)
self.space_mesh = self.backend_renderer.draw_structure(**self.draw_space_kwargs)
return self.space_mesh🤖 Prompt for AI Agents
In mesa/visualization/space_renderer.py around lines 219 to 227,
draw_structure() was changed to accept no parameters which breaks callers;
restore backwards compatibility by changing the signature to def
draw_structure(self, **kwargs):, detect if kwargs is non-empty, emit a
DeprecationWarning (use warnings.warn(..., DeprecationWarning, stacklevel=2)),
merge/update self.draw_space_kwargs with the passed kwargs, then call
self.backend_renderer.draw_structure(**self.draw_space_kwargs) and return the
mesh; ensure warnings is imported at top if not already.
| def draw_agents(self): | ||
| """Draw agents on the space. | ||
| Returns: | ||
| The visual representation of the agents. | ||
| """ | ||
| # Prepare data for agent plotting | ||
| arguments = self.backend_renderer.collect_agent_data( | ||
| self.space, agent_portrayal, default_size=self.space_drawer.s_default | ||
| self.space, self.agent_portrayal, default_size=self.space_drawer.s_default | ||
| ) | ||
| arguments = self._map_coordinates(arguments) | ||
|
|
||
| self.agent_mesh = self.backend_renderer.draw_agents( | ||
| arguments, **self.agent_kwargs | ||
| arguments, **self.draw_agent_kwargs | ||
| ) | ||
| return self.agent_mesh |
There was a problem hiding this comment.
Breaking change: draw_agents() signature changed.
The method signature changed from draw_agents(agent_portrayal, **kwargs) to draw_agents(). This breaks existing code as shown in pipeline failures. Add backwards compatibility.
Example backwards-compatible approach:
def draw_agents(self, agent_portrayal=None, **kwargs):
"""Draw agents on the space.
Args:
agent_portrayal: (Deprecated) Function that takes an agent and returns AgentPortrayalStyle.
Use setup_agents() instead.
**kwargs: (Deprecated) Additional keyword arguments for drawing.
Returns:
The visual representation of the agents.
"""
if agent_portrayal is not None:
warnings.warn(
"Passing agent_portrayal to draw_agents() is deprecated. "
"Use setup_agents(agent_portrayal, **kwargs) before calling draw_agents().",
DeprecationWarning,
stacklevel=2,
)
self.agent_portrayal = agent_portrayal
self.draw_agent_kwargs.update(kwargs)
# Existing logic...
arguments = self.backend_renderer.collect_agent_data(
self.space, self.agent_portrayal, default_size=self.space_drawer.s_default
)
arguments = self._map_coordinates(arguments)
self.agent_mesh = self.backend_renderer.draw_agents(
arguments, **self.draw_agent_kwargs
)
return self.agent_mesh🤖 Prompt for AI Agents
In mesa/visualization/space_renderer.py around lines 228 to 243, restore
backward compatibility for draw_agents by allowing the previous signature
draw_agents(agent_portrayal, **kwargs): accept an optional agent_portrayal param
and **kwargs, warn with DeprecationWarning if agent_portrayal is provided, set
self.agent_portrayal and merge kwargs into self.draw_agent_kwargs, and then
proceed with the existing logic (collect_agent_data, _map_coordinates, backend
draw_agents) so existing callers continue to work while encouraging use of
setup_agents; ensure to import warnings at the top of the file.
| def render(self): | ||
| """Render the complete space with structure, agents, and property layers. | ||
| It is an all-in-one method that draws everything required therefore eliminates | ||
| the need of calling each method separately, but has a drawback, if want to pass | ||
| kwargs to customize the drawing, they have to be broken into | ||
| space_kwargs and agent_kwargs. | ||
| Args: | ||
| agent_portrayal (Callable | None): Function that returns AgentPortrayalStyle. | ||
| If None, agents won't be drawn. | ||
| propertylayer_portrayal (Callable | dict | None): Function that returns | ||
| PropertyLayerStyle or dict with portrayal parameters. If None, | ||
| property layers won't be drawn. | ||
| post_process (Callable | None): Function to apply post-processing to the canvas. | ||
| **kwargs: Additional keyword arguments for drawing functions. | ||
| * ``space_kwargs`` (dict): Arguments for ``draw_structure()``. | ||
| * ``agent_kwargs`` (dict): Arguments for ``draw_agents()``. | ||
| the need of calling each method separately. | ||
| """ | ||
| space_kwargs = kwargs.pop("space_kwargs", {}) | ||
| agent_kwargs = kwargs.pop("agent_kwargs", {}) | ||
| if self.space_mesh is None: | ||
| self.draw_structure(**space_kwargs) | ||
| if self.agent_mesh is None and agent_portrayal is not None: | ||
| self.draw_agents(agent_portrayal, **agent_kwargs) | ||
| if self.propertylayer_mesh is None and propertylayer_portrayal is not None: | ||
| self.draw_propertylayer(propertylayer_portrayal) | ||
| self.draw_structure() | ||
| if self.agent_mesh is None and self.agent_portrayal is not None: | ||
| self.draw_agents() | ||
| if self.propertylayer_mesh is None and self.propertylayer_portrayal is not None: | ||
| self.draw_propertylayer() | ||
|
|
||
| self.post_process_func = post_process | ||
| return self |
There was a problem hiding this comment.
Breaking change: render() signature may need parameters.
Based on pipeline failures, render() is being called with agent_portrayal as a keyword argument. Consider accepting these parameters for backwards compatibility.
Consider adding backwards compatibility:
def render(self, agent_portrayal=None, propertylayer_portrayal=None, **kwargs):
"""Render the complete space with structure, agents, and property layers.
Args:
agent_portrayal: (Deprecated) Function for agent portrayal. Use setup_agents() instead.
propertylayer_portrayal: (Deprecated) Function for property layer portrayal. Use setup_propertylayer() instead.
**kwargs: (Deprecated) Additional keyword arguments.
"""
if agent_portrayal is not None or propertylayer_portrayal is not None or kwargs:
warnings.warn(
"Passing parameters to render() is deprecated. "
"Use setup_structure(), setup_agents(), and setup_propertylayer() before calling render().",
DeprecationWarning,
stacklevel=2,
)
if agent_portrayal is not None:
self.agent_portrayal = agent_portrayal
if propertylayer_portrayal is not None:
self.propertylayer_portrayal = propertylayer_portrayal
self.draw_space_kwargs.update(kwargs)
# Existing logic...🤖 Prompt for AI Agents
In mesa/visualization/space_renderer.py around lines 321 to 334, render()
currently accepts no parameters but callers pass agent_portrayal (breaking
compatibility); update the signature to def render(self, agent_portrayal=None,
propertylayer_portrayal=None, **kwargs): and add a deprecation handling block
that (1) issues warnings.warn(..., DeprecationWarning, stacklevel=2) when any of
those args are provided, (2) assigns self.agent_portrayal and
self.propertylayer_portrayal when non-None, and (3) merges any remaining kwargs
into self.draw_space_kwargs (or an existing dict used for draw params), then
continue with the existing render logic so old callers keep working while
encouraging use of setup_* methods.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sahil-Chhoker
changed the title
|
I made this kind of in a hurry. I’ll update the tests and add the documentation over the weekend. In the meantime, can you take a look at just the API to see if it’s working fine (it was for me)? |
quaquel
left a comment
quaquel
left a comment
There was a problem hiding this comment.
The code looks basically fine to me.
2 points:
- Do we want to keep backward compatibility? I am fine with breaking things, but it's good to discuss this
- All examples still need to be updated. Do you plan to make that part of this PR?
We did just release an API update over on a 3.x version, updating it again in 3.3.x wouldn't be very wise in my opinion.
I intend to, what do you suggest? |
Fair enough, but then we should ensure that we don't break it. So, e.g., def draw_agents(self, *args, **kwargs):
if args or kwargs: # implicit booleanness of empty list/kwarg to be checked if correct
warnings.warn("some message", DeprecationWarning, stack_level=2)
self.agent_portrayal = args[0]
self_draw_kwargs = kwargs
# normal code goes here.
Excellent, fixing it in this pr makes most sense to me. |
3 participants
Summary
Improves the current visualization API for
SpaceRenderer.Motive
Refer to #2874.
Implementation
Separated the draw functions in two parts, setup functions, used to update states and draw functions, used to draw stuff (agents, structure, property layer).
Usage Examples
Additional Notes
@quaquel I couldn't recreate the matplotlib bug, so it would be very helpful if you can provide some kind of example.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.