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

Compute third stage time-step for RK3 in a way that reduces the accumulation of error #3617

Merged
merged 14 commits into from
Jun 13, 2024

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Jun 6, 2024

Should help address some outstanding issues about error accumulation in model.clock.time, like #3606 and #3056.

@glwagner glwagner requested a review from tomchor June 7, 2024 23:14
Comment on lines +140 to 143
corrected_third_stage_Δt = tⁿ⁺¹ - model.clock.time
tick!(model.clock, corrected_third_stage_Δt)
update_state!(model, callbacks; compute_tendencies)
step_lagrangian_particles!(model, third_stage_Δt)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we tick! with corrected_third_stage_Δt, but other third stage steps (namely we calculate_pressure_correction!, pressure_correct_velocities! and step_langrangian_particles!) use the original third_stage_Δt. I would assume all of them need to use the corrected version, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm we can change that though, though it shouldn't matter because they are indistinguishable except by machine epsilon?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right I did consider this. We can change the pressure correction and lagrangian particles. However, other tracers effectively use the original third stage dt via rk3 substep. So I think it's actually simpler to use the same third_state_dt for everything (tracers, predictor velocity, pressure correction, lagrangian particles), but to simply shave off the error accumulated in the model clock during the substeps

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm we can change that though, though it shouldn't matter because they are indistinguishable except by machine epsilon?

Yeah, it's more a matter of consistency than anything else imo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so my point is that it's more consistent to use the non-corrected time-step everywhere, including rk3substep! (where it is used implicitly), as well as in the pressure correction and Lagrangian particles step. Do you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you're saying. Yes I agree.

@tomchor
Copy link
Collaborator

tomchor commented Jun 8, 2024

As per the example below, this PR seems to resolve #3593

using Oceananigans

grid_base = RectilinearGrid(topology = (Bounded, Periodic, Bounded), size = (16, 20, 4), extent = (800, 1000, 100),)
    
@inline east_wall(x, y, z) = x > 400
grid = ImmersedBoundaryGrid(grid_base, GridFittedBoundary(east_wall))

model = NonhydrostaticModel(grid = grid, timestepper = :RungeKutta3, buoyancy = BuoyancyTracer(), tracers = :b,)

N² = 6e-6
b∞(x, y, z) =* z
set!(model, b=b∞)
    
simulation = Simulation(model, Δt=25, stop_time=1e4,)

using Statistics: std
using Printf
progress_message(sim) = @printf("Iteration: %04d, time: %s, iteration×Δt: %s, std(pNHS) = %.2e\n",
                                iteration(sim), sim.model.clock.time, iteration(sim) * sim.Δt, std(model.pressures.pNHS))
add_callback!(simulation, progress_message, IterationInterval(1))

run!(simulation)

printing, at the last few time-steps:

Iteration: 0397, time: 9925.0, iteration×Δt: 9925.0, std(pNHS) = 5.99e-03
Iteration: 0398, time: 9950.0, iteration×Δt: 9950.0, std(pNHS) = 5.99e-03
Iteration: 0399, time: 9975.0, iteration×Δt: 9975.0, std(pNHS) = 5.99e-03
[ Info: Simulation is stopping after running for 11.645 seconds.
[ Info: Simulation time 2.778 hours equals or exceeds stop time 2.778 hours.
Iteration: 0400, time: 10000.0, iteration×Δt: 10000.0, std(pNHS) = 5.99e-03

I haven't been able to test it yet with more complex simulations though.

@navidcy navidcy added the numerics 🧮 So things don't blow up and boil the lobsters alive label Jun 9, 2024
@navidcy
Copy link
Collaborator

navidcy commented Jun 10, 2024

Interesting! Thanks for this @tomchor

@glwagner
Copy link
Member Author

Does anyone approve?

@glwagner
Copy link
Member Author

glwagner commented Jun 13, 2024

PS I think we still may want something like #3606 . This just makes small time-steps less likely but doesn't change the fact that small time steps are problematic for pressure correction. (Though I'm wondering if they are only problematic when we have a divergent velocity field, which may be a different issue to fix...)

@glwagner glwagner merged commit 9a23b06 into main Jun 13, 2024
46 checks passed
@glwagner glwagner deleted the glw/reduce-time-error-accumulation branch June 13, 2024 19:52
@navidcy
Copy link
Collaborator

navidcy commented Jun 13, 2024

Sorry, I was full in approval inside me the other day when I had a look but then I thought I'd wait for the discussion between you and @tomchor to be resolved.

Merged now! All good! Moving on!

@tomchor
Copy link
Collaborator

tomchor commented Jun 13, 2024

PS I think we still may want something like #3606 . This just makes small time-steps less likely but doesn't change the fact that small time steps are problematic for pressure correction. (Though I'm wondering if they are only problematic when we have a divergent velocity field, which may be a different issue to fix...)

Yes, I'll keep that open. I think we'll have a better idea after we start using this fix and figure out how many of small-step cases it prevents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
numerics 🧮 So things don't blow up and boil the lobsters alive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants