mesa icon indicating copy to clipboard operation
mesa copied to clipboard

feat: Implement Altair version of grid visualization

Open rht opened this issue 1 year ago • 12 comments

Demo for Boltzmann wealth model:

https://github.com/projectmesa/mesa/assets/395821/a74aaf4f-1547-41f8-88f5-466500506317

rht avatar Jan 23 '24 01:01 rht

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 +2.1% [+1.8%, +2.3%] 🔵 +0.2% [+0.1%, +0.3%]
Schelling large 🔵 +1.8% [+1.3%, +2.3%] 🔵 -0.7% [-1.5%, -0.1%]
WolfSheep small 🔵 +0.1% [-0.3%, +0.5%] 🔵 -0.3% [-0.4%, -0.1%]
WolfSheep large 🔵 +2.7% [-1.2%, +7.9%] 🔵 +1.5% [-0.8%, +4.1%]
BoidFlockers small 🔵 -1.6% [-2.0%, -1.2%] 🔵 +0.4% [-0.2%, +0.9%]
BoidFlockers large 🔵 +0.1% [-0.5%, +0.5%] 🔵 +0.9% [+0.3%, +1.5%]

github-actions[bot] avatar Jan 23 '24 01:01 github-actions[bot]

The animation jitter could be fixed with a plt.tight_layout() equivalent of Altair. I just haven't figured it out yet.

rht avatar Jan 23 '24 02:01 rht

Fixed for multigrid, where now you may see overlapping markers: 2024-01-22T23:22:59,903471196-05:00

rht avatar Jan 23 '24 04:01 rht

Thanks for this effort, will try to review it later today.

Also curious what @Corvince and @ankitk50 think

EwoutH avatar Jan 23 '24 09:01 EwoutH

Nice and simple solution, but the obvious question is how this relates to https://github.com/projectmesa/mesa/pull/1902

This PR has a agent_portrayal function, but I don't see how it is used. Since altair uses a declarative approach vs an imperative in matplotlib, I think its confusing to have different agent_portrayal functions. But would have to see the agent_portrayal.

Otherwise drawing agents on the edge itself looks strange to me, but this shouldn't be a deciding factor.

Corvince avatar Jan 23 '24 10:01 Corvince

Since altair uses a declarative approach vs an imperative in matplotlib, I think its confusing to have different agent_portrayal functions. But would have to see the agent_portrayal.

I need to say that there is no modification needed in https://github.com/projectmesa/mesa-examples/blob/21624513c6ad94faabbb1cb4b97d73b897f9afae/examples/boltzmann_wealth_model_experimental/app.py#L5-L11, i.e. the agent_portrayal API is the same for both Matplotlib and Altair version. I just need to specify space_drawer="altair" in the JupyterViz initialization.

rht avatar Jan 23 '24 10:01 rht

Since altair uses a declarative approach vs an imperative in matplotlib, I think its confusing to have different agent_portrayal functions. But would have to see the agent_portrayal.

I need to say that there is no modification needed in https://github.com/projectmesa/mesa-examples/blob/21624513c6ad94faabbb1cb4b97d73b897f9afae/examples/boltzmann_wealth_model_experimental/app.py#L5-L11, i.e. the agent_portrayal API is the same for both Matplotlib and Altair version. I just need to specify space_drawer="altair" in the JupyterViz initialization.

Not exactly. For size this works as you described, but for color in matplotlib you set the actual colors (in your example tab:red and tab:blue ). In altair/vega you set the color encoding. So you are just saying give the value "tab:red" one color and the value "tab:blue" another one. The actual colors that are used are just the default categorical ones (lightblue and orange)

Corvince avatar Jan 23 '24 12:01 Corvince

I was about to switch the Altair viz color behavior to match Matplotlib (i.e. color means color, instead of category encoded in color), but decided against it, because the underlying code will look more opaque for the users to modify (edit: as it deviates from idiomatic Altair). As such, I'm leaving the code in this PR as is, to limit its scope. The scope is to have a feature parity with Matplotlib JupyterViz's _draw_grid as much as possible.

We need something like the 2nd point in https://github.com/projectmesa/mesa/pull/1988#issuecomment-1904528049 that @quaquel raised. This can be in the form of abstracting out the chart definition https://github.com/projectmesa/mesa/pull/1991/files#diff-681727772982fb0400d413fc9253bbc5e1af9b51d5a9d8581f2ab9d39a3cd516R39-R51 to a function that can be specified by the user.

rht avatar Jan 24 '24 01:01 rht

The animation jitter could be fixed with a plt.tight_layout() equivalent of Altair. I just haven't figured it out yet.

I fixed this by converting the data from DF to alt.Data.

rht avatar Jan 29 '24 01:01 rht

New version of the video, with no more jitter.

https://github.com/projectmesa/mesa/assets/395821/d0bbaefb-bfa8-43e4-b29e-684bf0ae396f

The only concern in this PR is that the agent_portrayal function differs from the Matplotlib version in that color is categorical instead of literal. I have chosen to keep the discrepancy, to adapt to Altair whenever it makes sense instead.

Are there any other concerns?

rht avatar Feb 08 '24 08:02 rht

I don't know why, but I can't view the movie.

quaquel avatar Feb 08 '24 08:02 quaquel

I have converted the movie output from .mp4 to .gif

output

rht avatar Feb 08 '24 09:02 rht

Can we move forward with this PR ASAP? I want to implement #2049 but for Altair and many other things. There is nothing controversial in this PR that warrants a delay in review for a month. And I'm not pleased with the slowdown in the supposedly experimental feature.

rht avatar Feb 22 '24 07:02 rht

Can we move forward with this PR ASAP? I want to implement #2049 but for Altair and many other things. There is nothing controversial in this PR that warrants a delay in review for a month. And I'm not pleased with the slowdown in the supposedly experimental feature.

I understand your frustration, but for me the main question is still and you haven't commented on this yet, how this relates to #1902 and how to deal with this situation, as I asked on Matrix. I think this is very controversial

Corvince avatar Feb 22 '24 08:02 Corvince

There have been various requests for modifications to the code in #1902. The last response from the author of that PR is from 2 weeks ago and in no way acknowledges the requested changes. I am sympathetic to therefore moving forward with this one instead.

quaquel avatar Feb 22 '24 08:02 quaquel

I understand your frustration, but for me the main question is still and you haven't commented on this yet, how this relates to https://github.com/projectmesa/mesa/pull/1902 and how to deal with this situation, as I asked on Matrix. I think this is very controversial

@Corvince I already replied almost immediately on your question on Matrix, on Matrix, more than a week ago.

rht avatar Feb 22 '24 08:02 rht

I really don't know what to do with the two competing jupyter PRs implementing the same feature. How can we resolve this without devaluing the work of one of the PRs?

I guess this is @Corvince's question. I looked through your responses @rht, but I don't see a clear answer to this specific question. You did write

in #1991 i attribute @rlskoeser because that's what it is mainly based on, and we had already made a fanfare about including her in the next mesa release (https://github.com/projectmesa/mesa/pull/1991#discussion_r1463378129). i can't exactly attribute the student (ankit) either, because his pr is mainly based on corvince's mesa-interactive, almost an exact copy. i'm not sure if ankit intentionally ignored my reviews or something else. https://github.com/projectmesa/mesa/pull/1820 seems to suggest the former. it's meant to be a bugfix, where i have outlined a suggestion with a very simple fix (https://github.com/projectmesa/mesa/pull/1820#issuecomment-1774838319), but hasn't been responded to for ages. if i shouldn't be frustrated about this, i don't know what else.

So, are you saying that your answer to @Corvince is to just merge this PR and close the other without any acknowledgement to the author of #1902?

quaquel avatar Feb 22 '24 09:02 quaquel

So, are you saying that your answer to @Corvince is to just merge this PR and close the other without any acknowledgement to the author of https://github.com/projectmesa/mesa/pull/1902?

Yes, mesa-interactive already exists in a separate repo, https://github.com/Corvince/mesa-interactive.

rht avatar Feb 22 '24 09:02 rht

I don't know the backstory to any of this. At face value, merging this PR without any acknowledgement to the author of #1902 seems a s bit harsh even though the author has not replied to various reasonable modifications to the code. What is wrong with a simple mention in the PR message itself of the work done in #1902?

quaquel avatar Feb 23 '24 08:02 quaquel

Thank you for moving this forward. Finally.

rht avatar Feb 28 '24 01:02 rht