Introduce Altair Grid #1902

ankitk50 · 2023-12-08T05:18:25Z

Initial implementation of Altair Grid
This is based on one of the implementation by @Corvince in mesa interactive.

tpike3 · 2023-12-08T12:10:16Z

@rlskoeser I know you have been doing some work with altair and Mesa, would you mind taking a look at this?

rlskoeser · 2023-12-08T12:27:48Z

@tpike3 thanks for tagging me, I'm interested to take a look. I probably won't be able to get to it until sometime next week, I hope that's fine. (I'm at the Computational Humanities Research conference and about to give a lightning talk on my simulating risk project!)

rlskoeser

It's nice to see some altair work going into jupyterviz, I hope my comments are helpful.

rlskoeser · 2023-12-15T14:36:42Z

mesa/experimental/altair_grid.py

+import mesa
+
+
+def get_agent_data_from_coord_iter(data):


In my current implementation, I'm using an agent_portrayal method to generate the values needed to draw the space. That may be drawn from the old mesa visualization approach, IDK if there's a good way to pass in something like that to jupyterviz.

I mention because I wonder if it would be cleaner and more explicit than the way you're using json to dump and filter the agent dict.

I agree with @rlskoeser; the JSON approach is not very clean. Is there a more explicit way to do this?

rlskoeser · 2023-12-15T14:44:46Z

mesa/experimental/altair_grid.py

+    def click_handler(datum):
+        if datum is None:
+            return
+        on_click(model, datum["x"], datum["y"])
+        update_data()
+
+    default_tooltip = [f"{key}:N" for key in data.value[0]]


nice that you have tooltip and click handlers - a little hard to assess without documentation, I would want to know how I can customize these

Yes please add docstrings to this explaining how to customize the tooltip

rlskoeser · 2023-12-15T14:46:49Z

mesa/experimental/altair_grid.py

+                "y:N",
+                scale=alt.Scale(domain=list(range(model.grid.height - 1, -1, -1))),
+            ),
+            color=color,


In one of my models where I'm using a custom altair space drawer, I'm setting color, size, and shape. Probably reasonable not to support all of those on the first pass, but it would be good to think about a more generalized approach (like the agent portrayal method) that would make it possible to customize this without having to completely re-implement.

I think this should be pretty straightforward to do with the create_grid function

EwoutH · 2023-12-17T20:41:36Z

@ankitk50 Looks very interesting, could you give the steps on how to run and test this?

ankitk50 · 2023-12-18T13:26:23Z

You can run the Boltzmann wealth model with space_drawer = "altair"

codecov · 2023-12-26T11:29:01Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (9495a5a) 77.53% compared to head (9dbb494) 78.13%.
Report is 46 commits behind head on main.

Files	Patch %	Lines
mesa/experimental/altair_grid.py	36.36%	18 Missing and 3 partials ⚠️
mesa/experimental/jupyter_viz.py	62.50%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1902      +/-   ##
==========================================
+ Coverage   77.53%   78.13%   +0.60%     
==========================================
  Files          15       16       +1     
  Lines        1015     1171     +156     
  Branches      221      257      +36     
==========================================
+ Hits          787      915     +128     
- Misses        197      220      +23     
- Partials       31       36       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EwoutH · 2023-12-29T11:37:00Z

So to run this:

Switch to your development branch
Copy the boltzmann_wealth_model to the root folder?
And then?

Sorry really haven't played with the visualization yet, but very curious.

(sidenote: We really should put back some basic examples in the main repo #1929)

ankitk50 · 2023-12-29T11:54:46Z

So to run this:

Switch to your development branch

Copy the boltzmann_wealth_model to the root folder?

And then?

Sorry really haven't played with the visualization yet, but very curious.

(sidenote: We really should put back some basic examples in the main repo #1929)

You need to go to the folder boltzmann_wealth_model_experimental and then run solara run app.py

EwoutH · 2023-12-29T12:13:44Z

Right, so for anyone else wanting to try the visualisation:

Switch to this development branch ankitk50:altair_viz

git remote add ankitk50 https://github.com/ankitk50/mesa.git
git fetch ankitk50
git checkout -b altair_viz ankitk50/altair_viz

Download or clone the mesa-examples repo and move the boltzmann_wealth_model_experimental folder to your
Add space_drawer="altair" to the JupyterViz() instantiation (line 33 of app.py)
Make sure you have solara installed (pip install -U solara)
Open your terminal, make sure your Python environment is activated (default in IDEs like PyCharm), move to the boltzmann_wealth_model_experimental directory and run solara run app.py

rht · 2024-01-06T00:52:26Z

@ankitk50: the Altair version of JupyterViz needs to have a feature parity with the Matplotlib version, and as such, needs the agent_portrayal for generality, as per @rlskoeser's feedbacks (you may read @rlskoeser's Altair space drawer usage). If you want the PR to be merged soon, you need to stay within a precise scope of what it consists of. I see your point in wanting to automatically retrieve all the agent's attributes in your current implementation, but it is out of scope, and we can discuss it in a separate thread.

I'm looking forward to have the Altair version to be the default soon.

Regarding with attribution, @rlskoeser are you OK with being a co-author in @ankitk50's Git commit in this PR, or would you prefer attribution to the code for a derivative work?

Corvince · 2024-01-06T11:10:13Z

Maybe I can address some of the points, since this PR is basically copy and pasted from my implementation in mesa-interactive.

The overall idea there is that the visualization just accepts a list of solara widgets*, with no special handling for the space drawer. This also means an agent portrayal is intentionally left out of the implementation. Instead a specialized create_grid function can be used. Everything specific to agent portrayal goes there. Currently it only supports color, but it's simple to extend to other properties (in mesa interactive I just haven't found the time to do so yet). If you need to specialize your visualization beyond what is included on the create_grid function you could just create your own Altair visualization or you use any other library and wrap it in a solara widget.

*It's a bit more complicated actually, since we also want to pass in model data. So you need a function or widget that accepts a model property and returns a widget (or in mesa-interactive wrap it in a static function).

I am not saying this approach is better, it's primarily something I wanted to explore as part of mesa-interactive. Again, unfortunately I hadn't had much time lately to develop it further and have no strong opinion on what is better. Just wanted to give some perspective on why this is implemented the way it is.

Btw @ankitk50 I am fine with you using the code here, but next time it would be a nice gesture to ask first or give credit. No hard feelings though

ankitk50 · 2024-01-06T11:35:26Z

Hi @Corvince sorry that I didn't mention use your code. Actually @EwoutH pointed me to use it in one of the discussions, thought you were aware. But nevertheless, should have done it.

rlskoeser · 2024-01-10T17:44:13Z

Regarding with attribution, @rlskoeser are you OK with being a co-author in @ankitk50's Git commit in this PR, or would you prefer attribution to the code for a derivative work?

Sorry for the delay in responding. Co-author commit seems fine here, thanks for asking.

EwoutH · 2024-01-16T10:21:19Z

What's the state of this PR? Do we want it in 2.2.1?

Since it's all in the experimental space it's okay if this break things, as long as that's somewhat documented.

ankitk50 · 2024-01-16T10:38:59Z

I would like to merge it, we can anyways iterate over the ideas and features.

rht · 2024-01-16T10:42:51Z

Since it's all in the experimental space it's okay if this break things, as long as that's somewhat documented.

It's definitely not okay to ignore people's feedback (and I have already spent the time to run the code in this PR) and unilaterally decide it's fine to merge.

EwoutH · 2024-01-16T10:51:49Z

I meant mainly from a SemVer perspective. What exact feedback still needs to be addressed?

rht · 2024-01-16T10:59:52Z

What exact feedback still needs to be addressed?

#1902 (comment)
#1902 (comment)
#1902 (comment)
#1902 (comment)

I'm not sure how to interpret regarding with ignoring all of those comments as if they don't exist.

In case you wonder about the experimental status of JupyterViz. While it is experimental, it doesn't deviate that much from the previous Tornado viz, and so users may find them more familiar, while mesa-interactive is more speculative. You can't just have 2 viz with conflicting API get merged messily just like that. I'm not sure why as a maintainer, I have to be the one to justify all the reasons why the code I implemented should be fine to be used.

Corvince · 2024-01-16T11:15:32Z

I agree with @rht that it would be very beneficial if you @ankitk50 could try to address those comments. Most of them are really more suggestions then actionable items, but some statement that you at least have these points on your radar would be nice. Currently, it indeed feels like they are just ignored and that just doesn't feel right. Personally I haven't reviewed this PR yet, because I was also somewhat waiting on what happens as a response to those comments.

Corvince · 2024-01-16T11:18:05Z

@ankitk50: the Altair version of JupyterViz needs to have a feature parity with the Matplotlib version, and as such, needs the agent_portrayal for generality, as per @rlskoeser's feedbacks

Again, I was waiting on a response to this, but to put forward the discussion, I don't think we need feature parity. The matplotlib version also hasn't feature parity with the tornado stuff, so why make it a requirement suddenly? The scope of this PR is to introduce an altair grid, at for that I think its sufficient.

rht · 2024-01-16T11:54:56Z

The matplotlib version also hasn't feature parity with the tornado stuff, so why make it a requirement suddenly?

The goal is to replace the Matplotlib version of JupyterViz with the Altair version. It shouldn't be that complicated to implement. It's fine if it doesn't have custom portrayal yet, but at the very least, the API needs to stay consistent. Can't have 3 API's (Tornado, Matplotlib JupyterViz, and this PR's Altair grid) that is going to be confusing for the users.
That said, the only missing features from the Matplotlib version are custom marker (e.g. the wolf-sheep PNG portrayals) and the hex grid. Everything else in the mesa-examples has already been covered.

ankitk50 · 2024-01-16T12:11:50Z

I would need to maybe align a bit more. Apologies for the confusion but the initial idea was to try Altair grid. The feature only works when you parse it via the agent portrayal. I do not have a complete understanding of the need for the parity. I get the idea that Altair would try to become default , but those changes could be incremental(if not orthogonal) on this PR.

mesa/experimental/altair_grid.py

tpike3 · 2024-01-17T10:48:08Z

@ankitk50 I piled on to @rlskoeser comments, if you could address those then we could merge, in general I would emphasize, please add docstrings and comments to make the code more readable.

EwoutH · 2024-01-21T21:47:50Z

@ankitk50 do you need anything from us to move this forward?

EwoutH · 2024-01-22T13:25:32Z

I finally have an IDE with Solara support again, so let me know if you need anything fixed, tested or build!

rlskoeser · 2024-01-22T14:48:10Z

Regarding with attribution, @rlskoeser are you OK with being a co-author in @ankitk50's Git commit in this PR, or would you prefer attribution to the code for a derivative work?

Sorry for the delay in responding. Co-author commit seems fine here, thanks for asking.

I know I said co-author commit previously, but my collaborator thinks that attribution to the code would be better recognition for our project - is that still feasible? Thank you!

rht · 2024-01-23T10:26:09Z

I added an attribution in #1991.

ankitk50 · 2024-02-09T13:11:41Z

Can someone merge this @rht @tpike3 @Corvince ?

rht · 2024-02-09T21:37:54Z

mesa/experimental/jupyter_viz.py

@@ -144,6 +149,13 @@ def render_in_browser():
                ModelController(model, play_interval, current_step, reset_counter)
            with solara.Card("Progress", margin=1, elevation=2):
                solara.Markdown(md_text=f"####Step - {current_step}")
+            with solara.Card("Analytics", margin=1, elevation=2):


This is unrelated to the PR.

rht · 2024-02-09T21:41:40Z

There is #1991, which is what I requested for the Altair grid to begin with, but wasn't addressed after so much time has passed, so I made my own PR.

introduce altair grid

a7a6c4b

tpike3 requested a review from Corvince December 8, 2023 12:10

ankitk50 added 2 commits December 15, 2023 11:19

add statistics tab

6236e64

add altair to depencency

7707d5b

rlskoeser reviewed Dec 15, 2023

View reviewed changes

ankitk50 added 2 commits December 23, 2023 10:31

Merge branch 'main' into altair_viz

0b3b091

update unittest

300e48c

ankitk50 added 2 commits December 26, 2023 13:21

add altrair to unittest

9b1ba7f

add fix for python 3.9/8

9dbb494

tpike3 mentioned this pull request Jan 2, 2024

update history for 2.2.0 release #1917

Closed

ankitk50 marked this pull request as ready for review January 5, 2024 12:00

EwoutH added feature Release notes label visualisation labels Jan 9, 2024

tpike3 reviewed Jan 17, 2024

View reviewed changes

mesa/experimental/altair_grid.py Outdated Show resolved Hide resolved

updates

9e0c00e

Corvince mentioned this pull request Jan 23, 2024

feat: Implement Altair version of grid visualization #1991

Merged

ankitk50 added 6 commits January 26, 2024 09:45

add docstring

a0ce9d9

resolve conflicts

593a9f3

fix grid

5182198

fix tests

3fe1838

rebase

aa52006

apply ruff

487dca3

rht reviewed Feb 9, 2024

View reviewed changes

Introduce Altair Grid #1902

Are you sure you want to change the base?

Introduce Altair Grid #1902

Conversation

ankitk50 commented Dec 8, 2023 • edited

tpike3 commented Dec 8, 2023

rlskoeser commented Dec 8, 2023

rlskoeser left a comment

Choose a reason for hiding this comment

rlskoeser Dec 15, 2023

Choose a reason for hiding this comment

tpike3 Jan 17, 2024

Choose a reason for hiding this comment

rlskoeser Dec 15, 2023

Choose a reason for hiding this comment

tpike3 Jan 17, 2024

Choose a reason for hiding this comment

rlskoeser Dec 15, 2023

Choose a reason for hiding this comment

tpike3 Jan 17, 2024

Choose a reason for hiding this comment

EwoutH commented Dec 17, 2023

ankitk50 commented Dec 18, 2023

codecov bot commented Dec 26, 2023 • edited

Codecov Report

EwoutH commented Dec 29, 2023 • edited

ankitk50 commented Dec 29, 2023

EwoutH commented Dec 29, 2023

rht commented Jan 6, 2024 • edited

Corvince commented Jan 6, 2024 • edited

ankitk50 commented Jan 6, 2024

rlskoeser commented Jan 10, 2024

EwoutH commented Jan 16, 2024

ankitk50 commented Jan 16, 2024

rht commented Jan 16, 2024

EwoutH commented Jan 16, 2024

rht commented Jan 16, 2024

Corvince commented Jan 16, 2024

Corvince commented Jan 16, 2024

rht commented Jan 16, 2024

ankitk50 commented Jan 16, 2024

tpike3 commented Jan 17, 2024

EwoutH commented Jan 21, 2024

EwoutH commented Jan 22, 2024

rlskoeser commented Jan 22, 2024

rht commented Jan 23, 2024

ankitk50 commented Feb 9, 2024 • edited

rht Feb 9, 2024

Choose a reason for hiding this comment

rht commented Feb 9, 2024

ankitk50 commented Dec 8, 2023 •

edited

codecov bot commented Dec 26, 2023 •

edited

EwoutH commented Dec 29, 2023 •

edited

rht commented Jan 6, 2024 •

edited

Corvince commented Jan 6, 2024 •

edited

ankitk50 commented Feb 9, 2024 •

edited