-
Notifications
You must be signed in to change notification settings - Fork 191
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
Integrate finite volume operators #529
Conversation
Possibly redundant. Will discuss in PR.
@glwagner Do you think you'll have some time to review this PR this week? It affects a lot of code you've written and is a bit of a bottleneck as it refactors a lot of the existing code. |
Codecov Report
@@ Coverage Diff @@
## master #529 +/- ##
==========================================
+ Coverage 62.6% 67.65% +5.04%
==========================================
Files 70 69 -1
Lines 2054 1954 -100
==========================================
+ Hits 1286 1322 +36
+ Misses 768 632 -136
Continue to review full report at Codecov.
|
src/TurbulenceClosures/turbulence_closure_implementations/anisotropic_biharmonic_diffusivity.jl
Show resolved
Hide resolved
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.
Looks great! My only concern is the name of viscous and diffusive flux divergence functions. We should make sure these names are non-confusing in the case of tensor diffusivities and viscosities.
+ closure.νv * ∂z²_aaf(i, j, k, grid, U.w) | ||
) | ||
@inline ∂ⱼ_2ν_Σ₃ⱼ(i, j, k, grid, closure::ConstantAnisotropicDiffusivity, U, args...) = | ||
div_ν∇w(i, j, k, grid, closure.νh, closure.νh, closure.νv, U.w) |
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.
I'm a bit confused by this notation. We have a tensor diffusivity here, which means we cannot write this operator as "the divergence of ν∇w
". Why is this name for the operator preferred, when it does not correctly reflect the mathematical operation? Is there some kind of benefit to this shorthand? I feel the explicit original versions are easier to read and understand.
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's a good point. Originally I just wanted to get rid of things like ∇_κ∇c
as it looked like a "gradient of a gradient". Indeed div_ν∇w
is quite specific so I'll change them to use names like ∂ⱼνᵢⱼ∂ᵢw
.
|
||
which will end up at the location `ccf`. | ||
""" | ||
@inline function div_ν∇w(i, j, k, grid, νˣ, νʸ, νᶻ, w) |
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.
I feel the name of this function is confusing. The operation is represents is
∂ⱼ νᵢⱼ ∂ᵢ w
where νᵢⱼ
is a diagonal tensor. However, the notation div_ν∇w
, implies, at least to me, the divergence of a scalar (isotropic) diffusivity times the gradient of vertical velocity.
Maybe we don't have to resolve this here, but we should be careful that the names of our functions clearly reflect the mathematical objects they implement.
This PR integrates the finite volume operators introduced in PR #283. This includes merging
closure_operators.jl
intoOceananigans.Operators
. All tests pass but there's quite a bit of cleanup to be done.@glwagner This PR affects a lot of code you've written so we should probably work on merging this PR together. Now that tests pass we can focus on cleanup.
I still need to convert the biharmonic operators and
leith_enstrophy_diffusivity.jl
closure to finite volume. I'll also add a test for Leith.There are regression tests for constant diffusivty, Smagorinsky, and AMD, but not for other closures, so we should probably be careful that closures with no regression test remain unchanged.
Some comments:
The
norm
functions defined invelocity_tracer_gradients.jl
https://github.com/climate-machine/Oceananigans.jl/blob/2bb32015e64bff830f2233d588360ccc6b8605b4/src/TurbulenceClosures/velocity_tracer_gradients.jl#L130-L154 all make use of functions likeΔᶠx_ffc
but they are defined in an@eval
loop in inverstappen_anisotropic_minimum_dissipation.jl
. We should probably make it clearer that these functions are closure-specific.In
smagorinsky_lilly.jl
, theκ_∂x_c
,κ_∂y_c
, andκ_∂z_c
functions useℑxᶠᵃᵃ(i, j, k, grid, νₑ, closure)
however I'm pretty sure we can just useℑxᶠᵃᵃ(i, j, k, grid, νₑ)
here. If so, we can get rid of theℑxᶠᵃᵃ(i, j, k, grid::AG{FT}, c, args...)
function definitions.Certain functions are shared between turbulence closures, e.g.
ΣᵢⱼΣᵢⱼᶜᶜᶜ
is used by bothsmagorinsky_lilly.jl
andblasius_smagorinsky.jl
. Should they be moved toclosure_operators.jl
?Also, VAMD uses
Δᶠxᶜᶜᶜ
for the filter widths while Rozema AMD usesΔx
(which looks like the regularΔx
used byOceananigans.Operators
). We should probably change all filter widths to useΔᶠ
.Smagorinsky-Lilly uses
Δᶠ_ccc
for filter widths. Switching toΔᶠᶜᶜᶜ
is probably a bad idea asᶠ
is denotes face. Should we change them toΔᶠxᶜᶜᶜ = Δᶠyᶜᶜᶜ = Δᶠzᶜᶜᶜ = Δᶠ_ccc
?I could never find a symbol like
\cdot
for divergences that can be used in function names. What do you think of renaming function names like∇_κ_∇c
todiv_κ_∇c
?▶
is used in a lot of places in theAbstractOperators
module. They should probably be changed toℑ
for consistency, but since it's all local to AbstractOperators, I'm leave the decision to @glwagner.Some changes we need to make for vertically stretched grids:
apply_z_top_bc!
andapply_z_bottom_bc!
to use ΔzC or ΔzF based on the field, but this would only apply tow
for which you cannot use flux boundary conditions for z, so maybe it can always just use ΔzF (spacing between faces) and we can keep the one version._fill_top_halo!
and_fill_bottom_halo!
.I'll release v0.16 once this is merged as JULES.jl depends on these finite volume operators.