Repository: projectmesa/mesa
Pull request #2209 (2024-08-14 09:09:27)

breaking: Add dependencies argument to custom space_drawer
Fixes https://github.com/projectmesa/mesa/discussions/2208.

github-actions[bot]: 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[bot]: 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.
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 in 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.   


Patch:
diff --git a/mesa/visualization/solara_viz.py b/mesa/visualization/solara_viz.py
index 4f159884..fba73207 100644
--- a/mesa/visualization/solara_viz.py
+++ b/mesa/visualization/solara_viz.py
@@ -70,7 +70,7 @@ def Card(
                 )
             elif space_drawer:
                 # if specified, draw agent space with an alternate renderer
-                space_drawer(model, agent_portrayal)
+                space_drawer(model, agent_portrayal, dependencies=dependencies)
         elif "Measure" in layout_type:
             rv.CardTitle(children=["Measure"])
             measure = measures[layout_type["Measure"]]


Test patch:
diff --git a/tests/test_solara_viz.py b/tests/test_solara_viz.py
index f3422b87..5fef7e0b 100644
--- a/tests/test_solara_viz.py
+++ b/tests/test_solara_viz.py
@@ -136,7 +136,9 @@ def test_call_space_drawer(mocker):
             space_drawer=altspace_drawer,
         )
     )
-    altspace_drawer.assert_called_with(model, agent_portrayal)
+    altspace_drawer.assert_called_with(
+        model, agent_portrayal, dependencies=dependencies
+    )
 
 
 def test_slider():