(0.104.0) Reformulate hydrostatic model timestepping#4811
(0.104.0) Reformulate hydrostatic model timestepping#4811simone-silvestri merged 756 commits intomainfrom
Conversation
| velocities :: U # Container for velocity fields `u`, `v`, and `w` | ||
| transport_velocities :: W # Container for velocity fields used to transport tracers |
There was a problem hiding this comment.
the difference between "normal velocities" and "transport velocities" is unclear
There was a problem hiding this comment.
Hmm, how should we call them? They are the velocities that transport tracers around. transport is not good because it implies a multiplication times the area...
There was a problem hiding this comment.
what are the units of the transport velocities?
There was a problem hiding this comment.
it is also m/s so the same as the velocities
|
I am at the point here that the RK formulation is conservative both locally and globally. |
|
why are there so many changes to WENO advection, etc? |
|
It's really hard for me to understand how this is an improvement. I think some explanation is needed (maybe docs or a docstring...) |
|
It wouldn't be a completely crazy idea to start a new docs section on "Time steppers" which outlines how each time stepper works. Esp because there appears to be a kind of interface for using different methods, eg AB2 and RK3. |
I have merged in #4434 because I was expecting to merge it before this one. Maybe that was a mistake |
Yeah that would be nice! After this PR we can also use RK for sea ice, for example, by extending |
Do you want us to also review the WENO changes then or ignore them? |
|
Yeah, also some of the code here probably still needs to change as I am fixing the implementation. |
simone-silvestri
added
the
benchmark performance
There are 115 files changed on this PR. Does it make sense to split up some of these objectives? It seems like the first objective ("clean up time-stepping") doesn't need to be part of a PR that implements new methods. Also, why wouldn't it make sense to implement each of the different time-stepping methods in a new PR? This would make it easier and faster to get the PR merged and finished. |
|
Yeah, I will definitely split the PR; however, most of the changes here are already in #4434. So, until that PR is merged, the scale of this one is not really reflected on the file changes |
…/Oceananigans.jl into ss/reformulate-hydrostatic-model-2
Co-authored-by: Gregory L. Wagner <[email protected]>
| for stages in 2:5 | ||
| @eval TimeStepper(::Val{Symbol(:SplitRungeKutta, $stages)}, args...; kwargs...) = | ||
| SplitRungeKuttaTimeStepper(args...; coefficients=tuple(collect($stages:-1:1)...), kwargs...) | ||
| end |
There was a problem hiding this comment.
to be able to do
timestepper = :SplitRungeKutta2 of
timestepper = :SplitRungeKutta4we discussed above with @ali-ramadhan that we do not want to support schemes with higher number of stages since they are not tested and not be beneficial
There was a problem hiding this comment.
a github discussion is good, but that fact is not documented! also you could consider simply requiring users to build the scheme manually rather than generating constructors for the symbols (up to you)
There was a problem hiding this comment.
I can add something in the docs to say that we can build a timestepper appending the number of stages (:SplitRungeKutta3 for example).
…/Oceananigans.jl into ss/reformulate-hydrostatic-model-2
…/Oceananigans.jl into ss/reformulate-hydrostatic-model-2
| function assemble_advective_dissipation!(P, grid, ts::RungeKuttaScheme, substep, Fⁿ, Fⁿ⁻¹, Uⁿ, Uⁿ⁻¹, cⁿ⁺¹, cⁿ) | ||
| if substep == ts.Nstages |
There was a problem hiding this comment.
From JETLS:
FieldError: type Oceananigans.TimeSteppers.RungeKutta3TimeStepper has no field
Nstages, available fields:γ¹,γ²,γ³,ζ²,ζ³,Gⁿ,G⁻,implicit_solver
Oceananigans.jl/src/TimeSteppers/runge_kutta_3.jl
Lines 16 to 25 in 87c7213
RungeKuttaScheme is
SplitRungeKuttaTimeStepper: Oceananigans.jl/src/TimeSteppers/split_runge_kutta.jl
Lines 14 to 20 in 87c7213
Same for assemble_diffusive_dissipation! below.
There was a problem hiding this comment.
Right we can probable remove that alias, for RK3 this method is not supported. I have started in #4551 but then did not get to complete it
9 participants
This is a work-in-progress PR that aims to reformulate the timestepping scheme of the
HydrostaticFreeSurfaceModeltomake sure that three numerical properties are satisfied:
This is easier to achieve with an explicit discretization of the free surface (
ExplicitFreeSurfaceandSplitExplicitFreeSurface) rather than an implicit free surface discretization and with RK3 rather than AB2.However, this PR reformulates much of the timestepping algorithm, so it will be a lengthy WIP that will need to be thoroughly validated for all combinations of barotropic and baroclinic timestepping schemes.
More changes are performed to correctly abstract the timestepping schemes, which, at the moment, are tailored specifically to the
NonhydrostaticModel(for example, the timesteppers include acompute_pressure_correction!or acorrect_velocitiesfunction, which are meaningful only for a non-hydrostatic Navier Stokes solver)Edit: another objective I have is to remove any timestepping from the
update_state!function. We should be able to callupdate_state!multiple times in a row without problems and timestepping insideupdate_state!negates this possibility.TODO:
Remove timestepping fromIn another PRupdate_state!DistributedGridsImplement a semi - conservative method for AB2In another PRAddress precision errors that tend to destroy conservation for constant fieldsIn another PR