-
Notifications
You must be signed in to change notification settings - Fork 250
Move AbstractModel interface to top level and add docstrings #4798
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
base: main
Are you sure you want to change the base?
Changes from all commits
f664e4d
a13c8de
a130163
7442b40
ea3bb23
be79014
dd526a5
f1c6b73
e70393f
22907ba
55b2f88
ba52091
cb1d5d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,9 @@ export | |
| ConservativeFormulation, VectorInvariantFormulation, | ||
| PressureField, fields, ZCoordinate, ZStarCoordinate, | ||
|
|
||
| # Model interface | ||
| iteration, timesteppper, | ||
|
|
||
| # Hydrostatic free surface model stuff | ||
| VectorInvariant, ExplicitFreeSurface, ImplicitFreeSurface, SplitExplicitFreeSurface, | ||
| HydrostaticSphericalCoriolis, PrescribedVelocityFields, | ||
|
|
@@ -92,7 +95,7 @@ export | |
| Clock, TimeStepWizard, conjure_time_step_wizard!, time_step!, | ||
|
|
||
| # Simulations | ||
| Simulation, run!, Callback, add_callback!, iteration, | ||
| Simulation, run!, Callback, add_callback!, | ||
| iteration_limit_exceeded, stop_time_exceeded, wall_time_limit_exceeded, | ||
|
|
||
| # Diagnostics | ||
|
|
@@ -162,9 +165,52 @@ const defaults = Defaults() | |
| AbstractModel | ||
|
|
||
| Abstract supertype for models. | ||
|
|
||
| Models are required to have the following fields: | ||
| - `model.clock::Clock` | ||
| - `model.grid::AbstractGrid` | ||
| """ | ||
| abstract type AbstractModel{TS, A} end | ||
|
|
||
| # Method interface for AbstractModel | ||
|
|
||
| """ | ||
| $SIGNATURES | ||
|
|
||
| Return the current iteration of the `model.clock`. | ||
| """ | ||
| iteration(model::AbstractModel) = model.clock.iteration | ||
|
|
||
| """ | ||
| $SIGNATURES | ||
|
|
||
| Ensure consistency between internal variables in the `AbstractModel` and user-specified | ||
| initial conditions provided by via `set(::AbstractModel; kwargs...)`. | ||
| Default implementation does nothing. | ||
| """ | ||
| initialize!(model::AbstractModel) = nothing | ||
|
|
||
| """ | ||
| $SIGNATURES | ||
|
|
||
| Returns the sum of the user-prescribed background + prognostic velocities, where applicable. | ||
| """ | ||
| total_velocities(model::AbstractModel) = nothing | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really well defined at the moment. It returns the sum of background + prognostic velocities for the non hydrostatic model and the prognostic velocities in the hydrostatic model There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in be79014 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think "velocities" should be intrinsic to the concept of a model. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but I didn't want to rock the boat too much. |
||
|
|
||
| """ | ||
| Base.time(model::AbstractModel) | ||
|
|
||
| Return the current `time` from the `model.clock`. | ||
| """ | ||
| Base.time(model::AbstractModel) = model.clock.time | ||
|
|
||
| """ | ||
| Base.eltype(model::AbstractModel) | ||
|
|
||
| Return the numeric `eltype` of the `model.grid` and all associated `Field`s. | ||
| """ | ||
| Base.eltype(model::AbstractModel) = eltype(model.grid) | ||
|
|
||
| """ | ||
| AbstractDiagnostic | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,12 @@ export | |
| RungeKutta3TimeStepper, | ||
| SplitRungeKutta3TimeStepper, | ||
| time_step!, | ||
| timestepper, | ||
| Clock, | ||
| tendencies | ||
|
|
||
| using DocStringExtensions | ||
|
|
||
| using KernelAbstractions | ||
| using Oceananigans: AbstractModel, initialize!, prognostic_fields | ||
| using Oceananigans.Architectures: device | ||
|
|
@@ -24,6 +27,24 @@ function update_state! end | |
| function compute_tendencies! end | ||
| function compute_flux_bc_tendencies! end | ||
|
|
||
| """ | ||
| $SIGNATURES | ||
|
|
||
| Define initialization routines for the first call to `update_state!` after initialization. | ||
| Defaults to invoking `update_state!(model; kw...)`. | ||
| """ | ||
| function initialization_update_state!(model::AbstractModel; kw...) | ||
| initialize!(model) | ||
| update_state!(model; kw...) # fallback | ||
| end | ||
|
Comment on lines
+36
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we will be able to get rid of |
||
|
|
||
| """ | ||
| $SIGNATURES | ||
|
|
||
| Returns the `TimeStepper` used by the given `model`. | ||
| """ | ||
| timestepper(model::AbstractModel) = model.timestepper | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wary of baking in the notion of a time stepper There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. Is this used anywhere? |
||
|
|
||
| compute_pressure_correction!(model, Δt) = nothing | ||
| make_pressure_correction!(model, Δt) = nothing | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wary of this. It's a hack to begin with and this sort of cements it. I think we want to go the other way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the method in general, right? Or just the fact that it's at the top-level?