Refactor Agent Portrayal to Use AgentPortrayalStyle Instead of Dictionary(#2436) #2728
Conversation
|
Performance benchmarks:
|
aarav-shukla07
force-pushed
the
agent_potrayal
branch
from
60a7add to
3e8dc29
Compare
for more information, see https://pre-commit.ci
|
@aarav-shukla07 thanks for your PR! An |
|
I like the basic idea and resulting API. However, I have some concerns about the implementation and the performance overhead that would require some custom benchmarking.
|
|
@aarav-shukla07, The code in |
Hi @quaquel , Thank you for your detailed feedback! Here’s how I’ve addressed your concerns: (1) Converted AgentPortrayalStyle into a dataclass This encapsulates all visual encoding in a single object, improving readability and structure. (2) Optimized collect_agent_data Previously, it iterated over agents twice. Now, it processes them in a single loop, reducing unnecessary overhead. (3) Added loc directly to AgentPortrayalStyle This ensures that all relevant visual properties, including position, are encapsulated within a single structured object. I’ve also verified that all tests pass , and I’ve resolved the merge conflicts to ensure smooth integration. Please let me know if there are any further improvements or clarifications needed! Thanks again for your guidance. Looking forward to your feedback. |
|
Hi @Sahil-Chhoker , Thank you for your feedback. I acknowledge that the code in I have updated the PR and verified that the changes align with the latest codebase. Please let me know if there are any further improvements or clarification needed! Thanks again for your guidance. Looking forward to your feedback. |
| edgecolors: str = "black" # edge color for markers | ||
| loc: tuple[float, float] | None = None # stores agent position | ||
|
|
||
| def to_dict(self): |
There was a problem hiding this comment.
I would suggest doing it the other way arround. In get_agent_data if the return is a dict cast it to a AgentPortrayalStyle and issue a warning
|
|
||
| def MakeSpaceMatplotlib(model): | ||
| def wrapped_agent_portrayal(agent): |
There was a problem hiding this comment.
can't this be handled inside get_agent_data?
| warnings.warn( | ||
| f"the following fields are not used in agent portrayal and thus ignored: {msg}.", | ||
| stacklevel=2, | ||
| portrayal_dict = agent_portrayal(agent) or {} |
| portrayal_dict = agent_portrayal(agent) or {} | ||
|
|
||
| agent_data_list.append( | ||
| AgentPortrayalStyle( |
There was a problem hiding this comment.
This is wasteful because in the current code you already allways get an AgentPortrayalStyle instance.
| alpha=portrayal_dict.get("alpha", 1.0), | ||
| linewidths=portrayal_dict.get("linewidths", 1.0), | ||
| edgecolors=portrayal_dict.get("edgecolors", "black"), | ||
| loc=agent.pos if agent.pos is not None else (0, 0), # Store location |
There was a problem hiding this comment.
this is not correct.
loc is either agent.pos or agent.cell.coordinate
| @@ -346,7 +308,9 @@ def draw_orthogonal_grid( | |||
|
|
|||
| # gather agent data | |||
| s_default = (180 / max(space.width, space.height)) ** 2 | |||
| arguments = collect_agent_data(space, agent_portrayal, size=s_default) | |||
| arguments = collect_agent_data( | |||
| space, agent_portrayal, default_color="tab:red", default_size=30 | |||
There was a problem hiding this comment.
these defaults can be moved into AgentPortrayalStyle
| # Calculate hexagon centers for agents if agents are present and plot them. | ||
| if loc.size > 0: | ||
| loc[:, 0] = loc[:, 0] * x_spacing + ((loc[:, 1] - 1) % 2) * (x_spacing / 2) | ||
| loc[:, 1] = loc[:, 1] * y_spacing | ||
| arguments["loc"] = loc | ||
| for i, agent_data in enumerate(arguments): |
There was a problem hiding this comment.
Previously, arguments was structured as a dictionary holding arrays for all agents — so we could assign the entire loc array at once using arguments["loc"] = loc.
With the updated structure, arguments is now a list of dictionaries, each representing an individual agent’s data. To assign each agent its own specific location, we iterate through arguments and update each ```agent_data entry:
for i, agent_data in enumerate(arguments):
agent_data["loc"] = loc[i]This change ensures that each agent dictionary gets its corresponding location from the loc array, maintaining alignment between spatial data and individual agent portrayal.
| arr[:] = arguments["marker"] | ||
| data["marker"] = arr | ||
| return data | ||
| return agent_data_list |
There was a problem hiding this comment.
not sure but it might be better to turn all agent data into numpy arrays already here so we avoid needless iterations later.
| scatter_data = { | ||
| "s": [a.size for a in agent_data_list], | ||
| "c": [a.color for a in agent_data_list], | ||
| "marker": [a.marker for a in agent_data_list], | ||
| "zorder": [a.zorder for a in agent_data_list], | ||
| "loc": np.array([a.loc for a in agent_data_list], dtype=np.float64), | ||
| "alpha": [a.alpha for a in agent_data_list], | ||
| "edgecolors": [a.edgecolors for a in agent_data_list], | ||
| "linewidths": [a.linewidths for a in agent_data_list], | ||
| } |
There was a problem hiding this comment.
this is wasteful iteration. You iterate over the same data 8 times.
| marker = [agent_data.marker for agent_data in arguments] | ||
| zorder = [agent_data.zorder for agent_data in arguments] | ||
| edgecolors = [agent_data.edgecolors for agent_data in arguments] | ||
| linewidths = [agent_data.linewidths for agent_data in arguments] | ||
| alpha = [agent_data.alpha for agent_data in arguments] |
There was a problem hiding this comment.
same here, 5 repeated iterations
|
@Sahil-Chhoker / @sanika-n, could (one of) you review this PR? |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
|
Hi @quaquel , Thank you for your valuable feedback!
All tests are now passing locally after resolving the merge conflict. Please let me know if you’d prefer further optimizations (e.g., using structured arrays or different handling of defaults). Looking forward to your thoughts! |
Really sorry again, I am travelling most of the next week (until 10th) so I won't be able to find time to review this PR, @sanika-n is it alright if I leave the review to you? |
|
I am currently tied up with my proposal, but I can take this up after 8th |
aarav-shukla07
force-pushed
the
agent_potrayal
branch
from
cdcc6d1 to
939c8b6
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Hey @aarav-shukla07, I took a little look at this PR, so to just start the review can you please start by incorporating quaquel's review? Also I see that the previous docstrings are all missing, I don't see any need to remove them, so fix that as well. And at last, can you fix the tests so that they actually pass?
|
Also, could you pls update the tests/test_components_matplotlib.py file as well? That still uses a dictionary for agent-portrayal, you can find the code here |
|
Hello @Sahil-Chhoker @sanika-n |
Summary
This PR refactors Mesa’s visualization system by replacing the dictionary-based agent_portrayal system with a new AgentPortrayalStyle class. This improves code clarity, ensures validation, and enhances backend compatibility across different visualization methods
Bug / Issue
Closes #2436
Previously, agent_portrayal functions returned a dictionary with properties like "color", "marker", "size", and "zorder". However, this approach:
This PR introduces AgentPortrayalStyle, making agent portrayal more structured and type-safe while maintaining backward compatibility.
Implementation
Testing
Run all Mesa tests to ensure no regressions:
Verified that test_call_space_drawer correctly handles AgentPortrayalStyle
Result: All tests passed with no new failures.
Additional Notes