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

Cleaning up Simulation and run! #1095

Closed
glwagner opened this issue Oct 22, 2020 · 1 comment
Closed

Cleaning up Simulation and run! #1095

glwagner opened this issue Oct 22, 2020 · 1 comment
Labels
abstractions 🎨 Whatever that means feature 🌟 Something new and shiny

Comments

@glwagner
Copy link
Member

glwagner commented Oct 22, 2020

I think there's a bit of cleanup to do with the interface to both Simulation and run!.

Right now run! is barebones. It will take one more argument pickup when #1082 is merged.

I think run! should have more kwargs like diagnostics, any of the stop_criteria, Δt, and progress, since these parameters are currently only used in run!. We currently require these parameters to be specified in the Simulation constructor`:

function Simulation(model; Δt,
stop_criteria = Any[iteration_limit_exceeded, stop_time_exceeded, wall_time_limit_exceeded],
stop_iteration = Inf,
stop_time = Inf,
wall_time_limit = Inf,
diagnostics = OrderedDict{Symbol, AbstractDiagnostic}(),
output_writers = OrderedDict{Symbol, AbstractOutputWriter}(),
progress = default_progress,
iteration_interval = 1,
parameters = nothing)

However, I think its better if we encourage them to be set in run! where they are used (producing scripts that are more "locally understandable"). Note that storing these parameters in Simulation is really a convenience feature (eg if you want to reference them after using run!, you can) rather than essential to how run! functions. This would clean up cases where users want to embed run! within a loop, because they can then write

for i = 1:10
    run!(simulation, stop_iteration=model.clock.iteration+10)
end

or even (see below)

for i = 1:10
    run!(simulation, stop=Iteration(model.clock.iteration+10))
end

rather than the slightly more convoluted approach (its not a lot of code, but slightly more confusing I think) that is still used in some examples.

run! might also need a make-over, since I think we should do away with iteration_interval and use AbstractSchedules for progress (or perhaps "callafter") and the TimeStepWizard.

We may also want to design AbstractCriteria that mirror the design of AbstractSchedules for stopping a simulation, so we can have a similar interface for stop criteria as output scheduling, eg something like stop=SimulationTime(1day).

I suggest we collect and discussl the extant issues we see with the current design of Simulation here.

PS do we need Simulation.parameters?

@glwagner glwagner added abstractions 🎨 Whatever that means feature 🌟 Something new and shiny labels Oct 26, 2020
@glwagner
Copy link
Member Author

glwagner commented Nov 4, 2020

Hmm, this overlaps with #1138 so I'm closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abstractions 🎨 Whatever that means feature 🌟 Something new and shiny
Projects
None yet
Development

No branches or pull requests

1 participant