Updated docs and check_model param

[nissu99]

Summary

This PR adds documentation for the SolaraViz instantiation and adds a new test case to handle arguments and keyword arguments in solaraviz.

Bug / Issue

SolaraViz docs #2494

Implementation

changes in docs and explicit check in _check_model_params.

Testing

Additional Notes

[github-actions]

Performance benchmarks:

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

[EwoutH]

Thanks for your PR!

Could you format your PR following our bug template? That makes it readable on its own, helps us review and serves as a historical reference if we ever need to revisit it.

[quaquel]

I have left a few suggestions and 1 big point. However, some of these suggestions might have knock-on consequences elsewhere in the code. Also, you should most likely have to change the signature of MoneyModel to keyword arguments only.

The big thing is that the check in solara_viz.py is not doing what you think it is doing. It would be good to explicitly add a test to test_solara_viz.py which checks that the appropriate exception is raised if you try to use SolaraViz with a model that has arguments and keyword arguments.

[quaquel]

Suggested change

	"model1 = MoneyModel(n=50, width=10, height=10) #keyword arguments\n",
	"model = MoneyModel(n=50, width=10, height=10) #keyword arguments\n",

[tpike3]

Needs to be model not model1

[nissu99]

@quaquel ,i have added the test case.

[quaquel]

@quaquel ,i have added the test case.

I'll try to review this asap, but it might be somewhere next week before I get to it because I am traveling for work atm.

[tpike3]

@nissu99 Thanks for ding this and the fact you updated tests, migration guide, and tutorial is very cool!

I had one comment and will defer to @quaquel for final review.

[EwoutH]

@tpike3 @quaquel what's the state of this PR, and what's steps are needed to get it merged?

[quaquel]

I think all that is left is the last point on the failing read the docs built. It should be an easy fix that I had hoped to do today (but life intervened)

[nissu99]

@quaquel @EwoutH will make the required changes today .

[tpike3]

@nissu99 Thanks again for doing this! As indicated in #2535 this is needed

It is passing the build but I found some other model1 that need to be changed to model

For the solara_viz I would get rid of VAR_KEYWORD all together. I also made the value error more explicit as we have some users new to python and coding. Feel free to further improve

As soon as you get this done we will get it merged!

[nissu99]

@tpike3 thanks for all the instructive comments ,i have made the changes.

[tpike3]

Thanks @nissu99!

Concerns addressed

[EwoutH]

Great work, thanks!

[EwoutH]

@tpike3 could you please Squash and merge when merging PRs? That keeps our commits on the main branch clean. Clean commits can be easier backported or reverted if needed.