feat: Implement Altair version of grid visualization #1991

rht · 2024-01-23T01:53:23Z

Demo for Boltzmann wealth model:

output.mp4

github-actions · 2024-01-23T01:58:24Z

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%]

rht · 2024-01-23T02:00:44Z

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

rht · 2024-01-23T04:24:26Z

Fixed for multigrid, where now you may see overlapping markers:

EwoutH · 2024-01-23T09:07:47Z

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

Also curious what @Corvince and @ankitk50 think

mesa/experimental/components/altair.py

Corvince · 2024-01-23T10:03:56Z

Nice and simple solution, but the obvious question is how this relates to #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.

rht · 2024-01-23T10:25:19Z

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.

EwoutH · 2024-01-23T11:13:50Z

mesa/experimental/components/altair.py

+# https://github.com/Princeton-CDH/simulating-risk/blob/907c290e12c97b28aa9ce9c80ea7fc52a4f280ae/simulatingrisk/hawkdove/server.py#L114-L171
+# Copyright 2023 The Center for Digital Humanities of Princeton
+#
+# Licensed under the Apache License, Version 2.0 (the "License");


Adding this line requires a review from @jackiekazil @tpike3 @Corvince and maybe even a dev meeting with a vote. I’m totally fine with attribution, but now we’re changing our license to a mixed one.

It's technically a compatible license, with Mesa being Apache 2.0 as well, instead of GPLv3. This is how proper attribution of a code snippet should be, according to ChatGPT 3.5. I have only seen examples of attributing an entire file(s) in the past, so I haven't looked up for any precedence (for a subset of a file) yet.

What about when we update the code. After how much code change does that contribution still make sense? Would we get Ship of Theseus situations?

@rlskoeser I think attribution for your work is very important (@rht can confirm that, we recently discussed it). However, I think you should be credited for the current state of the code - the code that you actually wrote. The right place for that would be the commit message, and maybe the PR. The code is a living document, I don't think attribution there makes sense.

If you want full authorship it might be best to open a PR yourself, or make a clean nice commit we can cherry pick, giving you first authorship.

What we maybe could do additionally, is throw in a note in the next release notes. Release notes will remain static for eternity, and have (I think) way more exposure than the source code. You would be mentioned both on the GitHub Release pages as in the History, which is (or should be) also available on our official documentation (in Readthedocs). What would you think about that?

Thanks, all, for being so thoughtful about this.

I don't think I have capacity right now for a PR (unless there's something tightly scoped I can easily provide), and I think maybe the development has already moved beyond that anyway. (Apologies for the hidden axis thing in my code that tripped you up!)

An acknowledgement in the release notes sounds like a good solution to me.

I am switching to co-authorship in this PR for now.

Corvince · 2024-01-23T12:14:14Z

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)

rht · 2024-01-24T01:41:09Z

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 #1988 (comment) 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 · 2024-01-29T01:20:26Z

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.

_draw_grid is derived from https://github.com/Princeton-CDH/simulating-risk/blob/907c290e12c97b28aa9ce9c80ea7fc52a4f280ae/simulatingrisk/hawkdove/server.py#L114-L171 Co-authored-by: rlskoeser <rebecca.s.koeser@princeton.edu>

for more information, see https://pre-commit.ci

rht · 2024-02-08T08:33:18Z

New version of the video, with no more jitter.

output.mp4

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?

quaquel · 2024-02-08T08:38:29Z

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

rht · 2024-02-08T09:19:37Z

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

rht · 2024-02-22T07:53:58Z

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.

Corvince · 2024-02-22T08:02:28Z

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

quaquel · 2024-02-22T08:06:41Z

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.

rht · 2024-02-22T08:23:27Z

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 I already replied almost immediately on your question on Matrix, on Matrix, more than a week ago.

quaquel · 2024-02-22T09:01:00Z

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 (#1991 (comment)). 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. #1820 seems to suggest the former. it's meant to be a bugfix, where i have outlined a suggestion with a very simple fix (#1820 (comment)), 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?

rht · 2024-02-22T09:33:48Z

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?

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

quaquel · 2024-02-23T08:34:52Z

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?

Corvince

I think through the discussion here and on matrix it should hopefully be clear that we do value the other PR as well. But I think this PR is very clean and uncontroversial, so I am going to merge. Some ideas of#1902 can still be added later in a new pr

rht · 2024-02-28T01:39:10Z

Thank you for moving this forward. Finally.

rht force-pushed the solara_altair branch from e18180f to a83adc4 Compare January 23, 2024 01:56

rht force-pushed the solara_altair branch 2 times, most recently from dd64173 to bad03b0 Compare January 23, 2024 04:22

EwoutH requested a review from Corvince January 23, 2024 09:06

EwoutH added the visualisation label Jan 23, 2024

Corvince reviewed Jan 23, 2024

View reviewed changes

mesa/experimental/components/altair.py Outdated Show resolved Hide resolved

rht mentioned this pull request Jan 23, 2024

Introduce Altair Grid #1902

Open

rht force-pushed the solara_altair branch from bad03b0 to 5f83fda Compare January 23, 2024 10:30

EwoutH reviewed Jan 23, 2024

View reviewed changes

rht force-pushed the solara_altair branch 2 times, most recently from d5f0cb9 to bb34a7d Compare January 24, 2024 01:18

rht force-pushed the solara_altair branch 2 times, most recently from cedbaaa to 5e3900d Compare January 28, 2024 20:17

rht and others added 4 commits February 5, 2024 08:39

feat: Implement Altair version of grid visualization

1ba291a

_draw_grid is derived from https://github.com/Princeton-CDH/simulating-risk/blob/907c290e12c97b28aa9ce9c80ea7fc52a4f280ae/simulatingrisk/hawkdove/server.py#L114-L171 Co-authored-by: rlskoeser <rebecca.s.koeser@princeton.edu>

Make color and size optional

542f761

[pre-commit.ci] auto fixes from pre-commit.com hooks

14af7c8

for more information, see https://pre-commit.ci

Altair viz: Convert from DF -> alt.Data

1cbf501

rht force-pushed the solara_altair branch from ce4724e to 1cbf501 Compare February 5, 2024 13:39

pre-commit-ci bot and others added 2 commits February 5, 2024 13:39

[pre-commit.ci] auto fixes from pre-commit.com hooks

2fd4486

for more information, see https://pre-commit.ci

Altair viz: Hardcode width/height consistent with Card width/height

e5cbd6b

Corvince approved these changes Feb 24, 2024

View reviewed changes

Corvince merged commit 777f42e into projectmesa:main Feb 24, 2024
13 checks passed

rht deleted the solara_altair branch February 24, 2024 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement Altair version of grid visualization #1991

feat: Implement Altair version of grid visualization #1991

rht commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

rht commented Jan 23, 2024

rht commented Jan 23, 2024

EwoutH commented Jan 23, 2024

Corvince commented Jan 23, 2024

rht commented Jan 23, 2024 •

edited

Loading

EwoutH Jan 23, 2024

rht Jan 23, 2024 •

edited

Loading

EwoutH Jan 23, 2024

rlskoeser Jan 23, 2024

rht Jan 24, 2024

Corvince commented Jan 23, 2024

rht commented Jan 24, 2024 •

edited

Loading

rht commented Jan 29, 2024

rht commented Feb 8, 2024

quaquel commented Feb 8, 2024

rht commented Feb 8, 2024

rht commented Feb 22, 2024

Corvince commented Feb 22, 2024

quaquel commented Feb 22, 2024

rht commented Feb 22, 2024

quaquel commented Feb 22, 2024

rht commented Feb 22, 2024

quaquel commented Feb 23, 2024

Corvince left a comment

rht commented Feb 28, 2024

feat: Implement Altair version of grid visualization #1991

feat: Implement Altair version of grid visualization #1991

Conversation

rht commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

rht commented Jan 23, 2024

rht commented Jan 23, 2024

EwoutH commented Jan 23, 2024

Corvince commented Jan 23, 2024

rht commented Jan 23, 2024 • edited Loading

EwoutH Jan 23, 2024

Choose a reason for hiding this comment

rht Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

EwoutH Jan 23, 2024

Choose a reason for hiding this comment

rlskoeser Jan 23, 2024

Choose a reason for hiding this comment

rht Jan 24, 2024

Choose a reason for hiding this comment

Corvince commented Jan 23, 2024

rht commented Jan 24, 2024 • edited Loading

rht commented Jan 29, 2024

rht commented Feb 8, 2024

quaquel commented Feb 8, 2024

rht commented Feb 8, 2024

rht commented Feb 22, 2024

Corvince commented Feb 22, 2024

quaquel commented Feb 22, 2024

rht commented Feb 22, 2024

quaquel commented Feb 22, 2024

rht commented Feb 22, 2024

quaquel commented Feb 23, 2024

Corvince left a comment

Choose a reason for hiding this comment

rht commented Feb 28, 2024

rht commented Jan 23, 2024 •

edited

Loading

rht Jan 23, 2024 •

edited

Loading

rht commented Jan 24, 2024 •

edited

Loading