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

Only set model_parameters once #2505

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Only set model_parameters once #2505

merged 1 commit into from
Nov 13, 2024

Conversation

Corvince
Copy link
Contributor

Summary

While working on a seed widget I saw a major bug in solara viz. Model_parameters get reset on every model reset. This is pretty serious and I think we need another bug release soon.

Bug / Issue

Describe the bug
Hitting "reset" in the ui resets the model, but also resets the model_parameters to their initial value.

To Reproduce
Run Schelling example. Reduce agent density. Hit reset. Works as expected. Hit reset again. Density is now restored.

Implementation

Use solaras use_effect to initialize model_parameters only once.

screen-capture.webm

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.

Would there be a way to test this behavior as part of our unit tests?

@quaquel
Copy link
Member

quaquel commented Nov 13, 2024

That is a serious bug and something that had been nagging me but did not investigate yet. Great you spotted it and fixed it.

I will put some effort on the logging stuff because that would have helped in spotting this earlier.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🟢 -9.4% [-10.9%, -7.9%] 🔵 -0.1% [-0.2%, +0.1%]
BoltzmannWealth large 🔵 +1.1% [+0.8%, +1.4%] 🔵 +0.9% [+0.3%, +1.6%]
Schelling small 🔵 +0.6% [+0.1%, +1.0%] 🔵 -0.9% [-1.2%, -0.7%]
Schelling large 🔵 +1.1% [+0.6%, +1.5%] 🔵 -0.2% [-1.8%, +2.0%]
WolfSheep small 🔵 +2.5% [+2.1%, +2.9%] 🔵 +0.6% [+0.3%, +0.8%]
WolfSheep large 🔵 +1.7% [+0.6%, +3.1%] 🔵 +0.9% [-2.1%, +5.0%]
BoidFlockers small 🔵 +0.3% [-0.2%, +0.9%] 🔵 +0.3% [-0.4%, +1.0%]
BoidFlockers large 🔵 +0.4% [-0.2%, +1.1%] 🔵 -0.1% [-0.7%, +0.5%]

Copy link
Member

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Thanks for catching it and fixing it. I will do a backport + release as soon as you merge.

Anything else we'll want to include in the backport?

@EwoutH EwoutH added bug Release notes label backport-candidate PRs we might want to backport to an earlier branch labels Nov 13, 2024
@Corvince Corvince merged commit 747d1c9 into main Nov 13, 2024
14 checks passed
@Corvince
Copy link
Contributor Author

Would there be a way to test this behavior as part of our unit tests?

Not right now, at least not in an easy way. This would require a integration test that combined solara viz, ModelController and ModelCreator. We don't have that yet

@EwoutH
Copy link
Member

EwoutH commented Nov 13, 2024

3.0.3 with this fix is released.

@EwoutH EwoutH deleted the bugs/reset-model-parameters branch January 7, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate PRs we might want to backport to an earlier branch bug Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants