breaking: Add dependencies argument to custom space_drawer

[rht]

Fixes #2208.

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
Schelling	small	🔵 -0.6% [-1.0%, -0.3%]	🔵 -0.3% [-0.6%, -0.0%]
Schelling	large	🔵 +0.2% [-0.6%, +0.9%]	🔵 -0.5% [-4.1%, +2.8%]
WolfSheep	small	🔵 -0.1% [-1.3%, +1.0%]	🔵 -0.1% [-0.4%, +0.3%]
WolfSheep	large	🔵 -0.9% [-1.2%, -0.5%]	🔵 -0.4% [-1.5%, +1.0%]
BoidFlockers	small	🔵 +0.6% [-0.1%, +1.3%]	🔵 +0.1% [-0.7%, +0.9%]
BoidFlockers	large	🔵 +1.4% [+1.0%, +1.9%]	🔵 +0.0% [-0.5%, +0.6%]

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
Schelling	small	🔵 +1.3% [+0.8%, +1.7%]	🔵 +0.6% [+0.2%, +1.0%]
Schelling	large	🔵 +1.4% [+0.2%, +2.5%]	🔵 +4.7% [-0.0%, +9.4%]
WolfSheep	small	🔵 +1.4% [+0.2%, +2.5%]	🔵 +1.2% [+0.9%, +1.5%]
WolfSheep	large	🔵 +2.9% [+2.1%, +3.7%]	🔵 +5.2% [+2.3%, +8.1%]
BoidFlockers	small	🔵 +1.7% [+1.1%, +2.3%]	🔵 -0.1% [-0.9%, +0.9%]
BoidFlockers	large	🔵 +2.2% [+1.7%, +2.8%]	🔵 -0.9% [-1.4%, -0.4%]

[EwoutH]

Can you briefly explain in the PR start what the issue was, how it was caused and how this fixes it?

[rht]

It's described in #2208, and so it's redundant to say it again.

[EwoutH]

No it's not. Anyone can stumble onto this PR with a git blame, a mention in the changelog or in any other way. Then the issue needs to be clearly described, as well as why it broke in the first place and how this fixes the issue. And most importantly, what the user needs to do or doesn't do.

Documentation is important.

Especially with a breaking PR.

[DrEntropy]

The issue I am having (and posted in #2208 ) is that when using a custom space drawer (in my case, I want to draw a network but with a different layout) the display is not reactive to changes that happen during a step. It is reactive to changes in model_params passed to SolaraViz but not to any other changes. My temporary fix was to (yuck!) introduce a solara.reactive counter in the model and pass that as a dependency in my custom drawer's call to solara.FigureMatplotlib. This works but is ugly.

It seems to this newcommer to Solara that it was a simple oversite not to pass in the dependencies in the call to space_drawer in solara_viz.py for custom space_drawers. However, it is of course quite probable that I am just doing this wrong.

The fix proposed here does resolve this issue. However, I note that in my testing, with this fix, the graph drawing is no longer reactive to changes in model_params. This is also the case without this change but using the default space drawer. This is probably a separate issue.

BTW I should mention that this is all using the server based Solara (solara run app.py) not in a notebook.
EDIT: If the devs want I can open this as an issue for improved tracking. I think I can create a repro using the Virus example as well, but hopefully this is clear enough.

[EwoutH]

Thanks a lot for the write up and for reporting this issue in the first place!

I’m not familiar with the SolaraViz module to review this PR myself. I will leave a review to one of the other @projectmesa/maintainers.

[EwoutH]

Could we implement this PR in a way that it's not breaking? And pass a warning if space_drawer isn't passed (if we want to require that in the future)

However, I note that in my testing, with this fix, the graph drawing is no longer reactive to changes in model_params. This is also the case without this change but using the default space drawer. This is probably a separate issue.

Could you clarify if this PR introduces this as a new issue, or that this is an existing issue also present in the current Solara implementation?

[DrEntropy]

Could we implement this PR in a way that it's not breaking? And pass a warning if space_drawer isn't passed (if we want to require that in the future)

space_drawer is optional (there is a default implementation). The change is to pass a user space_drawer a third argument ('dependencies') just like the default function takes. I suppose one could make it non-breaking by using inspect.Signature to see if the user passed in a 2 argument or 3 argument function? Or is there some other way this is a breaking change?

Could you clarify if this PR introduces this as a new issue, or that this is an existing issue also present in the current Solara implementation?

It seems to be an existing issue. The default space drawer for graphs also doesn't respond to changes in the number of nodes (but does respond to other changes). I will verify / investigate this and open a new issue if it is true.

[EwoutH]

Okay tell me if I understand it correctly now:

This is quite an edge-case, in the situation space_drawer is passed but dependencies is not, when defining a Card(). But in that case space_drawer doesn't update fully/correctly (right?), because it needs dependencies to be passed.

This PR fixes this, and the only breaking scenario is when space_drawer is passed but dependencies is not, but it still accidentally behaves like the user wants to.

Am I now fully correct?

[tpike3]

Okay tell me if I understand it correctly now:

This is quite an edge-case, in the situation space_drawer is passed but dependencies is not, when defining a Card(). But in that case space_drawer doesn't update fully/correctly (right?), because it needs dependencies to be passed.

This PR fixes this, and the only breaking scenario is when space_drawer is passed but dependencies is not, but it still accidentally behaves like the user wants to.

Am I now fully correct?

I think so but I would explain it a little differently, so I believe we are saying the same thing.

Card requires the parameter dependencies. From their the main visualization (agents-grid, network etc) gives three options
1 - Plot in Matplotlib (default)
2 - Plot in Altair
3 - Define your own plotting approach.

The challenge is dependencies isn't passed to option three which is the parameters, step value etc. This impact resulted @DrEntropy' issue because he needed those parameters for his visual. Adding them will break any model that used option 3, as their custom space_drawer model will not expect the dependencies parameter.

Hope this makes sense/helps. I think to your point this will impact a small minority of users and @DrEntropy may be the first one.

[tpike3]

LGTM

[EwoutH]

Thanks! Could you give a two or three sentence note that I can include in the release notes for the next Mesa 3.0 alpha release?