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

Fix #2452 - handle solara viz model params better #2454

Merged
merged 7 commits into from
Nov 5, 2024
Merged

Fix #2452 - handle solara viz model params better #2454

merged 7 commits into from
Nov 5, 2024

Conversation

Corvince
Copy link
Contributor

@Corvince Corvince commented Nov 4, 2024

Summary

Bug description is in #2452, here is a copy

  1. If the model takes one or more arguments and those arguments are not part of the user-controllable parameters that are set from the UI, stuff will crash immediately
  2. Strangely, it crashes immediately even though we started SolaraViz with a valid model instance, so this points to a second bug: at the moment, the model shown after starting the UI is not the same as the original model with which the UI was initialized. Again, this is easy to reproduce. In the boltzman wealth model, in app.py, start the model with say a width and height of 20. Next, start Solara, and in the GUI, you'll see that the actual model has reverted back to 10 by 10 (as specified in model_params).
  3. Seed is currently handled differently from other parameters. With model: Move random seed and random to init #1940 merged, this is no longer needed, and seed can just be integrated into the user-specifiable parameters.

Implementation

  1. On startup solara viz will check the user_params and compare to model.__class__.__init__. If there are are parameters that don't have a default value but are not in user_params solara viz will raise a ValueError. Also raises ValueError if user provided model_params include parameters not used by the model.
  2. Currently the ModelCreator was creating a new instance whenever model parameters changed. However, the initial call to ModelCreator "changed" the parameters, triggering an update and overwriting the provided model. With this PR, only actual parameter changes trigger a model re-creation
  3. "Reactive" Seed handling was removed completely. No its treated just like any other model parameter.

Copy link

github-actions bot commented Nov 4, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🟢 -7.7% [-9.2%, -6.3%] 🟢 -3.2% [-3.4%, -3.0%]
BoltzmannWealth large 🔵 -3.3% [-3.7%, -3.0%] 🟢 -7.4% [-8.6%, -6.2%]
Schelling small 🔵 -1.4% [-1.9%, -1.0%] 🔵 -1.6% [-1.9%, -1.3%]
Schelling large 🔵 +0.2% [-0.6%, +1.0%] 🔵 -1.8% [-4.2%, +0.9%]
WolfSheep small 🔵 -1.2% [-1.6%, -0.8%] 🔵 -0.7% [-0.9%, -0.4%]
WolfSheep large 🔵 -1.3% [-2.6%, -0.0%] 🔵 -1.8% [-5.5%, +1.4%]
BoidFlockers small 🟢 -3.6% [-4.2%, -3.1%] 🔵 -0.7% [-1.5%, +0.2%]
BoidFlockers large 🟢 -4.0% [-4.7%, -3.4%] 🔵 -1.2% [-1.8%, -0.6%]

@quaquel
Copy link
Member

quaquel commented Nov 4, 2024

Potentially allows us to remove deepcopy for reinitializing a model.

I had a look at this yesterday and my approach would be to change do_reset from using the original model to create a new model instance based on the current values of model_parameters. This requires some moving around of code. Basically the stuff before on_change in ModelCreator would move up to SolaraViz such that the reactive version of model_parameters is available to ModelController. Hope this makes sense, happy to open a PR with my attempted fix if you want a closer look at the code.

@quaquel
Copy link
Member

quaquel commented Nov 4, 2024

A few minor comments, but great to see you working on this.

@Corvince
Copy link
Contributor Author

Corvince commented Nov 5, 2024

Updated the logic, added tests and reverted temporary example changes. Also moved the checking to ModelCreator where it is actually needed. Ready for review

@Corvince Corvince marked this pull request as ready for review November 5, 2024 09:48
Copy link

github-actions bot commented Nov 5, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 +0.0% [-1.3%, +1.3%] 🔵 -0.6% [-0.8%, -0.5%]
BoltzmannWealth large 🔵 +0.2% [-0.2%, +0.6%] 🔵 +0.4% [-0.1%, +1.0%]
Schelling small 🔵 -0.1% [-0.5%, +0.3%] 🔵 -0.6% [-0.9%, -0.4%]
Schelling large 🔵 +0.1% [-0.4%, +0.7%] 🔵 +1.0% [+0.6%, +1.5%]
WolfSheep small 🔵 -0.4% [-0.6%, -0.3%] 🔵 -0.1% [-0.2%, +0.1%]
WolfSheep large 🔵 +0.7% [-0.0%, +1.5%] 🔵 +2.8% [+0.6%, +5.1%]
BoidFlockers small 🔵 +1.1% [+0.7%, +1.5%] 🔵 +0.4% [-0.4%, +1.3%]
BoidFlockers large 🔵 +1.6% [+1.0%, +2.1%] 🔵 +1.1% [+0.6%, +1.6%]

Copy link
Member

@quaquel quaquel left a comment

Choose a reason for hiding this comment

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

One suggestions (probably a left over from testing) and then this is good to be merged.

@Corvince Corvince merged commit e9c0530 into main Nov 5, 2024
12 checks passed
@EwoutH EwoutH added the bug Release notes label label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants