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

feat: Display model seed & allow user to specify it in JupyterViz #2069

Merged
merged 6 commits into from Mar 17, 2024

Conversation

rht
Copy link
Contributor

@rht rht commented Mar 5, 2024

Fixes: #1812. The motivation can be found in that issue. It looks like this
2024-03-05T03:36:30,978108357-05:00
The current behavior is that the seed is not changed even when you press the reset button. After all, it is now a user input param. However, if you force refresh the browser, the seed is still not changed. A more sustainable solution (so that user doesn't accidentally lose the seed) is to add a button to generate a new seed.

Copy link

github-actions bot commented Mar 5, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 +0.8% [+0.5%, +1.1%] 🔵 +0.6% [+0.4%, +0.9%]
Schelling large 🔵 +0.6% [-0.0%, +1.2%] 🔵 -3.0% [-5.2%, -1.0%]
WolfSheep small 🔵 +0.7% [-0.6%, +1.9%] 🔵 +0.2% [-0.0%, +0.4%]
WolfSheep large 🔵 +0.3% [-0.4%, +1.2%] 🔵 +0.5% [-1.8%, +3.4%]
BoidFlockers small 🔵 +1.0% [+0.3%, +1.8%] 🔵 +1.9% [+1.2%, +2.7%]
BoidFlockers large 🔵 +1.3% [+0.6%, +1.9%] 🔵 +2.1% [+1.6%, +2.5%]

@EwoutH EwoutH requested a review from Corvince March 5, 2024 12:23
@EwoutH EwoutH added the enhancement Release notes label label Mar 5, 2024
@EwoutH
Copy link
Contributor

EwoutH commented Mar 5, 2024

Sounds useful, thanks! I will leave the formal review to Corvince.

One thought: Maybe we can have separate buttons for requesting a new seed and resetting the simulation to it's initial state.

@@ -71,6 +71,7 @@ def JupyterViz(
simulations with no space to visualize should
specify `space_drawer=False`
play_interval: play interval (default: 150)
seed: the random seed used to initialize the model
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we explicitly pass in the random model seed so it is obvious to the user where the seed is coming form and what they are changing is they pass in their own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already an explicit argument. Specified by adding a "seed" entry to the model_params dictionary: https://github.com/projectmesa/mesa-examples/blob/main/examples/schelling_experimental/app.py#L21.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to remove that line from the docstring.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rht I am sorry I don't mean to belabor this, but I feel like I am missing something here. So please bear with me, my understanding is below.

Scenario 1: User does not specify the seed so we want the UI to show the seed that was selected. Shouldn't the display of the random seed reference obj._seed

Scenario 2: User specifies the seed* then it would display the "seed" parameter or still reference obj._seed

Thank you for bearing with me, I know we have had issues with the seed in the past and just want to make doubly sure we have one solid pointer to the model seed.

Let me know what you think

*Apologies my earlier comment was very confusing. seed should come from model_params and be explicitly displayed not explicitly passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scenario 1: User does not specify the seed so we want the UI to show the seed that was selected. Shouldn't the display of the random seed reference obj._seed

The direction of the cause and effect should flow from the controls you see on the screen, and then the model properties. It can't be the obj._seed to have a value before the control.

If you notice, the seed value is displayed twice. The one below the information card is model._seed, displayed for debugging purpose. I will remove this once you agree to merge.

@rht
Copy link
Contributor Author

rht commented Mar 6, 2024

Update:

  • simplified the implementation by separating the seed from model parameters. The diff is smaller now.
  • changed the play widget color to be blue
  • added back the seed as an optional argument
  • added a reseed button

2024-03-06T12:15:51,712164823-05:00

To test this PR, you need to modify your model.py, __init__, e.g. for Schelling, to accept seed as an argument

    def __init__(self, width=20, height=20, density=0.8, minority_pc=0.2, homophily=3, seed=0):
        super().__init__(seed=seed)

@rht rht force-pushed the jupyterviz_seed branch 2 times, most recently from 10f2d57 to 1ee4276 Compare March 6, 2024 17:33
@Corvince
Copy link
Contributor

Corvince commented Mar 6, 2024

To test this PR, you need to modify your model.py, __init__, e.g. for Schelling, to accept seed as an argument

    def __init__(self, width=20, height=20, density=0.8, minority_pc=0.2, homophily=3, seed=0):
        super().__init__(seed=seed)

Hm, does that mean all the models need to accept a seed parameter? I don't think thats optimal

@rht
Copy link
Contributor Author

rht commented Mar 7, 2024

Hm, does that mean all the models need to accept a seed parameter? I don't think thats optimal

make_model can be modified to have a try-except to catch the TypeError: Schelling.__init__() got an unexpected keyword argument 'seed' error to do a fallback to not showing the seed controls.

@rht
Copy link
Contributor Author

rht commented Mar 7, 2024

Update: with 34a1853, no modification on existing models is necessary anymore. It works out of the box.

@tpike3
Copy link
Contributor

tpike3 commented Mar 8, 2024

LGTM - This is very cool, the model seed is one of those critical details that have burned many a modeler and I think making it explicit is an excellent idea.

To merge it is (1) fixing the tests and (2) removing the seed below the information card. Thanks @rht

@rht
Copy link
Contributor Author

rht commented Mar 15, 2024

@tpike3 sorry for the slow response, still recovering. I fixed the test by using pytest-mock instead of unittest mock. And removed the seed below the information card. I request this PR to be rebased & merged instead of squashed & merged, because it has the 2 unrelated commits:

  1. Change the play/stop button to blue
  2. Code cleanup

Copy link
Contributor

@tpike3 tpike3 left a comment

Choose a reason for hiding this comment

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

LGTM - I also like the change to the current_step as a parameter, at least for me much more intuitive.

@tpike3 tpike3 merged commit 57d9cb5 into projectmesa:main Mar 17, 2024
12 checks passed
@rht rht deleted the jupyterviz_seed branch March 19, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JupyterViz: display simulation seed on screen so that the result can be recreated if needed
4 participants