Adds `last_Δt` to `Clock` #3508

jagoosw · 2024-03-13T17:04:39Z

This PR adds last_Δt to Clock to make Δt available in more places (particularly in boundary condition halo fills addressing a challenge in #3482).

glwagner · 2024-03-13T18:52:40Z

I like last better than previous. But it's not perfect because "last" can also mean "final". Can anyone think of a better name?

Also, do we need previous_dt in the time-stepper anymore?

glwagner · 2024-03-13T18:53:23Z

I think we also need to add something to reset!:

Oceananigans.jl/src/Simulations/simulation.jl

Lines 163 to 175 in 3bb62a6

    
           function reset!(sim::Simulation) 
        
               sim.model.clock.time = 0.0 
        
               sim.model.clock.iteration = 0 
        
               sim.model.clock.stage = 1 
        
               sim.stop_iteration = Inf 
        
               sim.stop_time = Inf 
        
               sim.wall_time_limit = Inf 
        
               sim.run_wall_time = 0.0 
        
               sim.initialized = false 
        
               sim.running = true 
        
               reset!(timestepper(sim.model)) 
        
               return nothing 
        
           end

glwagner · 2024-03-13T18:54:16Z

Also

Oceananigans.jl/src/Simulations/simulation.jl

Line 164 in 3bb62a6

sim.model.clock.time = 0.0

should just be

 sim.model.clock.time = 0

(at least --- this will also have to be changed to support dates @navidcy)

src/TimeSteppers/clock.jl

glwagner · 2024-03-13T19:01:13Z

src/Models/ShallowWaterModels/shallow_water_model.jl

@@ -112,7 +112,7 @@ Keyword arguments
 function ShallowWaterModel(;
                           grid,
                           gravitational_acceleration,
-                               clock = Clock{eltype(grid)}(0, 0, 1),
+                               clock = Clock{eltype(grid), eltype(grid)}(0, Inf, 0, 1),


Personally I would add a constructor for Clock that takes one type rather than writing this. But you can decide.

Also note that we might reasonably expect users to provide custom clocks with their own time and iteration --- but that the stage and time-step probably won't commonly be provided when building a clock. So I think it would make more sense to have a syntax that looks like

Clock{TT}(time, iteration, [optional args])

I think the type of the time-step can probably be inferred from the time type TT (ie if the time type is either a floating point or a DateTime or something like that).

Yeah that makes sense

There actually is already a constructor I can just change it a bit to infer the DT

Oceananigans.jl/src/TimeSteppers/clock.jl

Line 27 in 3bb62a6

Clock(; time::T, iteration=0, stage=1) where T = Clock{T}(time, iteration, stage)

It seems like Dates only stores things as integers and is never directly used but the timestep is always eltype(grid) so I'm not sure it can be directly infered.

We don't support dates right now -- to support it, we have to generalize how the time-step specification works.

the timestep is always eltype(grid) so I'm not sure it can be directly infered.

We will have to change how time-step specification works to support DateTime properly.

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

jagoosw · 2024-03-13T20:21:17Z

Okay, so I've got a way we could do this. When a user specifies an AbstractTime we can then use the type they've given to set the timestep type like:

clock = Clock{eltype(grid)}(time = DateTime(2020))

jagoosw · 2024-03-13T20:22:32Z

src/TimeSteppers/clock.jl

 """
-Clock(; time::T, iteration=0, stage=1) where T = Clock{T}(time, iteration, stage)
+Clock(; time::TT, last_Δt::DT=Inf, iteration=0, stage=1) where {TT, DT} = Clock{TT, DT}(time, last_Δt, iteration, stage)
+Clock{TT}(; time, last_Δt=Inf, iteration=0, stage=1) where TT = Clock{TT, TT}(time, last_Δt, iteration, stage)


@glwagner is it too confusing to infer the types like this?

I thikn taht's good

It is a little bit of a conundrum --- I need to understand all the possibilities with DateTime a bit more. I think when we use DateTime, there will be many possibilities for the time-step type (Second, Minute, etc...). But we can figure it out when the time comes though and no need to overworry right now.

Oh I missed the line below

I suggest deleting the function with AbstractTime. But we can put a little place-holder for the future.

BTW julia doesn't support dispatch on keyword argument types

Oh thank you good to know

glwagner · 2024-03-13T22:42:10Z

Okay, so I've got a way we could do this. When a user specifies an AbstractTime we can then use the type they've given to set the timestep type like:
clock = Clock{eltype(grid)}(time = DateTime(2020))

I think it'll work a little differently, because we will want the time-step to also be a period rather than a floating point. We don't support it yet, but we will have to convert the time-step to a floating point somewhere internally. It does look like this will affect the code you are about to write! But don't worry too much --- as long as you write clear, concise code we will be able to change it approprately in the future!

src/TimeSteppers/clock.jl

src/Simulations/simulation.jl

src/Models/HydrostaticFreeSurfaceModels/hydrostatic_free_surface_model.jl

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

glwagner · 2024-03-15T16:17:58Z

Also going to change the title of this PR to use our convention --- try to do that in the future too!

src/TimeSteppers/clock.jl

navidcy

I like this

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

jagoosw · 2024-03-18T17:19:09Z

These test failures are just testing problems not actual code problems right?

glwagner · 2024-03-18T17:33:24Z

I think so, I'll retry them.

glwagner · 2024-03-18T19:54:19Z

we close now

glwagner · 2024-03-18T19:54:52Z

doesn't this mean that we can get rid of previous_dt from QuasiAdamsBashforth2TimeStepper ?

glwagner · 2024-03-18T19:55:04Z

It's fine if we do it in another PR.

jagoosw · 2024-03-18T21:37:27Z

Could you try and run the docs again?

jagoosw · 2024-03-18T21:37:38Z

doesn't this mean that we can get rid of previous_dt from QuasiAdamsBashforth2TimeStepper ?

I would have thought so

src/TimeSteppers/clock.jl

…l into jsw/add-Δt-to-clock

jagoosw added 5 commits March 13, 2024 13:04

added Δt to Clock

ac59cdf

fixed clock to work for DateTime

dfd62df

changed name to last_Δt

99aacbb

fixed Clock adapt

a68c34a

updated docstring

51f792b

jagoosw mentioned this pull request Mar 13, 2024

Open boundary conditions for NonhydrostaticModel #3482

Open

4 tasks

jagoosw marked this pull request as ready for review March 13, 2024 17:09

glwagner reviewed Mar 13, 2024

View reviewed changes

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

glwagner reviewed Mar 13, 2024

View reviewed changes

jagoosw and others added 3 commits March 13, 2024 15:52

Update src/TimeSteppers/clock.jl

1852d42

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

fixed constructor

a83e3bb

updated reset!

40db431

fixed formatting

65068f6

jagoosw commented Mar 13, 2024

View reviewed changes

glwagner reviewed Mar 13, 2024

View reviewed changes

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

glwagner reviewed Mar 13, 2024

View reviewed changes

src/Simulations/simulation.jl Show resolved Hide resolved

glwagner reviewed Mar 13, 2024

View reviewed changes

src/Models/HydrostaticFreeSurfaceModels/hydrostatic_free_surface_model.jl Show resolved Hide resolved

navidcy added the abstractions 🎨 Whatever that means label Mar 14, 2024

Apply suggestions from code review

d5f4916

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

glwagner changed the title ~~Added last_Δt to Clock~~ Adds last_Δt to Clock Mar 15, 2024

glwagner approved these changes Mar 16, 2024

View reviewed changes

navidcy reviewed Mar 16, 2024

View reviewed changes

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

navidcy approved these changes Mar 16, 2024

View reviewed changes

Update src/TimeSteppers/clock.jl

bc3ca25

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

Merge branch 'main' into jsw/add-Δt-to-clock

689c56c

increase size_threshold=1MiB

0170cf0

navidcy reviewed Mar 20, 2024

View reviewed changes

src/TimeSteppers/clock.jl Show resolved Hide resolved

navidcy added 6 commits March 20, 2024 10:36

fix doctest

9820fed

fix doctest

28cc105

Update pipeline.yml

b7d3253

Merge branch 'main' into jsw/add-Δt-to-clock

a48cbb8

fix size_threshold

84c206b

Merge branch 'jsw/add-Δt-to-clock' of github.com:CliMA/Oceananigans.j…

5af5f82

…l into jsw/add-Δt-to-clock

navidcy merged commit e243e5b into main Mar 21, 2024
48 checks passed

navidcy deleted the jsw/add-Δt-to-clock branch March 21, 2024 12:59

francispoulin mentioned this pull request May 13, 2024

error in box.jl example OceanBioME/OceanBioME.jl#175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `last_Δt` to `Clock` #3508

Adds `last_Δt` to `Clock` #3508

jagoosw commented Mar 13, 2024 •

edited

Loading

glwagner commented Mar 13, 2024

glwagner commented Mar 13, 2024

glwagner commented Mar 13, 2024

glwagner Mar 13, 2024

glwagner Mar 13, 2024 •

edited

Loading

glwagner Mar 13, 2024 •

edited

Loading

jagoosw Mar 13, 2024

jagoosw Mar 13, 2024

jagoosw Mar 13, 2024 •

edited

Loading

glwagner Mar 13, 2024

glwagner Mar 13, 2024

jagoosw commented Mar 13, 2024

jagoosw Mar 13, 2024

glwagner Mar 13, 2024

glwagner Mar 13, 2024

glwagner Mar 13, 2024

glwagner Mar 13, 2024

jagoosw Mar 14, 2024

glwagner commented Mar 13, 2024 •

edited

Loading

glwagner commented Mar 15, 2024

navidcy left a comment

jagoosw commented Mar 18, 2024

glwagner commented Mar 18, 2024

glwagner commented Mar 18, 2024

glwagner commented Mar 18, 2024

glwagner commented Mar 18, 2024

jagoosw commented Mar 18, 2024

jagoosw commented Mar 18, 2024

Adds last_Δt to Clock #3508

Adds last_Δt to Clock #3508

Conversation

jagoosw commented Mar 13, 2024 • edited Loading

glwagner commented Mar 13, 2024

glwagner commented Mar 13, 2024

glwagner commented Mar 13, 2024

Choose a reason for hiding this comment

glwagner Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

glwagner Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagoosw Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagoosw commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glwagner commented Mar 13, 2024 • edited Loading

glwagner commented Mar 15, 2024

navidcy left a comment

Choose a reason for hiding this comment

jagoosw commented Mar 18, 2024

glwagner commented Mar 18, 2024

glwagner commented Mar 18, 2024

glwagner commented Mar 18, 2024

glwagner commented Mar 18, 2024

jagoosw commented Mar 18, 2024

jagoosw commented Mar 18, 2024

Adds `last_Δt` to `Clock` #3508

Adds `last_Δt` to `Clock` #3508

jagoosw commented Mar 13, 2024 •

edited

Loading

glwagner Mar 13, 2024 •

edited

Loading

glwagner Mar 13, 2024 •

edited

Loading

jagoosw Mar 13, 2024 •

edited

Loading

glwagner commented Mar 13, 2024 •

edited

Loading