-
Notifications
You must be signed in to change notification settings - Fork 188
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
Some CATKE performance optimizations #3453
base: main
Are you sure you want to change the base?
Conversation
disregard the clipping of TKE to zero, it is just a stability test. In general this branch will remain a experimental for a little bit |
…into ss/optimize-catke
Co-authored-by: Gregory L. Wagner <[email protected]>
…into ss/optimize-catke
end | ||
|
||
# Fallbacks for explicit time discretization | ||
# Dissipation: if e is negative treat it implicitly, otherwise explicit (we do not want to subtract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Dissipation: if e is negative treat it implicitly, otherwise explicit (we do not want to subtract | |
# Dissipation: if e is negative treat it explictly, otherwise implicit (we do not want to subtract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah 😅
Adapt.adapt_structure(to, scheme::TracerAdvection{N, FT}) where {N, FT} = | ||
TracerAdvection{N, FT}(Adapt.adapt(to, scheme.x), | ||
Adapt.adapt(to, scheme.y), | ||
Adapt.adapt(to, scheme.z)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this stuff have to do with CATKE performance optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is code from another PR #3404 we have to merge before this one
Q_e = - Cᵂϵ * turbulent_velocityᶜᶜᶜ(i, j, k, grid, closure_ij, tracers.e) / Δz * on_bottom | ||
Q_e = - Cᵂϵ * w★[i, j, k] / Δz * on_bottom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should discuss whether it would be better to keep the design that uses the function turbulent_velocity
. I tried to write this code so that we could experiment with putting TKE at cell interfaces. All of these changes hard code the TKE location, making it harder to modify in the future...
@inbounds begin | ||
S²[i, j, k] = shearᶜᶜᶠ(i, j, k, grid, u, v) | ||
N²[i, j, k] = ∂z_b(i, j, k, grid, buoyancy, tracers) | ||
w★[i, j, k] = turbulent_velocityᶜᶜᶜ(i, j, k, grid, closure, tracers.e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really need to be precomputed? It's just the sqrt of e
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the big one here is precomputing the buoyancy gradient, since that's potentially expensive.
Rather than completely nuking the existing code, it might make sense to generalize it instead so we can avoid this memory allocation if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't even need that extra memory. It can be precomputed inside the _compute_CATKE_diffusivities!
kernel (at centers and faces).
|
||
# "Convective length" | ||
# ℓᶜ ∼ boundary layer depth according to Deardorff scaling | ||
ℓᶜ = Cᶜ * w★³ / (Qᵇ + Qᵇᵋ) | ||
ℓᶜ = ifelse(isnan(ℓᶜ), zero(grid), ℓᶜ) | ||
|
||
# Figure out which mixing length applies | ||
convecting = (Qᵇ > Qᵇᵋ) & (N² < 0) | ||
convecting = (Qᵇ > Qᵇᵋ) & (N²_local < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"local" implies there is such a thing as a "non-local buoyancy gradient". But I don't think that's what you mean..
build up on #3404.