Allow Tuple of closures with CATKE

[simone-silvestri]

in this way we don't timestep the energy equation twice within a time step when using a tuple of closures containing CATKE

[ali-ramadhan]

Nice! Was this a numerical bug, or is it just a performance issue as the :e tendency term was being computed twice? I assume it's done somewhere in the TurbulenceClosures module.

[simone-silvestri]

I think it is a bug because the tracer is advanced in the TurbulenceClosures module here

Oceananigans.jl/src/TurbulenceClosures/turbulence_closure_implementations/TKEBasedVerticalDiffusivities/time_step_catke_equation.jl

Line 12 in fe4123f

function time_step_catke_equation!(model)

So with closure tuple it is advanced twice

[simone-silvestri]

Should I add a test for it?

[ali-ramadhan]

Always good to add a test! Especially since this issue might come up again in the future with refactors.

I'm just trying to think of a good test... Are there analytic solutions to the TKE where we can take one time step and make sure the TKE values are correct (and that only one, not two, time steps were taken)?

As a side note: I've been running with CATKE in a tuple of closures and I encountered NaNs at the bottom-most grid cell but the simulations seems to continue time stepping just fine. Maybe it's related to this bug fix?

[simone-silvestri]

We can also brutally just check that we do not step the kinetic energy twice:
i.e that

eⁿ⁺¹ == eⁿ + Δt * Gᵉ # we have to account for the fast parts that depend on uⁿ⁺¹ here

and not

eⁿ⁺¹ == eⁿ + 2 * Δt * Gᵉ 

[glwagner]

We can also brutally just check that we do not step the kinetic energy twice: i.e that

eⁿ⁺¹ == eⁿ + Δt * Gᵉ # we have to account for the fast parts that depend on uⁿ⁺¹ here

and not

eⁿ⁺¹ == eⁿ + 2 * Δt * Gᵉ 

We do this elsewhere, eg for test_forcings.jl, I think adding a forcing test with CATKE + e would be quick and easy.