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.7) Correctly account for field names in ContinuousBoundaryFunctions #4008

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
32 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
e4d3867
Merge branch 'main' into ss/correct-continuous-boundary-functions
navidcy Jan 11, 2025
d9fd965
bump patch release
navidcy Jan 11, 2025
4853626
Merge branch 'main' into ss/correct-continuous-boundary-functions
navidcy Jan 29, 2025
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.4"
version = "0.95.5"

[deps]
Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e"
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,9 +75,19 @@ 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),
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)

merge(hydrostatic_fields(model.velocities, model.free_surface, model.tracers), model.auxiliary_fields)

"""
field_names(model)

Return a tuple of the field names associated with `HydrostaticFreeSurfaceModel`.
Equivalent to doing `keys(fields(model))`.
"""
@inline field_names(model::HydrostaticFreeSurfaceModel) = field_names(model.free_surface, tracernames(model.tracers), model.auxiliary_fields)

@inline field_names(free_surface, tracernames, auxiliary_fields) = (:u, :v, :w, tracernames..., :η, keys(auxiliary_fields)...)
@inline field_names(::Nothing, tracernames, auxiliary_fields) = (:u, :v, :w, tracernames..., keys(auxiliary_fields)...)
@inline field_names(::SplitExplicitFreeSurface, tracernames, auxiliary_fields) = (:u, :v, :w, tracernames..., :η, :U, :V, keys(auxiliary_fields)...)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bad idea. I think that fields and prognostic_fields should be the source of truth. Those functions return NamedTuple --- both names, and fields. Thus the names can be derived from fields or prognostic_fields.

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Dec 18, 2024

Choose a reason for hiding this comment

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

we need something to regularize the boundary conditions, so we need some names that correspond to keys(fields(model)) before the model is built. I am open to a different implementation though.
At the moment there is a (wrong) assumption that fields(model) will always have the same structure. We could change fields(model) to return nothing in the positions where we would have a field, in that way the regularization would work

Copy link
Member

Choose a reason for hiding this comment

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

I see

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 have changed the design for the fields(model) to always output a predictable namedtuple with nothing in place of fields that don't exist. In this way we don't need a new function


"""
prognostic_fields(model::HydrostaticFreeSurfaceModel)
Expand Down Expand Up @@ -111,11 +121,11 @@ Return a flattened `NamedTuple` of the prognostic fields associated with `Hydros

@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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,14 @@ 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))
hydrostatic_field_names = field_names(free_surface, tracernames(tracers), auxiliary_fields)
default_boundary_conditions = NamedTuple{hydrostatic_field_names}(Tuple(FieldBoundaryConditions()
for name in hydrostatic_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, hydrostatic_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
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
14 changes: 14 additions & 0 deletions test/test_hydrostatic_free_surface_models.jl
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ topos_3d = ((Periodic, Periodic, Bounded),
@test grid isa SingleColumnGrid
@test isnothing(model.free_surface)
@test !(:η ∈ keys(fields(model))) # doesn't include free surface
@test field_names(model) == keys(fields(model))
end
end

Expand All @@ -114,6 +115,7 @@ topos_3d = ((Periodic, Periodic, Bounded),
model = HydrostaticFreeSurfaceModel(; grid)
@test model isa HydrostaticFreeSurfaceModel
@test :η ∈ keys(fields(model)) # contrary to the SingleColumnGrid case
@test field_names(model) == keys(fields(model))
end
end
end
Expand All @@ -129,6 +131,18 @@ topos_3d = ((Periodic, Periodic, Bounded),
end
end

for FreeSurface in (ExplicitFreeSurface, ImplicitFreeSurface, SpliExplicitFreeSurface, Nothing)
@testset "$topo model construction" begin
@info " Testing $free_surface model construction..."
for arch in archs, FT in float_types
grid = RectilinearGrid(arch, FT, topology=topo, size=(1, 1, 1), extent=(1, 2, 3))
model = HydrostaticFreeSurfaceModel(; grid, free_surface=FreeSurface())
@test model isa HydrostaticFreeSurfaceModel
@test field_names(model) == keys(fields(model))
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
Loading