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.

Sign up for GitHub

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

Add UserInputs component #1788

Merged
merged 4 commits into from
Sep 7, 2023
Merged

Add UserInputs component #1788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bfdcc5f
Add UserInputs component
Corvince Sep 3, 2023
30341cb
update tests
Corvince Sep 3, 2023
90e1243
remove global state dependency from UserInputs
Corvince Sep 4, 2023
8d6c673
Simplify UserInputs change handler
Corvince Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 58 additions & 48 deletions mesa/experimental/jupyter_viz.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,26 @@ def JupyterViz(

current_step, set_current_step = solara.use_state(0)

solara.Markdown(name)

# 0. Split model params
model_params_input, model_params_fixed = split_model_params(model_params)

# 1. User inputs
user_inputs = {}
for name, options in model_params_input.items():
user_input = solara.use_reactive(options["value"])
user_inputs[name] = user_input.value
make_user_input(user_input, name, options)
# 1. Set up model parameters
user_params, fixed_params = split_model_params(model_params)
model_parameters, set_model_parameters = solara.use_state(
fixed_params | {k: v["value"] for k, v in user_params.items()}
)

# 2. Model
# 2. Set up Model
def make_model():
return model_class(**user_inputs, **model_params_fixed)
model = model_class(**model_parameters)
set_current_step(0)
return model

model = solara.use_memo(make_model, dependencies=list(user_inputs.values()))
model = solara.use_memo(make_model, dependencies=list(model_parameters.values()))

# 3. Buttons
def handle_change_model_params(name: str, value: any):
set_model_parameters(model_parameters | {name: value})

# 3. Set up UI
solara.Markdown(name)
UserInputs(user_params, on_change=handle_change_model_params)
ModelController(model, play_interval, current_step, set_current_step)

with solara.GridFixed(columns=2):
Expand Down Expand Up @@ -160,44 +161,53 @@ def check_param_is_fixed(param):
return True


def make_user_input(user_input, name, options):
"""Initialize a user input for configurable model parameters.
@solara.component
def UserInputs(user_params, on_change=None):
"""Initialize user inputs for configurable model parameters.
Currently supports :class:`solara.SliderInt`, :class:`solara.SliderFloat`,
and :class:`solara.Select`.

Args:
user_input: :class:`solara.reactive` object with initial value
name: field name; used as fallback for label if 'label' is not in options
options: dictionary with options for the input, including label,
Props:
user_params: dictionary with options for the input, including label,
min and max values, and other fields specific to the input type.
on_change: function to be called with (name, value) when the value of an input changes.
"""
# label for the input is "label" from options or name
label = options.get("label", name)
input_type = options.get("type")
if input_type == "SliderInt":
solara.SliderInt(
label,
value=user_input,
min=options.get("min"),
max=options.get("max"),
step=options.get("step"),
)
elif input_type == "SliderFloat":
solara.SliderFloat(
label,
value=user_input,
min=options.get("min"),
max=options.get("max"),
step=options.get("step"),
)
elif input_type == "Select":
solara.Select(
label,
value=options.get("value"),
values=options.get("values"),
)
else:
raise ValueError(f"{input_type} is not a supported input type")

for name, options in user_params.items():
# label for the input is "label" from options or name
label = options.get("label", name)
input_type = options.get("type")

def change_handler(value, name=name):
on_change(name, value)

if input_type == "SliderInt":
solara.SliderInt(
label,
value=options.get("value"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't the value be a use_state hook?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant, use_reactive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strict rule here, but I try to follow a one-directional data flow model here. JupyterViz "owns" the state here, i.e. we just pass the current parameters down to UserInputs. Any changes are brought back via the on_change method and the parent component decides what to do with the changes.

Contrast this with keeping the state inside UserParams. It wouldn't be clear how to extract the current value back to the parent component (JupyterViz) so we can act on a change. I don't know enough about how solara yet, so maybe this would also be possible. I am mainly following React best practices here - for a reference (and a great start to react in general) see https://react.dev/learn/thinking-in-react

Or maybe you meant something completely different?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in the very simplest example of Solara that you pass in reactive variable to the user input value: https://github.com/widgetti/solara/blob/master/solara/website/pages/examples/basics/sine.py. I find the on_value construct too convoluted, not to mention its nested functions for achieving the same functionality.

Copy link
Contributor Author

@Corvince Corvince Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the question is where should that state live? In the sine example global state is used. Which I did in the previous iteration, where you - correctly - blamed me for it being not reusable that way.

I think the current way is very simple. At the top level we have the model_parameters state variable that is used for the model creation. Its a simple variable-value dictionary. And we have the list of user-settable parameters. This list is passed independently of the model_parameters to UserParams. And any changes inside UserParams are handled at the top level by updating the model_parameters. This way we have maximal reusability of the UserParams component.

Now if we were to define the reactive variables at the JupyterViz component and pass them to UserParams we have a tight coupling between the user parameters and the model parameters and the logic resides both in JupyterViz and UserParams. UserParams can't be used without model_parameters although that dependency is not strictly required.

And if we define the reactive variables inside UserParams than we are back to my problem of not knowing how to get the updated value back to the parent component.

I fully agree that it appears a bit convoluted at first and especially the nested functions are not nice - in JS the function definition could simply be written as (value) => (name) => on_change(name, value). I don't know if it would be possible to have a similar concise syntax in Python. But the important part is that it is not the same functionality. Just defining value as a reactive value doesn't help us changing the model_parameters dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nested functions are just one of the complications.
It's that you have to do handle_* at the JupyterViz level, and do something along the line of subsequent on_change's inside the subcomponents. Simpler would be to have this at the parent component, and at the last leg of the child component.

I'm not sure what you meant by UserParams. Is it a hypothetical reactive variable, a Solara component, a class, or something else not yet implemented? If it is initialized inside JupyterViz, why can't JupyterViz be notified of its change when you can add do solara.use_reactive(..., on_change=do_something)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the UserParams confusion. I meant the UserInputs component implemented in this PR.

It makes sense for the handle_ function to be at JupyterViz because it changes the model_parameters state - where else should that happen.

Frankly I don't understand the rest of your comment. Maybe it would be more productive if you could have some concrete advice on how to move forward. Is this blocking for this PR or could this be an improvement in a further PR? I am really not sure anymore what your critique revolves around and how to move on

on_value=change_handler,
min=options.get("min"),
max=options.get("max"),
step=options.get("step"),
)
elif input_type == "SliderFloat":
solara.SliderFloat(
label,
value=options.get("value"),
on_value=change_handler,
min=options.get("min"),
max=options.get("max"),
step=options.get("step"),
)
elif input_type == "Select":
solara.Select(
label,
value=options.get("value"),
on_value=change_handler,
values=options.get("values"),
)
else:
raise ValueError(f"{input_type} is not a supported input type")


def make_space(model, agent_portrayal):
Expand Down
64 changes: 41 additions & 23 deletions tests/test_jupyter_viz.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,53 +1,71 @@
import unittest
from unittest.mock import Mock, patch

import ipyvuetify as vw
import solara

from mesa.experimental.jupyter_viz import JupyterViz, make_user_input
from mesa.experimental.jupyter_viz import JupyterViz, UserInputs


class TestMakeUserInput(unittest.TestCase):
def test_unsupported_type(self):
@solara.component
def Test(user_params):
UserInputs(user_params)

"""unsupported input type should raise ValueError"""
# bogus type
with self.assertRaisesRegex(ValueError, "not a supported input type"):
make_user_input(10, "input", {"type": "bogus"})
solara.render(Test({"mock": {"type": "bogus"}}), handle_error=False)

# no type is specified
with self.assertRaisesRegex(ValueError, "not a supported input type"):
make_user_input(10, "input", {})
solara.render(Test({"mock": {}}), handle_error=False)

def test_slider_int(self):
@solara.component
def Test(user_params):
UserInputs(user_params)

@patch("mesa.experimental.jupyter_viz.solara")
def test_slider_int(self, mock_solara):
value = 10
name = "num_agents"
options = {
"type": "SliderInt",
"value": 10,
"label": "number of agents",
"min": 10,
"max": 20,
"step": 1,
}
make_user_input(value, name, options)
mock_solara.SliderInt.assert_called_with(
options["label"],
value=value,
min=options["min"],
max=options["max"],
step=options["step"],
)
user_params = {"num_agents": options}
_, rc = solara.render(Test(user_params), handle_error=False)
slider_int = rc.find(vw.Slider).widget

assert slider_int.v_model == options["value"]
assert slider_int.label == options["label"]
assert slider_int.min == options["min"]
assert slider_int.max == options["max"]
assert slider_int.step == options["step"]

@patch("mesa.experimental.jupyter_viz.solara")
def test_label_fallback(self, mock_solara):
def test_label_fallback(self):
"""name should be used as fallback label"""
value = 10
name = "num_agents"

@solara.component
def Test(user_params):
UserInputs(user_params)

options = {
"type": "SliderInt",
"value": 10,
}
make_user_input(value, name, options)
mock_solara.SliderInt.assert_called_with(
name, value=value, min=None, max=None, step=None
)

user_params = {"num_agents": options}
_, rc = solara.render(Test(user_params), handle_error=False)
slider_int = rc.find(vw.Slider).widget

assert slider_int.v_model == options["value"]
assert slider_int.label == "num_agents"
assert slider_int.min is None
assert slider_int.max is None
assert slider_int.step is None


class TestJupyterViz(unittest.TestCase):
Expand Down