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

(0.95.6) Correctly account for field names in ContinuousBoundaryFunctions #4008

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
193efb7
add masking
simone-silvestri Dec 17, 2024
1c20a5b
also xy fields
simone-silvestri Dec 17, 2024
d1b83b4
improvements
simone-silvestri Dec 18, 2024
ad802d9
add bump
simone-silvestri Dec 18, 2024
7b7639d
Merge branch 'main' into ss/correct-continuous-boundary-functions
simone-silvestri Dec 18, 2024
999b195
bump
simone-silvestri Dec 18, 2024
a861a37
comment
simone-silvestri Dec 18, 2024
0e7dc4a
small correction
simone-silvestri Dec 18, 2024
1d02b9b
couple of corrections
simone-silvestri Dec 18, 2024
d61187e
better comment
simone-silvestri Dec 18, 2024
44fdd9f
correct model_fields
simone-silvestri Dec 18, 2024
1b90451
using field names
simone-silvestri Dec 18, 2024
13d6d57
bugfix
simone-silvestri Dec 18, 2024
fd90c83
validate closure
simone-silvestri Dec 18, 2024
6ae4a6d
whoops not here
simone-silvestri Dec 18, 2024
5a26ecf
not here
simone-silvestri Dec 18, 2024
6f28d5f
add new design
simone-silvestri Dec 18, 2024
b591b45
add tests
simone-silvestri Dec 18, 2024
6ab2279
correction
simone-silvestri Dec 19, 2024
6861ae5
Merge branch 'main' into ss/correct-continuous-boundary-functions
simone-silvestri Dec 19, 2024
b511529
correct
simone-silvestri Dec 22, 2024
39bcdca
some corrections
simone-silvestri Dec 23, 2024
c2da5ee
this should work now
simone-silvestri Dec 23, 2024
19ad053
probably we need to inline
simone-silvestri Dec 23, 2024
f975fc0
bump
simone-silvestri Dec 23, 2024
d9c3177
Merge branch 'main' into ss/correct-continuous-boundary-functions
simone-silvestri Dec 23, 2024
7644409
just do an issue
simone-silvestri Dec 23, 2024
52c248a
Merge branch 'ss/correct-continuous-boundary-functions' of github.com…
simone-silvestri Dec 23, 2024
ddfdab0
Merge branch 'main' into ss/correct-continuous-boundary-functions
navidcy Dec 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "Oceananigans"
uuid = "9e8cae18-63c1-5223-a75c-80ca9d6e9a09"
authors = ["Climate Modeling Alliance and contributors"]
version = "0.95.5"
version = "0.95.6"

[deps]
Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e"
Expand Down
2 changes: 1 addition & 1 deletion src/Biogeochemistry.jl
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ update_biogeochemical_state!(bgc, model) = nothing
"""
AbstractBiogeochemistry

Abstract type for biogeochemical models. To define a biogeochemcial relaionship
Abstract type for biogeochemical models. To define a biogeochemcial relationship
the following functions must have methods defined where `BiogeochemicalModel`
is a subtype of `AbstractBioeochemistry`:

Expand Down
4 changes: 2 additions & 2 deletions src/BoundaryConditions/continuous_boundary_function.jl
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ The regularization of `bc.condition::ContinuousBoundaryFunction` requries
of the boundary.
"""
function regularize_boundary_condition(bc::BoundaryCondition{C, <:ContinuousBoundaryFunction},
grid, loc, dim, Side, prognostic_field_names) where C
grid, loc, dim, Side, field_names) where C

boundary_func = bc.condition

Expand All @@ -81,7 +81,7 @@ function regularize_boundary_condition(bc::BoundaryCondition{C, <:ContinuousBoun

indices, interps = index_and_interp_dependencies(LX, LY, LZ,
boundary_func.field_dependencies,
prognostic_field_names)
field_names)

regularized_boundary_func = ContinuousBoundaryFunction{LX, LY, LZ, Side}(boundary_func.func,
boundary_func.parameters,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ Return a flattened `NamedTuple` of the fields in `model.velocities`, `model.free
`model.tracers`, and any auxiliary fields for a `HydrostaticFreeSurfaceModel` model.
"""
@inline fields(model::HydrostaticFreeSurfaceModel) =
merge(hydrostatic_fields(model.velocities, model.free_surface, model.tracers),
merge(hydrostatic_fields(model.velocities, model.free_surface, model.tracers),
model.auxiliary_fields,
biogeochemical_auxiliary_fields(model.biogeochemistry))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am a bit confused about this function. I think that the biogeochemical auxiliary fields have been added to the auxiliary_fields during model construction, so we shouldn't need an extra biogeochemical_auxiliary_fields(model.biogeochemistry) here right? @jagoosw is it true?

Copy link
Member

@glwagner glwagner Dec 18, 2024

Choose a reason for hiding this comment

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

no, the biogeochemistry struct is allowed to have extra auxiliary fields that are not attached to the model. Basically model.auxiliary_fields are purely for the user, whereas biogeochemistry can have its own auxiliary fields, much like the closure has auxiliary fields that are stored in diffusivity_fields (which could be called closure_auxiliary_fields).

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Dec 23, 2024

Choose a reason for hiding this comment

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

Then I guess the implementation is bugged because It actually looks like the fields are added to the auxiliary_fields, and, thus they are accounted for twice in the fields function

biogeochemical_fields = merge(auxiliary_fields, biogeochemical_auxiliary_fields(biogeochemistry))
tracers, auxiliary_fields = validate_biogeochemistry(tracers, biogeochemical_fields, biogeochemistry, grid, clock)


Expand Down Expand Up @@ -107,20 +107,25 @@ Return a flattened `NamedTuple` of the prognostic fields associated with `Hydros
v = velocities.v,
w = velocities.w),
tracers,
(; η = free_surface.η))
(; η = free_surface.η,
U = nothing,
V = nothing))

@inline hydrostatic_fields(velocities, free_surface::SplitExplicitFreeSurface, tracers) = merge((u = velocities.u,
v = velocities.v,
w = velocities.w,
η = free_surface.η,
U = free_surface.barotropic_velocities.U,
V = free_surface.barotropic_velocities.V),
tracers)
w = velocities.w),
tracers,
(; η = free_surface.η,
U = free_surface.barotropic_velocities.U,
V = free_surface.barotropic_velocities.V))

@inline hydrostatic_fields(velocities, ::Nothing, tracers) = merge((u = velocities.u,
v = velocities.v,
w = velocities.w),
tracers)
tracers,
(; η = nothing,
U = nothing,
V = nothing))

displacement(free_surface) = free_surface.η
displacement(::Nothing) = nothing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ function split_explicit_substepping(cfl, ::Nothing, fixed_Δt, grid, averaging_k
return substepping
end

# Disambiguation for default `SplitExplicitFreeSurface` constructors
split_explicit_substepping(::Nothing, ::Nothing, ::Nothing, grid, averaging_kernel, gravitational_acceleration) =
split_explicit_substepping(nothing, MINIMUM_SUBSTEPS, nothing, grid, averaging_kernel, gravitational_acceleration)

# TODO: When open boundary conditions are online
# We need to calculate the barotropic boundary conditions
# from the baroclinic boundary conditions by integrating the BC upwards
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ function HydrostaticFreeSurfaceModel(; grid,

# Validate biogeochemistry (add biogeochemical tracers automagically)
tracers = tupleit(tracers) # supports tracers=:c keyword argument (for example)
biogeochemical_fields = merge(auxiliary_fields, biogeochemical_auxiliary_fields(biogeochemistry))
tracers, auxiliary_fields = validate_biogeochemistry(tracers, biogeochemical_fields, biogeochemistry, grid, clock)
biogeochemical_fields = biogeochemical_auxiliary_fields(biogeochemistry)
tracers, biogeochemical_fields = validate_biogeochemistry(tracers, biogeochemical_fields, biogeochemistry, grid, clock)

# Reduce the advection order in directions that do not have enough grid points
@apply_regionally momentum_advection = validate_momentum_advection(momentum_advection, grid)
Expand Down Expand Up @@ -158,14 +158,17 @@ function HydrostaticFreeSurfaceModel(; grid,
extract_boundary_conditions(diffusivity_fields))

# Next, we form a list of default boundary conditions:
prognostic_field_names = (:u, :v, :w, tracernames(tracers)..., :η, keys(auxiliary_fields)...)
default_boundary_conditions = NamedTuple{prognostic_field_names}(Tuple(FieldBoundaryConditions()
for name in prognostic_field_names))
field_names = (:u, :v, :w, tracernames(tracers)..., :η, :U, :V,
keys(auxiliary_fields)...,
keys(biogeochemical_auxiliary_fields(biogeochemistry))...)

default_boundary_conditions = NamedTuple{field_names}(Tuple(FieldBoundaryConditions()
for name in field_names))

# Then we merge specified, embedded, and default boundary conditions. Specified boundary conditions
# have precedence, followed by embedded, followed by default.
boundary_conditions = merge(default_boundary_conditions, embedded_boundary_conditions, boundary_conditions)
boundary_conditions = regularize_field_boundary_conditions(boundary_conditions, grid, prognostic_field_names)
boundary_conditions = regularize_field_boundary_conditions(boundary_conditions, grid, field_names)

# Finally, we ensure that closure-specific boundary conditions, such as
# those required by CATKEVerticalDiffusivity, are enforced:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ implicitly during time-stepping.
- ∂xᶠᶜᶜ(i, j, k, grid, hydrostatic_pressure_anomaly)
- ∂ⱼ_τ₁ⱼ(i, j, k, grid, closure, diffusivities, clock, model_fields, buoyancy)
- immersed_∂ⱼ_τ₁ⱼ(i, j, k, grid, velocities, u_immersed_bc, closure, diffusivities, clock, model_fields)
+ forcing(i, j, k, grid, clock, hydrostatic_prognostic_fields(velocities, free_surface, tracers)))
+ forcing(i, j, k, grid, clock, model_fields))
end

"""
Expand Down Expand Up @@ -120,7 +120,9 @@ where `c = C[tracer_index]`.
forcing) where tracer_index

@inbounds c = tracers[tracer_index]
model_fields = merge(hydrostatic_fields(velocities, free_surface, tracers), auxiliary_fields)
model_fields = merge(hydrostatic_fields(velocities, free_surface, tracers),
auxiliary_fields,
biogeochemical_auxiliary_fields(biogeochemistry))

biogeochemical_velocities = biogeochemical_drift_velocity(biogeochemistry, val_tracer_name)
closure_velocities = closure_turbulent_velocity(closure, diffusivities, val_tracer_name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,8 @@
##### There's no free surface with a rigid lid
#####

@inline materialize_free_surface(::Nothing, velocities, grid) = nothing

@inline explicit_barotropic_pressure_x_gradient(i, j, k, grid, ::Nothing) = zero(grid)
@inline explicit_barotropic_pressure_y_gradient(i, j, k, grid, ::Nothing) = zero(grid)

Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ extract_boundary_conditions(::PrescribedVelocityFields) = NamedTuple()
free_surface_displacement_field(::PrescribedVelocityFields, ::Nothing, grid) = nothing
HorizontalVelocityFields(::PrescribedVelocityFields, grid) = nothing, nothing

materialize_free_surface(::Nothing, ::PrescribedVelocityFields, grid) = nothing
materialize_free_surface(::ExplicitFreeSurface{Nothing}, ::PrescribedVelocityFields, grid) = nothing
materialize_free_surface(::ImplicitFreeSurface{Nothing}, ::PrescribedVelocityFields, grid) = nothing
materialize_free_surface(::SplitExplicitFreeSurface, ::PrescribedVelocityFields, grid) = nothing
Expand Down
8 changes: 4 additions & 4 deletions src/Models/NonhydrostaticModels/nonhydrostatic_model.jl
Original file line number Diff line number Diff line change
Expand Up @@ -194,14 +194,14 @@ function NonhydrostaticModel(; grid,
extract_boundary_conditions(diffusivity_fields))

# Next, we form a list of default boundary conditions:
prognostic_field_names = (:u, :v, :w, tracernames(tracers)..., keys(auxiliary_fields)...)
default_boundary_conditions = NamedTuple{prognostic_field_names}(FieldBoundaryConditions()
for name in prognostic_field_names)
field_names = (:u, :v, :w, tracernames(tracers)..., keys(auxiliary_fields)...)
default_boundary_conditions = NamedTuple{field_names}(FieldBoundaryConditions()
for name in field_names)

# Finally, we merge specified, embedded, and default boundary conditions. Specified boundary conditions
# have precedence, followed by embedded, followed by default.
boundary_conditions = merge(default_boundary_conditions, embedded_boundary_conditions, boundary_conditions)
boundary_conditions = regularize_field_boundary_conditions(boundary_conditions, grid, prognostic_field_names)
boundary_conditions = regularize_field_boundary_conditions(boundary_conditions, grid, field_names)

# Ensure `closure` describes all tracers
closure = with_tracers(tracernames(tracers), closure)
Expand Down
2 changes: 1 addition & 1 deletion test/test_biogeochemistry.jl
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const MB = Union{MinimalDiscreteBiogeochemistry, MinimalContinuousBiogeochemistr

@inline required_biogeochemical_tracers(::MB) = tuple(:P)
@inline required_biogeochemical_auxiliary_fields(::MB) = tuple(:Iᴾᴬᴿ)
@inline biogeochemical_auxiliary_fields(bgc::MB) = (; Iᴾᴬᴿ = bgc.photosynthetic_active_radiation)
@inline biogeochemical_auxiliary_fields(bgc::MB) = (; Iᴾᴬᴿ = bgc.photosynthetic_active_radiation)
@inline biogeochemical_drift_velocity(bgc::MB, ::Val{:P}) = bgc.sinking_velocity

# Update state test (won't actually change between calls but here to check it gets called)
Expand Down
18 changes: 9 additions & 9 deletions test/test_boundary_conditions_integration.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ include("dependencies_for_runtests.jl")
using Oceananigans.BoundaryConditions: ContinuousBoundaryFunction, FlatExtrapolationOpenBoundaryCondition, fill_halo_regions!
using Oceananigans: prognostic_fields

function test_boundary_condition(arch, FT, topo, side, field_name, boundary_condition)
function test_boundary_condition(arch, FT, Model, topo, side, field_name, boundary_condition)
grid = RectilinearGrid(arch, FT, size=(1, 1, 1), extent=(1, π, 42), topology=topo)

boundary_condition_kwarg = (; side => boundary_condition)
field_boundary_conditions = FieldBoundaryConditions(; boundary_condition_kwarg...)
bcs = (; field_name => field_boundary_conditions)
model = NonhydrostaticModel(; grid, boundary_conditions=bcs,
buoyancy=SeawaterBuoyancy(), tracers=(:T, :S))
model = Model(; grid, boundary_conditions=bcs,
buoyancy=SeawaterBuoyancy(), tracers=(:T, :S))

success = try
time_step!(model, 1e-16)
Expand Down Expand Up @@ -257,15 +257,15 @@ test_boundary_conditions(C, FT, ArrayType) = (integer_bc(C, FT, ArrayType),
topo = (Bounded, Bounded, Bounded)

for C in (Gradient, Flux, Value), boundary_condition in test_boundary_conditions(C, FT, array_type(arch))
@test test_boundary_condition(arch, FT, topo, :east, :T, boundary_condition)
@test test_boundary_condition(arch, FT, topo, :south, :T, boundary_condition)
@test test_boundary_condition(arch, FT, topo, :top, :T, boundary_condition)
@test test_boundary_condition(arch, FT, NonhydrostaticModel, topo, :east, :T, boundary_condition)
@test test_boundary_condition(arch, FT, NonhydrostaticModel, topo, :south, :T, boundary_condition)
@test test_boundary_condition(arch, FT, NonhydrostaticModel, topo, :top, :T, boundary_condition)
end

for boundary_condition in test_boundary_conditions(Open, FT, array_type(arch))
@test test_boundary_condition(arch, FT, topo, :east, :u, boundary_condition)
@test test_boundary_condition(arch, FT, topo, :south, :v, boundary_condition)
@test test_boundary_condition(arch, FT, topo, :top, :w, boundary_condition)
@test test_boundary_condition(arch, FT, NonhydrostaticModel, topo, :east, :u, boundary_condition)
@test test_boundary_condition(arch, FT, NonhydrostaticModel, topo, :south, :v, boundary_condition)
@test test_boundary_condition(arch, FT, NonhydrostaticModel, topo, :top, :w, boundary_condition)
end
end
end
Expand Down
13 changes: 11 additions & 2 deletions test/test_hydrostatic_free_surface_models.jl
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ topos_3d = ((Periodic, Periodic, Bounded),
# SingleColumnGrid tests
@test grid isa SingleColumnGrid
@test isnothing(model.free_surface)
@test !(:η ∈ keys(fields(model))) # doesn't include free surface
end
end

Expand All @@ -113,7 +112,6 @@ topos_3d = ((Periodic, Periodic, Bounded),
grid = RectilinearGrid(arch, FT, topology=topo, size=(1, 1), extent=(1, 2))
model = HydrostaticFreeSurfaceModel(; grid)
@test model isa HydrostaticFreeSurfaceModel
@test :η ∈ keys(fields(model)) # contrary to the SingleColumnGrid case
end
end
end
Expand All @@ -129,6 +127,17 @@ topos_3d = ((Periodic, Periodic, Bounded),
end
end

for FreeSurface in (ExplicitFreeSurface, ImplicitFreeSurface, SplitExplicitFreeSurface, Nothing)
@testset "$FreeSurface model construction" begin
@info " Testing $FreeSurface model construction..."
for arch in archs, FT in float_types
grid = RectilinearGrid(arch, FT, size=(1, 1, 1), extent=(1, 2, 3))
model = HydrostaticFreeSurfaceModel(; grid, free_surface=FreeSurface())
@test model isa HydrostaticFreeSurfaceModel
end
end
end

@testset "Halo size check in model constructor" begin
for topo in topos_3d
grid = RectilinearGrid(topology=topo, size=(1, 1, 1), extent=(1, 2, 3), halo=(1, 1, 1))
Expand Down
11 changes: 5 additions & 6 deletions test/utils_for_runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,11 @@ end
##### Boundary condition utils
#####

discrete_func(i, j, grid, clock, model_fields) = - model_fields.u[i, j, grid.Nz]
parameterized_discrete_func(i, j, grid, clock, model_fields, p) = - p.μ * model_fields.u[i, j, grid.Nz]

parameterized_fun(ξ, η, t, p) = p.μ * cos(p.ω * t)
field_dependent_fun(ξ, η, t, u, v, w) = - w * sqrt(u^2 + v^2)
exploding_fun(ξ, η, t, T, S, p) = - p.μ * cosh(S - p.S0) * exp((T - p.T0) / p.λ)
@inline parameterized_discrete_func(i, j, grid, clock, model_fields, p) = - p.μ * model_fields.u[i, j, grid.Nz]
@inline discrete_func(i, j, grid, clock, model_fields) = - model_fields.u[i, j, grid.Nz]
@inline parameterized_fun(ξ, η, t, p) = p.μ * cos(p.ω * t)
@inline field_dependent_fun(ξ, η, t, u, v, w) = - w * sqrt(u^2 + v^2)
@inline exploding_fun(ξ, η, t, T, S, p) = - p.μ * cosh(S - p.S0) * exp((T - p.T0) / p.λ)

# Many bc. Very many
integer_bc(C, FT=Float64, ArrayType=Array) = BoundaryCondition(C, 1)
Expand Down
Loading