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

Make RK3 time-stepping compatible with CATKE #4019

Closed
wants to merge 20 commits into from

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Dec 20, 2024

This implementation may not be correct.

The sub stepping scheme is in principle different than the baroclinic stepping scheme so I am not extremely sure this method works. I ll think a bit more about it.

We actually need to implement it like we did for the SplitExplicitFreeSurface, by saving down the slow tendencies, and averaging them in the last step. All stages should go from n to n+1.

@simone-silvestri simone-silvestri marked this pull request as draft December 20, 2024 13:59
@glwagner
Copy link
Member

You could also update how CATKE works so that things are correct when you aren't using substepping. And then just only support substepping as time allows

@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Dec 21, 2024

Sounds good. I have implemented this strategy for the moment (just revert to the standard tracer time stepping if tke_time_step == nothing), so we can test how CATKE performs with RK3. I suspect it will be better than AB2 and maybe we don't need substepping after all.

const FlavorOfCATKE{TD} = Union{CATKEVD{TD}, CATKEVDArray{TD}} where TD
const CATKEVD{TD} = CATKEVerticalDiffusivity{TD, <:Any, <:Any, DT} where {TD, DT}
const CATKEVDArray{TD, DT} = AbstractArray{<:CATKEVD{TD, DT}} where {TD, DT}
const FlavorOfCATKE{TD, DT} = Union{CATKEVD{TD, DT}, CATKEVDArray{TD, DT}} where TD, DT
Copy link
Member

Choose a reason for hiding this comment

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

it's kind of a mess, is it worth it?

@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Dec 23, 2024

Right. It seems a bit complicated to make it work like this.
Probably we need a refactor and we need find an abstraction for the time stepping details of the TKE.
I can close this now and we can think about how to generalize this substepping later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants