Move AbstractModel interface to top level and add docstrings #4798

bgroenks96 · 2025-09-19T08:37:27Z

Resolves #4797

bgroenks96 · 2025-09-19T08:38:24Z

@glwagner Are you OK with moving the model interface methods out of the Models submodule up to the top-level module?

src/Oceananigans.jl

simone-silvestri · 2025-09-19T09:12:28Z

src/Oceananigans.jl

+
+TODO
+"""
+total_velocities(model::AbstractModel) = nothing


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

Updated in be79014

I don't think "velocities" should be intrinsic to the concept of a model.

I agree, but I didn't want to rock the boat too much.

simone-silvestri · 2025-09-19T09:13:12Z

src/Oceananigans.jl

+Models are required to have the following fields:
+  - `model.clock::Clock`
+  - `model.architecture`
+  - `model.timestepper` with `timestepper.G⁻` and `timestepper.Gⁿ`


I don't think we want to assume a timestepper: the OceanSeaIceModel does not have a timestepper

and timestepper.G⁻ is an AB2-specific field

Removed in be79014

What about the timestepper method?

I don't think we need it. It is used only in the reset!(::AbstractModel) method, which should know wether we have a timestepper or not.

oh, actually it is currently used in the initialize!(simulation) step... But this should call initialize!(model) and not reset!(timestepper(model))

src/Oceananigans.jl

Co-authored-by: Simone Silvestri <[email protected]>

src/TimeSteppers/TimeSteppers.jl

src/Models/Models.jl

glwagner · 2025-09-19T11:34:10Z

I think there needs to be some thought / intentionality about what functions belong to a "generic model" interface, versus what functions are only associated with NonhydrostaticModel / HydrostaticFreeSurfaceModel. Here are some examples

models should not need to have "time steppers". The intrinsic thing is a time stepping rule, ie time_step!(model, dt). It doesn't matter if there is a property model.timestepper.
models should not need to have grids or architectures. Its not well defined in many cases. A coupled model does not have a single grid.

To develop the scope of models I would consider the following cases:

a fluids model, like NonhydrostaticModel (which used to be the only model)
a coupled model, like an EarthSystemModel with a mixture of prescribed and prognostic componets
a 0D model with a very simple evolution rule, ie something like next = prev + dt * exp(next) (exponential growth). This model has neither grid nor architecture, only time_step!(model, dt).

The big danger is that we go in the opposite direction that we have been generally striving for --- a simple, useful interface which is disentangled from the "finite volume" (grids, fields) part of the code.

glwagner · 2025-09-19T11:35:23Z

@glwagner Are you OK with moving the model interface methods out of the Models submodule up to the top-level module?

Does this move us in the direction of being able to disentangle finite volume stuff (grids, fields, etc) from model implementation? What is your vision? Usually moving things to top-level acts to further entangle...

glwagner · 2025-09-19T11:36:09Z

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl

@@ -1,5 +1,5 @@
+using Oceananigans: total_velocities


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...

You mean the method in general, right? Or just the fact that it's at the top-level?

glwagner · 2025-09-19T11:36:59Z

src/TimeSteppers/TimeSteppers.jl

+function initialization_update_state!(model::AbstractModel; kw...) 
+    initialize!(model)
+    update_state!(model; kw...) # fallback
+end


I think we will be able to get rid of initialization_update_state! soon FYI. It was introduced to support work with Reactant but I do not think it is needed anymore.

glwagner · 2025-09-19T11:37:16Z

src/TimeSteppers/TimeSteppers.jl

+
+Returns the `TimeStepper` used by the given `model`.
+"""
+timestepper(model::AbstractModel) = model.timestepper


I'm wary of baking in the notion of a time stepper

True. Is this used anywhere?

glwagner · 2025-09-19T11:53:15Z

I said this in a chat so I'll say it here so we can all discuss --- I think that the "definition" of a model belongs to the Simulations module. This makes sense because the requirements of a model are determined by Simulation and its functions. For example, Simulation requires a model to have a function time_step!(model, dt).

I am not sure about all of the other functions. We have things like initialize!, reset!. Maybe we need finalize!. We definitely don't want any physics assumptions baked in, it total_velocities is bad. I am not even sure that we want to enshrine the notion of "prognostic fields", but that is more debatable. I'd be ok with accepting fields as a requirement.

At the top-level, I think we want to move towards an import order that looks somethin like

Grids, Fields, Solvers, FieldTimeSeries, utils
Simulation, OutputWriters
Models

Then someday we may pull out 1 & 2 into another package. We could also have 3 packages (but that's probably overkill. Then, the "models" will basically be the only part of Oceananigans that remains (which makes sense --- ocean specific stuff).

src/Oceananigans.jl

Co-authored-by: Navid C. Constantinou <[email protected]>

Co-authored-by: Gregory L. Wagner <[email protected]>

Co-authored-by: Navid C. Constantinou <[email protected]>

Move AbstractModel interface to top level and add docstrings

f664e4d

bgroenks96 requested a review from glwagner September 19, 2025 08:37

bgroenks96 added 2 commits September 19, 2025 10:40

Move update_model_field_time_series! back to Models

a13c8de

Fix import errors

a130163

simone-silvestri reviewed Sep 19, 2025

View reviewed changes

src/Oceananigans.jl Outdated Show resolved Hide resolved

simone-silvestri reviewed Sep 19, 2025

View reviewed changes

src/Oceananigans.jl Outdated Show resolved Hide resolved

simone-silvestri reviewed Sep 19, 2025

View reviewed changes

src/Oceananigans.jl Outdated Show resolved Hide resolved

bgroenks96 added 4 commits September 19, 2025 11:21

Move initialization_update_state! to TimeSteppers

7442b40

Call initialize! in initialization_update_state!

ea3bb23

Update docstrings for AbstractModel methods

be79014

Move timestepper method to TimeSteppers

dd526a5

simone-silvestri reviewed Sep 19, 2025

View reviewed changes

src/Oceananigans.jl Outdated Show resolved Hide resolved

Update docstring for total_velocities

f1c6b73

Co-authored-by: Simone Silvestri <[email protected]>

glwagner reviewed Sep 19, 2025

View reviewed changes

src/TimeSteppers/TimeSteppers.jl Outdated Show resolved Hide resolved

glwagner reviewed Sep 19, 2025

View reviewed changes

src/Models/Models.jl Outdated Show resolved Hide resolved

glwagner reviewed Sep 19, 2025

View reviewed changes

bgroenks96 self-assigned this Sep 19, 2025

bgroenks96 added cleanup 🧹 Paying off technical debt abstractions 🎨 Whatever that means documentation 📜 The sacred scrolls labels Sep 19, 2025

navidcy reviewed Sep 19, 2025

View reviewed changes

src/Oceananigans.jl Outdated Show resolved Hide resolved

navidcy reviewed Sep 19, 2025

View reviewed changes

src/Oceananigans.jl Outdated Show resolved Hide resolved

navidcy reviewed Sep 19, 2025

View reviewed changes

src/Oceananigans.jl Outdated Show resolved Hide resolved

bgroenks96 and others added 5 commits September 22, 2025 09:04

Update src/Oceananigans.jl

e70393f

Co-authored-by: Navid C. Constantinou <[email protected]>

Update src/Models/Models.jl

22907ba

Co-authored-by: Gregory L. Wagner <[email protected]>

Update src/TimeSteppers/TimeSteppers.jl

55b2f88

Co-authored-by: Gregory L. Wagner <[email protected]>

Update src/Oceananigans.jl

ba52091

Co-authored-by: Navid C. Constantinou <[email protected]>

Update src/Oceananigans.jl

cb1d5d5

Co-authored-by: Navid C. Constantinou <[email protected]>

Move AbstractModel interface to top level and add docstrings #4798

Are you sure you want to change the base?

Move AbstractModel interface to top level and add docstrings #4798

Uh oh!

Conversation

bgroenks96 commented Sep 19, 2025

Uh oh!

bgroenks96 commented Sep 19, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simone-silvestri Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glwagner commented Sep 19, 2025

Uh oh!

glwagner commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glwagner commented Sep 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

simone-silvestri Sep 19, 2025 •

edited

Loading