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 API was refactored to a stateful, chainable setup/draw pattern: SpaceRenderer now stores portrayals and draw kwargs via setup_* methods and executes drawings without per-call kwargs. Space drawer parameter names were standardized (space/ chart kwargs renamed), and Solara integration now calls draw_* without forwarding kwargs. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Solara Viz
participant Renderer as SpaceRenderer
participant Drawer as Space Drawer
Note over Page,Renderer: New stateful, chainable flow
Page->>Renderer: setup_structure(**draw_space_kwargs)
Renderer-->>Page: self
Page->>Renderer: setup_agents(agent_portrayal, **draw_agent_kwargs)
Renderer-->>Page: self
Page->>Renderer: setup_propertylayer(propertylayer_portrayal)
Renderer-->>Page: self
Page->>Renderer: render()
Note right of Renderer: Uses stored state
Renderer->>Drawer: draw_matplotlib() / draw_altair()
Drawer-->>Renderer: rendered output
Renderer-->>Page: visualization
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.
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. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
mesa/examples/advanced/epstein_civil_violence/app.py (1)
67-71: Consider consistency with other examples regarding render() vs draw_agents().This example uses
draw_agents()individually, while other examples (e.g., schelling/app.py) userender()which draws all layers. Both patterns appear valid, but for consistency and clarity:
- If you only need to draw agents,
draw_agents()is appropriate- If you want to draw structure + agents,
render()is more conciseConsider adding a comment explaining why
draw_agents()alone is used here, or standardize onrender()if the structure should also be drawn.mesa/examples/advanced/sugarscape_g1mt/app.py (1)
60-68: Consider using render() for simpler code.The current implementation chains setup methods (good!) but then calls individual draw methods. According to the PR description,
render()will draw all configured layers (structure, agents, propertylayers). Consider simplifying:renderer = ( SpaceRenderer(model, backend="altair") .setup_agents(agent_portrayal) .setup_propertylayer(propertylayer_portrayal) ) -renderer.draw_agents() -renderer.draw_propertylayer() +renderer.render() renderer.post_process = post_processThis makes the intent clearer: "render everything I've configured."
mesa/visualization/space_drawers.py (1)
85-156: Kwarg renaming in space drawers keeps behavior while aligning with renderer stateThe switch to
**draw_space_kwargs/**draw_chart_kwargsand the updatedpop/updatecalls in the Matplotlib and Altair drawers preserve existing behavior (figsize/dpi handling, node/edge styling, chart properties) while matching the newSpaceRenderer.draw_space_kwargs/draw_agent_kwargsnaming. The previous bug arounddraw_chart_kwargs.update(draw_chart_kwargs)is also resolved viachart_props.update(draw_chart_kwargs). No functional regressions spotted here.Also applies to: 266-330, 403-485, 543-585, 715-782
mesa/visualization/space_renderer.py (1)
173-217: Consider resetting cached meshes on setup_ to make repeated render() more predictable*Right now
render()only callsdraw_structure/draw_agents/draw_propertylayerwhen the corresponding*_meshisNone, and the altaircanvasre-draws only when those meshes are already set. If a caller does:
renderer.setup_agents(old_portrayal).render()- later
renderer.setup_agents(new_portrayal).render()the second
render()will skipdraw_agents()becauseagent_meshis already non-None, so the new configuration won’t be reflected unless the caller manually clearsagent_meshor callsdraw_agents()directly.To keep the cache but make the API safer, you could reset meshes in the setup methods, e.g.:
def setup_structure(self, **kwargs) -> SpaceRenderer: - self.draw_space_kwargs = kwargs + self.draw_space_kwargs = kwargs + self.space_mesh = None return self def setup_agents(self, agent_portrayal: Callable, **kwargs) -> SpaceRenderer: - self.agent_portrayal = agent_portrayal - self.draw_agent_kwargs = kwargs + self.agent_portrayal = agent_portrayal + self.draw_agent_kwargs = kwargs + self.agent_mesh = None return self def setup_propertylayer( self, propertylayer_portrayal: Callable | dict ) -> SpaceRenderer: - self.propertylayer_portrayal = propertylayer_portrayal + self.propertylayer_portrayal = propertylayer_portrayal + self.propertylayer_mesh = None return selfThis keeps caching semantics but ensures any new setup configuration is honored on the next
render()/canvasaccess without requiring callers to know about the internal*_meshfields.Also applies to: 219-239, 241-279, 280-368, 369-397, 417-423
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
mesa/examples/advanced/epstein_civil_violence/app.py(1 hunks)mesa/examples/advanced/pd_grid/app.py(1 hunks)mesa/examples/advanced/sugarscape_g1mt/app.py(1 hunks)mesa/examples/advanced/wolf_sheep/app.py(1 hunks)mesa/examples/basic/boid_flockers/app.py(1 hunks)mesa/examples/basic/boltzmann_wealth_model/app.py(1 hunks)mesa/examples/basic/conways_game_of_life/app.py(1 hunks)mesa/examples/basic/schelling/app.py(1 hunks)mesa/examples/basic/virus_on_network/app.py(1 hunks)mesa/visualization/solara_viz.py(2 hunks)mesa/visualization/space_drawers.py(21 hunks)mesa/visualization/space_renderer.py(6 hunks)tests/test_solara_viz_updated.py(3 hunks)tests/test_space_renderer.py(2 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/solara_viz.pymesa/examples/advanced/sugarscape_g1mt/app.pytests/test_solara_viz_updated.pymesa/examples/basic/virus_on_network/app.pymesa/visualization/space_drawers.pymesa/visualization/space_renderer.pytests/test_space_renderer.py
🔇 Additional comments (12)
mesa/examples/basic/schelling/app.py (1)
76-80: LGTM: Correct usage of the new fluent API.The example correctly demonstrates the new builder pattern: configure via
setup_agents(), then render viarender(). The updated comments clearly explain the behavior.tests/test_space_renderer.py (2)
120-122: LGTM: Test correctly validates the new API flow.The test properly demonstrates the fluent API pattern: chain setup methods, then call
render()to execute all configured draws. The assertions verify that all three draw methods are invoked.
141-143: LGTM: Good edge case validation.The test correctly validates error handling when property layers are missing, using the new setup API pattern.
mesa/examples/basic/conways_game_of_life/app.py (1)
58-62: LGTM: Clear intent with explanatory comment.The code correctly uses
draw_agents()individually, and the comment clearly explains the intent (drawing only agents, not the grid structure). This is a good example of when to use individual draw methods vsrender().mesa/examples/advanced/pd_grid/app.py (1)
48-52: LGTM: Clean usage of the fluent API.The code demonstrates the recommended pattern: configure via
setup_agents(), then render everything viarender(). The multi-line formatting enhances readability.mesa/examples/basic/boltzmann_wealth_model/app.py (1)
64-71: LGTM: Excellent demonstration of the fluent API.This example showcases the full capabilities of the new API:
- Chains
setup_structure()for grid customization- Chains
setup_agents()with additional drawing kwargs (cmap, vmin, vmax)- Completes with a single
render()callThe comments effectively explain the layered rendering approach, making this a strong reference example.
mesa/visualization/solara_viz.py (1)
273-277: Mesh checks already guard against uninitialized draws in this context.The code at lines 273-277 is protected: it checks whether each mesh attribute is set (non-
None) before calling the corresponding draw method. Sincespace_mesh,agent_mesh, andpropertylayer_meshare initialized toNoneand only populated after setup is complete, the guards prevent calling draw methods on uninitialized state.That said,
draw_agents()anddraw_structure()inspace_renderer.pylack explicit validation (unlikedraw_propertylayer(), which documents that it raises an exception if property layers are missing). If these methods are called directly by users without setup, they would silently operate on empty/default state rather than error clearly. Adding validation or raising informative exceptions in those methods would improve the API.mesa/examples/basic/boid_flockers/app.py (1)
73-80: Builder-style SpaceRenderer usage looks correctChained
SpaceRenderer(...).setup_agents(boid_draw).render()cleanly matches the new API and still leavesrendereras aSpaceRendererinstance forSolaraViz. No issues spotted here.mesa/examples/basic/virus_on_network/app.py (1)
111-121: Altair renderer setup chain is consistent with the new APIUsing
.setup_structure(node_kwargs=..., edge_kwargs=...).setup_agents(agent_portrayal)followed by a separaterenderer.render()fits the new stateful API and the NetworkSpaceDrawer’s expectation fornode_kwargs/edge_kwargs. Looks good.tests/test_solara_viz_updated.py (1)
150-155: Tests correctly exercise the new builder-style API and draw hooksThe updated
test_call_space_drawernow configures renderers viasetup_agents/setup_propertylayerand then calls.render(), while the spies still verify thatdraw_structure,draw_agents, anddraw_propertylayerare invoked for both matplotlib and altair backends, and not invoked when no renderer is supplied. This gives good coverage of the new API surface and the backward-compatible draw methods.Also applies to: 171-175, 199-205, 211-214, 221-224
mesa/examples/advanced/wolf_sheep/app.py (1)
84-90: Wolf–Sheep example now uses the intended setup/draw sequenceSwitching to
.setup_agents(wolf_sheep_portrayal)followed byrenderer.draw_agents()aligns this example with the new SpaceRenderer contract while keeping behavior equivalent to the previous version.mesa/visualization/space_renderer.py (1)
173-217: New setup_ API and deprecation paths look coherent and backward compatible*
setup_structure,setup_agents, andsetup_propertylayercleanly capture state for later drawing and support fluent chaining.draw_structure,draw_agents,draw_propertylayer, andrendernow accept the legacy signatures, emitDeprecationWarnings, and update the stored kwargs/portrayals before delegating to the backend, which addresses the earlier backward-compatibility concerns without breaking the new builder-style usage.- Dict-based
propertylayer_portrayalis still supported via_dict_to_callable, with a clear deprecation warning pointing to the migration guide.This structure matches the intended API evolution while preserving existing callers.
Also applies to: 219-368, 369-389
Sahil-Chhoker
added
example
EwoutH
left a comment
EwoutH
left a comment
There was a problem hiding this comment.
Thanks for the effort so far!
Doesn’t need to be this PR (it can), but before we deprecate this officially we should have a section in the migration guide and the deprecation messages should point to that section.
There was a problem hiding this comment.
- You are using a
PendingDerpecationWarning. I am not sure that is the best one to use. See the discussion on stack overflow, I personally favor using aFutureWarning(for all users of a library) these days, or alternatively aDeprecationWarning(for code that uses mesa from within__main__). - There are a few other changes here (e.g., the name change of kwargs in space_drawers, which technically were not needed and might cause trouble for users. Curious to know why you made the change. I am inclined to suggest reverting those changes.
- Have you tested all examples, or are you only relying on the unit tests for this? I just want to ensure that all examples continue to work correctly after this PR is merged.
the discussion also mentions:
and the term
I just wanted to make naming consistent in the backend, this is not user facing, so I don't think we should worry about that. Let me know if you feel otherwise.
I've tested all the examples by running them myself, they will work after this PR is merged if I haven't missed anything. |
|
I will try to write up some guidelines on how we deprecate stuff. Than we don’t have to discuss and reinvent that on every PR. |
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
Refactor
Tests
Examples
✏️ Tip: You can customize this high-level summary in your review settings.