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

Do TimeSteppers belong to Models, or to Simulations? #1175

Closed
glwagner opened this issue Nov 13, 2020 · 8 comments
Closed

Do TimeSteppers belong to Models, or to Simulations? #1175

glwagner opened this issue Nov 13, 2020 · 8 comments
Labels
question 💭 No such thing as a stupid question

Comments

@glwagner
Copy link
Member

glwagner commented Nov 13, 2020

Currently, IncompressibleModel keeps a reference to TimeStepper:

timestepper :: TS # Object containing timestepper fields and parameters

This means that every model has one time stepper, and we use time_step!(model, dt) to advance a model forward.

Yet this is not a necessary entanglement. A perfectly valid time-stepping loop is

for i = 1:100
    time_step!(model, forward_euler_stepper, dt)
    time_step!(model, rk3_stepper, dt)
end

(obviously we don't have a forward Euler time-stepper, which is a separate issue, but hopefully the point is made.)

We can disentangle the timestepper from model by moving the timestepper reference over to Simulation. This would also clean up the interface since we wouldn't need to use symbols to specify the time-stepping method (#1119, this might be a hint that we're on the right track...)

model = IncompressibleModel(...)
simulation = Simulation(model, RungeKutta3TimeStepper(model), kwargs...)

I think there's a conceptual advantage to this too. Simulations manage the creation of a time-series, while a Model is more the idealization of a discrete physical system at a particular moment in time.

To make this change, we first have to checkpoint Simulations rather than Models to support time-steppers that require history like AB2. I think this is something we need anyways.

This is a major API change, though it could be mitigated if we put a default into the Simulation constructor.

@navidcy navidcy added the question 💭 No such thing as a stupid question label Nov 16, 2020
@navidcy
Copy link
Collaborator

navidcy commented Nov 16, 2020

I agree with this point.

But, to play devil's advocate one could argue that the advection scheme should not be part of the model either, no? One could intuitively think that model=PDE and simulation=PDE+numerical schemes required to solve it.

@glwagner
Copy link
Member Author

I think it's useful to think of the "model" as a spatially discrete approximation to the PDE. The simulation sends it barreling forward through time.

@navidcy
Copy link
Collaborator

navidcy commented Nov 16, 2020

Framed this way, it makes absolute sense to include time stepper in simulation.

@francispoulin
Copy link
Collaborator

I am happy to follow this discussion and even though I am very new to the table but will share a perspective.

Outside of the context of Oceananigans, I would think of the collection of PDEs as the physical model, and the method by which we discretize them in both space and time, as the numerical model. In this context, when we say model we mean the spatial discretization of the physical model, which is most of but not quite all of the numerical model. The fact that we don't have Advection in the Models folder is consistent and a good idea.

It makes sense to me to put the time stepping in simulations since that is the part where we turn the crank and find the solutions to the prognostic variables.

I suspect the user might benefit from a little bit of discusson on this in the docs so that they can better understand the framework you've set up, which works great. This is of course something to be done when you have spare time, if such a thing really exists. ;)

@glwagner
Copy link
Member Author

glwagner commented Nov 17, 2020

Well, right now, the Model includes both discrete and continuous aspects of the PDE. Model also stores architecture, for example, which is unrelated to the PDE.

We did consider an abstraction that represented the equation set independent from discretization. This would not be a replacement for model, but would simply be a way to organize some of the properties of model (eg buoyancy, coriolis, diffusion, etc). Yet this is a challenging abstraction to design. One issue is specification. If we try to separate the terms from their numerical implementation, we have to figure how to distinguish between "diffusion with second-order differences" and "diffusion with fourth-order differences". It's clearly possible to do this, but it's going to take some time and careful thought to implement. We've taken a more incremental approach to development the model instead, resulting in a fairly "flat" interface to IncompressibleModel that combines aspects both of the continuous equations, their discretization, and things like architecture, the pressure_solver, etc.

I think flat is simple (think parameter files as the platonic ideal of a flat API); however, we still want to benefit from modularity where it's a simple change that doesn't overcomplicate the interface (hence this issue).

@francispoulin
Copy link
Collaborator

That makes sense and I am a firm believer in starting simple, and I'm glad you made that choice.

@navidcy
Copy link
Collaborator

navidcy commented Nov 17, 2020

"Platonic ideal" :)

@glwagner glwagner mentioned this issue Dec 2, 2020
8 tasks
@glwagner
Copy link
Member Author

I'm closing this issue because I'm judging that it's not of current, timely relevance to Oceananigans development. If you would like to make it a higher priority or if you think the issue was closed in error please feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question 💭 No such thing as a stupid question
Projects
None yet
Development

No branches or pull requests

3 participants