- 
                Notifications
    You must be signed in to change notification settings 
- Fork 22
Add biogeochemical interface to ocean_simulation #123
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
base: main
Are you sure you want to change the base?
Conversation
| advection_value = tuple(fill(tracer_advection, length(tracers))...) | ||
| tracer_advection = (; zip(tracers,advection_value)...) | ||
| tracer_advection = merge(tracer_advection, (e = nothing,)) | 
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.
Something along these lines might work too:
tracer_advection = NamedTuple(name => tracer_advection for name in tracers)
tracer_advection = merge(tracer_advection, (; e=nothing))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 should we have something that checks if the user has already provided a named tuple? something like
if !(tracer_advection isa NamedTuple)
    # expand tracer_advection into a named tuple with `e=nothing`
endThat way people can experiment with also advecting TKE which may be useful.
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.
Dictionaries also can be used which may be a little easier to understand:
tracer_advection = Dict{Symbol, Any}(name => tracer_advection for name in tracers)
tracer_advection[:e] = nothing
tracer_advection = NamedTuple(name => tracer_advection[name] for name in tracers)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.
Thanks @glwagner for the suggestions
| free_surface, | ||
| coriolis, | ||
| forcing, | ||
| biogeochemistry, | 
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.
🚀
| Looking good so far! Dictionaries can be useful for building up keyword arguments / parameters with values that have to be overwritten, since it's easy to overwrite entries in a dictionary. | 
| Codecov ReportAttention: Patch coverage is  
 
 Additional details and impacted files@@          Coverage Diff          @@
##            main    #123   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         31      31           
  Lines       1729    1742   +13     
=====================================
- Misses      1729    1742   +13     ☔ View full report in Codecov by Sentry. | 
Co-authored-by: Simone Silvestri <[email protected]>
| @glwagner in the most recent commit I added a way to merge the user supplied  | 
| 
 I think what you wrote captures the gist of it! But maybe we can make a few tweaks, I'll suggest something | 
| Hmm yes actually I realized we need to do something more like this: using Oceanaingans.BoundaryConditions: DefaultBoundaryCondition
# create a utility that filters `DefaultBoundaryCondition`
boundaries = (:west, :east, :south, :north, :bottom, :top, :immersed)
"""
    field_bcs_to_dict(field_bcs)
Return a `Dict` with the properties of `field_bcs` filtered to remove `DefaultBoundaryCondition`.
"""
function field_bcs_to_dict(field_bcs)
    all_bcs = Dict(boundary => getproperty(field_bcs, boundary) for boundary in boundaries)
    non_default_bcs = filter(kv -> !isa(kv[2], DefaultBoundaryCondition), all_bcs)
    return non_default_bcs
end
# inside ocean_simulation...
# make clear these are the defaults
default_boundary_conditions = Dict(
    :u => Dict(:top => FluxBoundaryCondition(Jᵘ), :bottom => u_bot_bc),
    :v => Dict(:top => FluxBoundaryCondition(Jᵛ), :bottom => v_bot_bc),
    :T => Dict(:top => FluxBoundaryCondition(Jᵀ)),
    :S => Dict(:top => FluxBoundaryCondition(Jˢ))
)
# Build a list of the default + user-specified boundary condition names
user_bc_names = keys(boundary_conditions)
bc_names = tuple(:u, :v, :T, :S, user_bc_names...) |> unique
# Merge user and default boundary conditions
merged_boundary_conditions = Dict()
for fieldname in bc_names
    default_bcs = get(default_boundary_conditions, fieldname, Dict())
    if fieldname ∈ user_bc_names
        user_bcs = field_bcs_to_dict(boundary_conditions[fieldname])
    else
        user_bcs = Dict()
    end
    # Generate a dictionary specifying `FieldBoundaryConditions` for each `fieldname`
    bc_dict = merge(default_bcs, user_bcs)
    merged_boundary_conditions[fieldname] = FieldBoundaryConditions(; bc_dict...)
end
# Convert to NamedTuple
merged_boundary_conditions = NamedTuple(merged_boundary_conditions)A few things we could do (in a future PR) are: 
 | 
| Not super pretty either... | 
| Realized my code sketch was missing the conversion to  | 
| 
 Ok no worries. I'll go through and test it and make sure it's all working, thanks. | 
| I dropped the ball on this but picking it back up now and going to test it and hopefully it'll be ready for review this week! | 
This PR addresses #107 by adding a
biogeochemistrykwarg toocean_simulation, adding user inputboundary_conditionsas a kwarg and movestracerto be explicitly spelled out be the user as a kwarg.Right now the user input
boundary_conditionswill be merged with hardcoded boundary conditions for T and S, unless the user supplies BCs for T and S in which case they will override the hardcoded ones. As @glwagner pointed outbut I haven't implemented that yet.