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

Support for ABMSimulator and DEVSSimulator in SolaraViz #2428

Closed
quaquel opened this issue Oct 27, 2024 · 7 comments · Fixed by #2470
Closed

Support for ABMSimulator and DEVSSimulator in SolaraViz #2428

quaquel opened this issue Oct 27, 2024 · 7 comments · Fixed by #2470
Labels
feature Release notes label

Comments

@quaquel
Copy link
Member

quaquel commented Oct 27, 2024

At the moment, it's not possible to use SolaraVis for models that use the experimental event scheduling functionality. This is because SolaraViz expects a model and does all control of the model via model.step.

The issue can be addressed in at least 2 ways: (1) change event scheduling to not use a simulator and a model but turn the simulator into e.g., a mixin for a model; (2) expand SolaraViz to work with either a model or a (model, simulator).

@quaquel quaquel added the feature Release notes label label Oct 27, 2024
@quaquel
Copy link
Member Author

quaquel commented Nov 7, 2024

Thinking about this a bit more, and in light of some of the recent PRs on SolaraViz, it might be quite easy to support this. The key thing that changes is the ModelController, which would probably be something like a SimulatorController. Basically, SolaraViz would take an optional simulator keyword argument. If this is passed, SolaraViz creates a SimulatorController instead of a ModelController. At the UI side, they would look the same, but at the backend, SimulatorController would interact with the simulator. So step would call simulator.run_for (or even only do next event time progression, which is not currently easy to do on the simulator), while play would work similar to how it's done already in the controller (basically instead of wrapping model.step, you would wrap model.next_event or a time_delta).

@Corvince
Copy link
Contributor

Corvince commented Nov 7, 2024

Sounds good! Would be great if it works as you described, it would show the benefit of the modular approach (just exchaning the model controller)

@quaquel
Copy link
Member Author

quaquel commented Nov 7, 2024

Exactly, it's the realization of how nicely modular SolaraViz is that makes this quite easy to do now.

I hope to have some time in the coming 2 weeks to put in a PR for this, which in turn would allow #2364 to progress further.

@quaquel
Copy link
Member Author

quaquel commented Nov 8, 2024

I started working on this but I am running into 3 issues:

  1. SolaraViz updates everything via patching the model.step function. However, this does not work in combination with a simulator. On startup, the simulator already has an event to call the original model.step method, rather than the solaraviz wrapped step method. Would it not make more sense to make model.steps, so the clock, a reactive variable and tie updates to this?
  2. It appears that the model is not updating while running, even though I can see that the do_step method is being called. My hunch this is tied to the previous point. ABMSimulator schedules model.step events, but it seems this uses the unpatched step method rather than the patched step method.
  3. At the moment, ModelCreator creates a new model instance for every on_change event. So, you are, in essence, resetting the model if you interact with the model controllers. For now, I removed this behavior and let on_change only update the model parameters. This means to restart the model based on the modified parameters, you have to call reset explicitly. To maintain the old behavior, I would have to make ModelCreator also dependent on the absence or presence of a simulator.

@EwoutH
Copy link
Member

EwoutH commented Nov 8, 2024

Thanks for working on this, if you need any specific input let us know.

@quaquel
Copy link
Member Author

quaquel commented Nov 8, 2024

I'll continue to work on this, but input from @Corvince on the above points would be appreciated

It also makes me want to close #2461 even more. Debugging this without logging is a pain.

@Corvince
Copy link
Contributor

Corvince commented Nov 8, 2024

Just really quick response, without going into too much details.

The main trigger for updates is calling force_update(). I don't remember at the moment why I tied the updates to model.step. but just calling force_update() inside do_step should be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Release notes label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants