Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

solara_viz: Add borders around ContinuousSpace #1988

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Jan 22, 2024

For all intents and purposes, this should work. However, on my systems, I can't get it to visualise

It should looks something like this:

Screenshot_355

But in reality, on my system, it keeps looking like this:

Screenshot_356

I can add a title for some reason, but can't add borders or a background color.

Could it be that the background color and spines are overwritten later? @rht @quaquel @ankitk50, if one of you can explain or find the source of this madness, please.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 -0.2% [-0.4%, -0.0%] 🔵 +0.2% [-0.0%, +0.4%]
Schelling large 🔵 -0.7% [-1.2%, -0.2%] 🔵 -0.9% [-1.8%, -0.1%]
WolfSheep small 🔵 -0.5% [-0.9%, -0.0%] 🔵 -0.6% [-0.8%, -0.5%]
WolfSheep large 🔵 +3.6% [+0.6%, +6.2%] 🔵 +4.2% [+2.6%, +6.2%]
BoidFlockers small 🔵 -0.2% [-0.7%, +0.3%] 🔵 +1.0% [+0.5%, +1.4%]
BoidFlockers large 🔵 +0.4% [-0.1%, +0.9%] 🔵 -0.4% [-0.8%, +0.0%]

@rht
Copy link
Contributor

rht commented Jan 22, 2024

This line is the cause:

space_ax.set_axis_off()
.

@EwoutH
Copy link
Member Author

EwoutH commented Jan 22, 2024

That's 45 minutes of my life I will never get back.

Now looks like this. Dotted line is for torus=True (when false it's solid). What does everyone think, is this better?

Screenshot_358

@EwoutH EwoutH added the experimental Release notes label label Jan 22, 2024
@EwoutH EwoutH requested review from rht and Corvince January 22, 2024 15:48
@EwoutH
Copy link
Member Author

EwoutH commented Jan 22, 2024

@quaquel @ankitk50 also please let me know what you think!

@EwoutH EwoutH marked this pull request as ready for review January 22, 2024 15:48
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 -2.1% [-2.5%, -1.8%] 🔵 -0.4% [-0.5%, -0.2%]
Schelling large 🔵 -1.9% [-2.2%, -1.7%] 🔵 -3.3% [-5.4%, -1.4%]
WolfSheep small 🟢 -3.6% [-4.1%, -3.1%] 🔵 -0.1% [-0.3%, +0.0%]
WolfSheep large 🟢 -6.4% [-9.2%, -3.7%] 🟢 -10.5% [-12.1%, -8.7%]
BoidFlockers small 🔵 -3.0% [-3.4%, -2.7%] 🔵 +0.1% [-0.7%, +0.9%]
BoidFlockers large 🔵 -2.8% [-3.4%, -2.3%] 🔵 +0.8% [+0.3%, +1.4%]

@EwoutH
Copy link
Member Author

EwoutH commented Jan 22, 2024

While I think these are good defaults, maybe we should allow passing the background color and turning the borders on or off.

@quaquel
Copy link
Member

quaquel commented Jan 22, 2024

I started looking at the current code last week when I ran into the bug fixed this morning via #1984. I want to return to this at some point in the next few weeks. I see two options

  1. Handle it like it is done in matplotlib and seaborne where you have various dicts that you can pass that control the behavior in detail.
  2. Make it much easier to write your own plotting function that you pass to jupyterviz. Basically, specify the interface (so (keyword) arguments with which the function will be called and what the function should return. Ideally, you would model this on matplotlib.animation. So you have an init function and a function called to update the plot. I, however, don't know how easy that would be in Solara.

In the long run, I believe that the second option is much more flexible and easier to use. For example, in my exam model, I want to plot a background 2d KDE and have my agents on top of this. It is impossible to do this via (1) but easy to support via (2). In fact, I would only need to plot the KDE once in the init and only have to update the location of the agents while running the model.

@rht
Copy link
Contributor

rht commented Jan 22, 2024

Now looks like this. Dotted line is for torus=True (when false it's solid). What does everyone think, is this better?

The dotted/solid line SGTM. The background grey color is debatable, but if you must provide a background color:

  1. I'd prefer Seaborn's shade of grey. Because it is what everyone who use ggplot2-based libraries are used to.
  2. Needs a way for users to easily change the background color.

@EwoutH
Copy link
Member Author

EwoutH commented Jan 22, 2024

Shall I do only the line for now in this PR? And then we can discuss the other stuff later?

@rht
Copy link
Contributor

rht commented Jan 22, 2024

Shall I do only the line for now in this PR? And then we can discuss the other stuff later?

SGTM

@rht
Copy link
Contributor

rht commented Jan 22, 2024

Handle it like it is done in matplotlib and seaborne where you have various dicts that you can pass that control the behavior in detail.

See also #1797. I checked that Seaborn doesn't have its own Axes.set API. And you'd have to reuse Matplotlib's Axes.set for a declarative axes configuration.

Make it much easier to write your own plotting function that you pass to jupyterviz. Basically, specify the interface (so (keyword) arguments with which the function will be called and what the function should return. Ideally, you would model this on matplotlib.animation. So you have an init function and a function called to update the plot. I, however, don't know how easy that would be in Solara.

See also #885. The solution in this issue is blocked from being merged because we are considering of migrating to Altair instead of Matplotlib. However, we should implement both Matplotlib and Altair viz in parallel: #1849 (reply in thread), and to make sure they have as similar API as possible.

@EwoutH
Copy link
Member Author

EwoutH commented Jan 22, 2024

@rht go ahead for final review and squash and merge.

@quaquel Do you want a 2.2.3 release, and if so, should this PR be included?

@EwoutH EwoutH changed the title solara_viz: Add grey background and borders solara_viz: Add borders around ContinuousSpace Jan 22, 2024
@quaquel
Copy link
Member

quaquel commented Jan 22, 2024

So 2.2.3 would include the view limits and this? It would be nice for teaching and the exam if it all works. I will still make the exam to also work on older versions because students...

@rht rht merged commit c1fae84 into projectmesa:main Jan 22, 2024
12 of 13 checks passed
EwoutH added a commit that referenced this pull request Jan 22, 2024
* solara_viz: Add grey background and borders

* solara_viz: Render axis

* solara_viz: Better dash

Use a 'loosely dashed' line for torus=True

See https://matplotlib.org/stable/gallery/lines_bars_and_markers/linestyles.html

* solara_viz: Remove background color
@EwoutH
Copy link
Member Author

EwoutH commented Jan 22, 2024

So 2.2.3 would include the view limits and this? It would be nice for teaching and the exam if it all works.

Ever got a release from an open-source release custom made for you, within a day? There you go!

I will still make the exam to also work on older versions because students...

Just add at the top of the notebook:

import mesa
if mesa.__version__ != "2.2.3"
    %pip install -U mesa==2.2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants