Repository: projectmesa/mesa
Pull request #1783 (2023-08-30 18:16:37)

Add an option to disable drawing agent space #1778
as discussed on #1778 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/projectmesa/mesa/pull/1783?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) Report
Patch coverage has no change and project coverage change: **`-0.19%`** :warning:
> Comparison is base [(`79bde5a`)](https://app.codecov.io/gh/projectmesa/mesa/commit/79bde5a259cfab4adbbfb2a9bd0bb4219af2713c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) 80.02% compared to head [(`a86f51c`)](https://app.codecov.io/gh/projectmesa/mesa/pull/1783?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) 79.84%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
- Coverage   80.02%   79.84%   -0.19%     
==========================================
  Files          15       15              
  Lines         881      883       +2     
  Branches      188      189       +1     
==========================================
  Hits          705      705              
- Misses        155      157       +2     
  Partials       21       21              
```


| [Files Changed](https://app.codecov.io/gh/projectmesa/mesa/pull/1783?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa) | Coverage Δ | |
|---|---|---|
| [mesa/experimental/jupyter\_viz.py](https://app.codecov.io/gh/projectmesa/mesa/pull/1783?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa#diff-bWVzYS9leHBlcmltZW50YWwvanVweXRlcl92aXoucHk=) | `19.70% <0.00%> (-0.30%)` | :arrow_down: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/projectmesa/mesa/pull/1783?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=projectmesa).

rht: I have seen `None` and `False` being used as distinct possible values in a parameter in a library somewhere. Can't remember which one. But it's easy to confuse the 2, because `None` is considered falsy. I am not fully sure yet of the best way forward in this situation.
rht: Maybe a string `"disabled"`? @Corvince thoughts?
Corvince: I think space_drawer=None reads pretty good and should be unambiguous.

That said I think we shouldn't assume what and how people want to plot there models.

I think it would be easier to let users just decide on which solara components they want to display (and provide some components) 
rht: > I think it would be easier to let users just decide on which solara components they want to display (and provide some components)

The reason why I set the space drawing to be automatic was to cover the 80% of the use case to be easy. The previous Tornado version does require users to specify manually, but I find this too much boilerplate code.
rlskoeser: I was thinking of `None` as the default / unspecified option, but it's plausible that someone would specify `specify_drawer=None` and expect to have no space and I agree that the code isn't as readable.

I was wondering if there was a different way to pass in the default `make_space` method instead of using `None` to flag the default, but when I tried that locally I couldn't pass through the `False` / `None` value; maybe because there's something I'm missing about how the `JupyterViz` method generates a `MesaComponent`.

I thought about a defined constant for either the default or the disabled state, but adding an optional kwarg parameter to disable drawing the space as @tpike3 suggested sounds easier to use and more readable to me.  Although I'm not sure of a brief, readable name for that argument.
rht: What about passing a do-nothing `lambda: None` function and document this in the docstring?
rht: As in, if user doesn't want the space to be drawn, they should pass in the empty lambda function.
rht: Another option is to set `space_drawer=make_space` as the default argument, and if user specifies it with a falsy value (`None`, `False`, 0, etc), then don't draw anything.
rlskoeser: > Another option is to set `space_drawer=make_space` as the default argument, and if user specifies it with a falsy value (`None`, `False`, 0, etc), then don't draw anything.

I tried this and it didn't work (the `None` turned into default behavior somewhere - I think related to the component creation). I will try again with the refactored jupyterviz code and see if I can get it to work. This seems like the cleanest option to me.
rht: You could also do `space_drawer="default"` and then replace is with `make_space`, and do nothing for falsy values.
tpike3: I don't want to stand in the way of merging this as it is an unequivocally an improvement;

The doc build issue was fixed with #1785 

But as I opened a can of worms, any issue merging @rht, @Corvince, @jackiekazil ?  
rht: >  But it's easy to confuse the 2, because None is considered falsy.

I stand by the statement of the problem of using `None` and `False` in a different way, so this PR can't be merged yet.
tpike3: > You could also do `space_drawer="default"` and then replace is with `make_space`, and do nothing for falsy values.

I am good with this solution
Corvince: > > You could also do `space_drawer="default"` and then replace is with `make_space`, and do nothing for falsy values.
> 
> I am good with this solution

me too
tpike3: @rlskoeser This is your PR, so you are lead, let us know your thoughts or if there is any support we can provide.
jackiekazil: > >>  You could also do `space_drawer="default"` and then replace is with `make_space`, and do nothing for falsy values.
>>  
> > I am good with this solution

> me too

+1
rlskoeser: Thanks for the input, everyone. I've re-implemented based on the latest refactor of the jupyter viz code with the proposed 'default' option, which does seem to be the simplest solution. I added a docstring for `JupyterViz` (although not sure my description of the `measures` parameter is accurate/helpful) and a minimal unit test that confirms the expected behavior for various options for drawing the space (default, custom, none).
rht: The `ruff` lint failed.
tpike3: @rlskoeser This is great and thanks for the tests! I think 2 commits is good, one for the improvement and one for the tests, but please squash the lint correction

Again thank you very much!

rht: It's fine to not squash the commits. We can do "squash and merge" later instead.
rlskoeser: Sorry I missed the linting error! I guess I've gotten too used to the isort pre-commit hook that I've started using on other projects.

Corrected now, and I amended & squashed the commit history.
jackiekazil: I am excited about this addition. Thank you for your contribution.  <3 
I haven't had a chance to test, so I will defer to someone else to make the final review/merge.

Patch:
diff --git a/mesa/experimental/jupyter_viz.py b/mesa/experimental/jupyter_viz.py
index 6007b9a8..93b0d852 100644
--- a/mesa/experimental/jupyter_viz.py
+++ b/mesa/experimental/jupyter_viz.py
@@ -20,9 +20,23 @@ def JupyterViz(
     measures=None,
     name="Mesa Model",
     agent_portrayal=None,
-    space_drawer=None,
+    space_drawer="default",
     play_interval=400,
 ):
+    """Initialize a component to visualize a model.
+    Args:
+        model_class: class of the model to instantiate
+        model_params: parameters for initializing the model
+        measures: list of callables or data attributes to plot
+        name: name for display
+        agent_portrayal: options for rendering agents (dictionary)
+        space_drawer: method to render the agent space for
+            the model; default implementation is :meth:`make_space`;
+            simulations with no space to visualize should
+            specify `space_drawer=False`
+        play_interval: play interval (default: 400)
+    """
+
     current_step, set_current_step = solara.use_state(0)
 
     solara.Markdown(name)
@@ -48,10 +62,14 @@ def JupyterViz(
 
     with solara.GridFixed(columns=2):
         # 4. Space
-        if space_drawer is None:
+        if space_drawer == "default":
+            # draw with the default implementation
             make_space(model, agent_portrayal)
-        else:
+        elif space_drawer:
+            # if specified, draw agent space with an alternate renderer
             space_drawer(model, agent_portrayal)
+        # otherwise, do nothing (do not draw space)
+
         # 5. Plots
         for measure in measures:
             if callable(measure):


Test patch:
diff --git a/tests/test_jupyter_viz.py b/tests/test_jupyter_viz.py
index b2e701df..4e26d569 100644
--- a/tests/test_jupyter_viz.py
+++ b/tests/test_jupyter_viz.py
@@ -1,7 +1,9 @@
 import unittest
-from unittest.mock import patch
+from unittest.mock import Mock, patch
 
-from mesa.experimental.jupyter_viz import make_user_input
+import solara
+
+from mesa.experimental.jupyter_viz import JupyterViz, make_user_input
 
 
 class TestMakeUserInput(unittest.TestCase):
@@ -46,3 +48,54 @@ class TestMakeUserInput(unittest.TestCase):
         mock_solara.SliderInt.assert_called_with(
             name, value=value, min=None, max=None, step=None
         )
+
+
+class TestJupyterViz(unittest.TestCase):
+    @patch("mesa.experimental.jupyter_viz.make_space")
+    def test_call_space_drawer(self, mock_make_space):
+        mock_model_class = Mock()
+        agent_portrayal = {
+            "Shape": "circle",
+            "color": "gray",
+        }
+        # initialize with space drawer unspecified (use default)
+        # component must be rendered for code to run
+        solara.render(
+            JupyterViz(
+                model_class=mock_model_class,
+                model_params={},
+                agent_portrayal=agent_portrayal,
+            )
+        )
+        # should call default method with class instance and agent portrayal
+        mock_make_space.assert_called_with(
+            mock_model_class.return_value, agent_portrayal
+        )
+
+        # specify no space should be drawn; any false value should work
+        for falsy_value in [None, False, 0]:
+            mock_make_space.reset_mock()
+            solara.render(
+                JupyterViz(
+                    model_class=mock_model_class,
+                    model_params={},
+                    agent_portrayal=agent_portrayal,
+                    space_drawer=falsy_value,
+                )
+            )
+            # should call default method with class instance and agent portrayal
+            assert mock_make_space.call_count == 0
+
+        # specify a custom space method
+        altspace_drawer = Mock()
+        solara.render(
+            JupyterViz(
+                model_class=mock_model_class,
+                model_params={},
+                agent_portrayal=agent_portrayal,
+                space_drawer=altspace_drawer,
+            )
+        )
+        altspace_drawer.assert_called_with(
+            mock_model_class.return_value, agent_portrayal
+        )