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

High-level Simulation type to manage time stepping #621

Merged
merged 23 commits into from
Feb 16, 2020

Conversation

ali-ramadhan
Copy link
Member

This PR introduces a Simulation type that manages time stepping with a high level interface.

I think diagnostics and output_writers will have to become keyword arguments to time_step!.

It will be a pretty big breaking change once we move diagnostics and output_writers outside of Model so would be good to agree on the design and API before refactoring.

@glwagner Let me know what you think, we can iterate on the design in this PR.

Resolves #432
Resolves #447

@ali-ramadhan ali-ramadhan added feature 🌟 Something new and shiny abstractions 🎨 Whatever that means labels Feb 11, 2020
@ali-ramadhan ali-ramadhan changed the title Drafting a Simulation type and a run! simulation function High-level Simulation type to manage time stepping Feb 11, 2020
@glwagner
Copy link
Member

glwagner commented Feb 11, 2020

Ah so great. One quick thought: we can reinterpret time_step to refer only to a single time-step, and move the current multi step function to run! Then time_step does not take keyword arguments. Is there a convenience function that takes AbstractModel as input rather than Simulation? I think that’s important for simple scripts and examples. Agree this is a huge breaking change that we should finalize as soon as possible.

src/Simulation.jl Outdated Show resolved Hide resolved
src/Simulation.jl Outdated Show resolved Hide resolved
src/Simulation.jl Outdated Show resolved Hide resolved
@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Feb 11, 2020

From discussion with @glwagner:

  1. Should get rid of
    https://github.com/climate-machine/Oceananigans.jl/blob/73bea229721624d65d4e2b2b79810622cf221993/src/TimeSteppers/adams_bashforth.jl#L39
  2. Multiple stop criteria would be nice: called via stop_simulation(simulation).
  3. stop_criteria should be a Tuple of functions.
  4. We want a run_until!(model, t) or time_step_until!(model, t) function for AB2 (and RK3).
  5. Should be progress(simulation).
  6. Deprecate but keep around time_step! for multiple time steps.

@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #621 into master will increase coverage by 0.34%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
+ Coverage   74.53%   74.87%   +0.34%     
==========================================
  Files         117      118       +1     
  Lines        2246     2277      +31     
==========================================
+ Hits         1674     1705      +31     
  Misses        572      572              
Impacted Files Coverage Δ
src/Diagnostics/nan_checker.jl 33.33% <0.00%> (-46.67%) ⬇️
src/Utils/pretty_time.jl 40.00% <0.00%> (-13.34%) ⬇️
src/Utils/time_step_wizard.jl 100.00% <0.00%> (ø) ⬆️
src/Simulations.jl 93.75% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb9b07f...397c15d. Read the comment docs.

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Feb 15, 2020

Simulation API is almost ready.

Comments on some of the points above:
3. I made stop criteria an array so you can push/append extra stop criteria as needed.
4. Out of scope for this PR.
6. Went all out and completely removed time_step! for multiple time steps. Only the single step version remains.

Major breaking changes:

  1. Diagnostics and output writers are now part of Simulation, not Model.
  2. time_step! takes a single time step. For multiple time steps, use a loop or a Simulation.

Still need to:

  1. Write some simple Simulation units tests although it's already used by a lot of tests already.
  2. Update examples.
  3. Update model setup documentation.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Too much to review all the test edits, but this is epic. Onwards!

src/Simulations.jl Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

'Simulation' type for managing time stepping Make progress statements a part of time_step!
2 participants