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

New Simulation API: more callbacks, less "progress" #1971

Merged
merged 51 commits into from
Nov 3, 2021
Merged

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Sep 9, 2021

This PR overhauls the API for Simulation, TimeStepWizard, and printing of progress. It also simplifies the implementation of run!.

After this PR, Simulation no longer accepts the keyword arguments iteration_interval or progress. Instead, progress printing is achieved with callbacks, eg:

progress(sim) = @info "Iteration: $(iteration(sim)), time: $(time(sim))"
simulation.callbacks[:progress] = Callback(progress, IterationInterval(100))

It also refactors the TimeStepWizard so that it can be used as a callback, eg

wizard = TimeStepWizard(cfl=1.0, max_change=1.1, max_Δt=2minutes)
simulation.callbacks[:wizard] = Callback(wizard, IterationInterval(10))

This is a better design for a few reasons:

  1. Adaptive time-stepping and progress printing are not longer arbitrarily constrained to occur on the same iteration interval.
  2. Both progress printing and adaptive time-stepping can use any schedule (rather than only IterationInterval).
  3. The simulation time-step is always simulation.Δt. No more shenanigans like simulation.Δt.Δt.

Eventually, we should also eliminate the "diagnostics" list so that we have only two lists of callback-like objects: simulation.callbacks and simulation.output_writers.

I think this resolves an issue or two but I need to find them. Also, I've so far only updated the examples. There are probably tests and validation cases that need to be updated for the new API as well.

This PR is an important step toward generalizing Oceananigans.Simulation so that it can be used by ClimaAtmos.jl. cc @bischtob @akshaysridhar

@glwagner
Copy link
Member Author

glwagner commented Sep 9, 2021

Well, we are almost done with #1138 but not quite. We still need to nuke simulation.diagnostics for that.

@navidcy
Copy link
Collaborator

navidcy commented Sep 11, 2021

hm... shall I have a look? seems like test are failing?

@navidcy
Copy link
Collaborator

navidcy commented Sep 11, 2021

No more shenanigans like simulation.Δt.Δt.

:)

@glwagner
Copy link
Member Author

hm... shall I have a look? seems like test are failing?

I think the tests are failing because of some detail about how the simulations are stopped / time-step is aligned at the end of a run --- the overall design can be reviewed!

@glwagner
Copy link
Member Author

@tomchor @navidcy @simone-silvestri can any of you help review this PR?

Copy link
Collaborator

@tomchor tomchor left a comment

Choose a reason for hiding this comment

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

This is looking good! Thanks for going through all this effort.

This PR is definitely too big and complex for me fully grasp the details, but I helped with some minor comments wherever I could.

src/Simulations/simulation.jl Show resolved Hide resolved
src/Simulations/simulation.jl Outdated Show resolved Hide resolved
src/Simulations/simulation.jl Show resolved Hide resolved
src/Simulations/time_step_wizard.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

If tests pass go for it

@glwagner glwagner merged commit 9d1643c into main Nov 3, 2021
@glwagner glwagner deleted the glw/simulanigans branch November 3, 2021 03:01
@tomchor
Copy link
Collaborator

tomchor commented Nov 3, 2021

Awesome PR!

Quick question: did we skip version 0.64? I seems like this PR bumped the version from 0.63.4 to 0.65.

@glwagner
Copy link
Member Author

glwagner commented Nov 3, 2021

That was a mistake, so I'll bump back to 0.64...

@aramirezreyes
Copy link
Contributor

aramirezreyes commented Dec 26, 2021

Could someone take a look at this related comment? #1895 (comment) I would argue in favor of restoring the "parameters" argument or an alternative way of passing additional parameters to the callback function.

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

Successfully merging this pull request may close these issues.

None yet

5 participants