-
Notifications
You must be signed in to change notification settings - Fork 195
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
Yet another ZStar implementation #3956
base: main
Are you sure you want to change the base?
Conversation
…jl into ss/new-bottom-height
@inline function upwinded_divergence_flux_Uᶠᶜᶜ(i, j, k, grid, scheme::VectorInvariantCrossVerticalUpwinding, u, v) | ||
@inbounds û = u[i, j, k] | ||
δ_stencil = scheme.upwinding.divergence_stencil | ||
|
||
δᴿ = _biased_interpolate_xᶠᵃᵃ(i, j, k, grid, scheme, scheme.divergence_scheme, bias(û), flux_div_xyᶜᶜᶜ, δ_stencil, u, v) | ||
δᴿ = _biased_interpolate_xᶠᵃᵃ(i, j, k, grid, scheme, scheme.divergence_scheme, bias(û), flux_div_xyᶜᶜᶜ, δ_stencil, u, v) | ||
∂ts = _symmetric_interpolate_xᶠᵃᵃ(i, j, k, grid, scheme, cross_scheme, V_times_∂t_σ) |
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.
∂ts = _symmetric_interpolate_xᶠᵃᵃ(i, j, k, grid, scheme, cross_scheme, V_times_∂t_σ) | |
∂t_σ = _symmetric_interpolate_xᶠᵃᵃ(i, j, k, grid, scheme, cross_scheme, V_times_∂t_σ) |
Is this 0 by default?
end | ||
|
||
@inline function upwinded_divergence_flux_Vᶜᶠᶜ(i, j, k, grid, scheme::VectorInvariantCrossVerticalUpwinding, u, v) | ||
@inbounds v̂ = v[i, j, k] | ||
δ_stencil = scheme.upwinding.divergence_stencil | ||
|
||
δᴿ = _biased_interpolate_yᵃᶠᵃ(i, j, k, grid, scheme, scheme.divergence_scheme, bias(v̂), flux_div_xyᶜᶜᶜ, δ_stencil, u, v) | ||
δᴿ = _biased_interpolate_yᵃᶠᵃ(i, j, k, grid, scheme, scheme.divergence_scheme, bias(v̂), flux_div_xyᶜᶜᶜ, δ_stencil, u, v) | ||
∂ts = _symmetric_interpolate_yᵃᶠᵃ(i, j, k, grid, scheme, cross_scheme, V_times_∂t_σ) |
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.
∂ts = _symmetric_interpolate_yᵃᶠᵃ(i, j, k, grid, scheme, cross_scheme, V_times_∂t_σ) | |
∂t_σ = _symmetric_interpolate_yᵃᶠᵃ(i, j, k, grid, scheme, cross_scheme, V_times_∂t_σ) |
@inline δy_V(i, j, k, grid, u, v) = δyᶜᶜᶜ(i, j, k, grid, Ay_qᶜᶠᶜ, v) | ||
|
||
@inline δx_U_plus_metric(i, j, k, grid, u, v) = δxᶜᶜᶜ(i, j, k, grid, Ax_qᶠᶜᶜ, u) + V_times_∂t_σ(i, j, k, grid) | ||
@inline δy_V_plus_metric(i, j, k, grid, u, v) = δyᶜᶜᶜ(i, j, k, grid, Ay_qᶜᶠᶜ, v) + V_times_∂t_σ(i, j, k, grid) |
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 "times" mean? Like multiplicatin?
I think just V_∂t_σ
is good enough. It's better to choose either math notation or english, but not mix the two.
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.
Also how does V
get in here if this function just depends on i, j, k, grid
#### | ||
#### ZStarVerticalCoordinate | ||
#### |
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.
#### | |
#### ZStarVerticalCoordinate | |
#### | |
##### | |
##### ZStarVerticalCoordinate | |
##### |
σᶠᶜⁿ = new_data(FT, arch, (Face, Center, Nothing), args...) | ||
σᶜᶠⁿ = new_data(FT, arch, (Center, Face, Nothing), args...) | ||
σᶠᶠⁿ = new_data(FT, arch, (Face, Face, Nothing), args...) | ||
ηⁿ = new_data(FT, arch, (Center, Center, Nothing), args...) |
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.
ηⁿ = new_data(FT, arch, (Center, Center, Nothing), args...) | |
ηⁿ = new_data(FT, arch, (Center, Center, Nothing), args...) |
I'm finding a lot of little typos, is this ready / has been proofread? |
Refactoring
Following #3411 it is clear that the grids require some refactor to allow ZStar.
This PR implements another proposal for ZStar which hinges on changing the grids to have just one
z
field.The case of a static z (the only allowed in Oceananigans at the moment) is covered with a
StaticVerticalCoordinate
type that contains all the specifications that were in the grid before:Oceananigans.jl/src/Grids/vertical_coordinate.jl
Lines 14 to 19 in 9e05e21
This refactor should change nothing in the user interface nor the internals of the code, since the whole API should depend on
znode
andΔz
, but just reorganizes the vertical coordinate in a compact type that can be modified.ZStar Implementation
With this refactor, a ZStar coordinate is implemented through a
ZStarVerticalCoordinate
typeOceananigans.jl/src/Grids/vertical_coordinate.jl
Lines 36 to 48 in 9e05e21
e₃
is the vertical scaling of the grid spacing defined aswith$z$ the actual spatial coordinate (moving with the free surface) and $r$ is the reference vertical coordinate also called $z^\star$ (equivalent to a static coordinate, or $z$ when $\eta = 0$ ).
ηⁿ
is the value of the free surface at the current time step, needed to calculateznodes
Finally$H$ is the output of $z$ properties ($r$ ($z$ properties for a zstar vertical coordinate are calculated appropriately in the Operators module (https://github.com/CliMA/Oceananigans.jl/blob/ss/new-zstar/src/Operators/variable_grid_operators.jl)
static_column_depth
.As a consequence of the definitions, all
znodes
,zspacings
,Δz
, etc...) are replaced withrnodes
,rspacings
,Δr
, etc...) and theAn example of the proposed user interface is shown in the
validation/z_star_coordinate
examplesChanges in the model timestepping
Another requirement is changing the internals of the
HydrostaticFreeSurfaceModel
to solve the correct equations, in particular, the notable changes are:(1) Updating the grid at each timestep in the
update_state!
step(2) including the gradient of the grid in the momentum equations, calculated as:
Oceananigans.jl/src/Models/HydrostaticFreeSurfaceModels/z_star_vertical_spacing.jl
Lines 85 to 100 in 9e05e21
(3) Changing the computation of the vertical velocity to include the movement of the grid
Oceananigans.jl/src/Models/HydrostaticFreeSurfaceModels/compute_w_from_continuity.jl
Lines 22 to 35 in 9e05e21
(4) Advancing$e_3\theta$ instead of $\theta$ in the tracer equations and subsequently unscale it back after the grid scaling factor at the new time step $e_3^{n+1}$ is known
Oceananigans.jl/src/Models/HydrostaticFreeSurfaceModels/hydrostatic_free_surface_ab2_step.jl
Lines 111 to 128 in 9e05e21
(5) Adding a non-linear free surface by changing the
static_column_depth
in the split explicit free surface implementation to adynamic_column_depth
defined asOceananigans.jl/src/ImmersedBoundaries/zstar_immersed_grid.jl
Lines 14 to 17 in 9e05e21
Note: these changes are valid only for the QuasiAdamsBashort2 timestepper. Support for the SplitRungeKutta3 timestepper requires a bit more work and can be done when all the infrastructure is in place
OutputWriters?
The last piece of the puzzle (not implemented in this PR) would be to change the
OutputWriters
to include by default the variable grid properties in thetimeseries
field of the jld2 / netcdf writer.Possibility for improvements
I got a bit carried away and completed a working implementation to make sure it could be done this way, but I am happy to change just about everything in here. I would like to know what people think about this implementation and what people suggest especially in terms of
(1) Implementation
(2) Variable naming
I can also split this PR in a couple of ones, probably one that includes the refactor to the grids (implementation of a
StaticVerticalCoordinate
) and one that implements a working version of aZStarVerticalCoordinate
.