Fix color_patches SolaraViz portrayal + update README#346
Fix color_patches SolaraViz portrayal + update README#346Nandha-kumar-S wants to merge 5 commits intomesa:mainfrom
Conversation
|
hi @EwoutH Can you review this PR please? as this example's readme.md may mislead the users who try to run this model. I know that maintainers are busy, but hoping for a review when you get time :) |
|
Thanks @Nandha-kumar-S! This is definitely overdue. Could you please fix the parameters (heigh and width) they do not show up in the interface. If you have time could you also clean up some of the outdated comments like the line 66 in |
|
hi @tpike3 , i have updated the changes as requested. Please review them. Thanks |
|
Hi @tpike3 following up on the latest updates here. Ready for your review whenever you have a moment. Thanks |
|
@Sahil-Chhoker any chance you can give this a look? |
| return { | ||
| "color": _COLORS[cell.state], | ||
| "size": 100, | ||
| } | ||
|
|
||
|
|
||
| space_component = make_space_component( | ||
| color_patch_draw, | ||
| draw_grid=False, | ||
| ) |
There was a problem hiding this comment.
Why are we not using the lastest visualization API here?
There was a problem hiding this comment.
Hi @Sahil-Chhoker, thanks for the review! Since this PR was originally just focused on fixing the missing height/width parameters in the interface and cleaning up the outdated comments as requested by @tpike3, I kept the existing dictionary structure to keep the scope of these changes narrow.
While looking into this, I noticed that several other examples in the repository also still need to be updated to the latest AgentPortrayalStyle visualization API.
Would it be acceptable to get these immediate fixes merged in first so this example runs correctly for users? If so, I'll gladly open a new issue to track the API migration across the whole repository, assign it to myself, and submit a separate PR to update all the examples at once. I figure this will keep the commit history clean and tackle the deprecation globally.
Let me know if this works for you!
|
Thanks for stating your thought process @Nandha-kumar-S, let's go with that. I'll give a final review to this PR by tonight. |
|
Thanks @Sahil-Chhoker ! |
| @property | ||
| def grid(self): | ||
| """ | ||
| /mesa/visualization/modules/CanvasGridVisualization.py | ||
| is directly accessing Model.grid | ||
| 76 def render(self, model): | ||
| 77 grid_state = defaultdict(list) | ||
| ---> 78 for y in range(model.grid.height): | ||
| 79 for x in range(model.grid.width): | ||
| 80 cell_objects = model.grid.get_cell_list_contents([(x, y)]) | ||
|
|
||
| AttributeError: 'ColorPatches' object has no attribute 'grid' | ||
| """ | ||
| """Provide backward compatibility for accessing the grid.""" | ||
| return self._grid |
There was a problem hiding this comment.
Is there a specific reason for creating a separate grid property to access the _grid attribute of the model instead of just renaming _grid to grid?
I personally think latter one is more cleaner.
There was a problem hiding this comment.
Hi @Sahil-Chhoker, great catch! And that's much cleaner. I looked at the docstring and saw it was just a lingering workaround for CanvasGridVisualization backward compatibility. But now we're moving this to SolaraViz anyway, I've gone ahead and removed that legacy @Property block entirely and renamed self._grid to self.grid directly in model.py.
I just pushed the commit.
Let me know if everything looks good to go now! Thanks again for the review.
|
This PR is good to go from my side. @tpike3 can you give it a final review? |
4 participants
Summary
Updates the
color_patchesexample to run with Mesa 3.x SolaraViz and updtaed the README run instructions.Motive
The README referenced the legacy
mesa runserverflow, and the Solara app portrayal used CanvasGrid-style keys that are ignored in Mesa 3.x.Implementation
color,size).solara run app.pyand corrected the listed files.Usage Examples