-
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
Create field boundary conditions using information from grid topology #620
Conversation
function FieldBoundaryConditions(grid; kwargs...) | ||
kws = keys(kwargs) | ||
for kw in kws | ||
if kw ∉ valid_directions |
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.
Another strategy would be to set all defaults in the constructor to 'nothing', and then reset them to default if they are nothing. The error the user receives would then bean ordinary KeywordError. This also may simplify subsequent validation.
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 ended up assigning default boundary conditions in the constructor which cleaned up the function quite a bit.
e = "Cannot specify west or east boundary conditions with $TX topology in x-direction." | ||
throw(ArgumentError(e)) | ||
else | ||
west_bc = :west in kws ? kwargs[:west] : default_bc(TX) |
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.
could let default_bc take two arguments: The topology and the current bc. Then define a fallback that throws an error, whitelist the bcs that are valid, and define a default for ::Nothing. Might be a bit simpler and more modular, less repeated code.
I've refactored a bit more so that we can use u_bcs = FieldBoundaryConditions(grid, field_type=:velocity)
T_bcs = FieldBoundaryConditions(grid, field_type=:tracer, top=FluxBoundaryCondition(0.1)) An alternative design (really a wrapper): VelocityFieldBoundaryConditions(grid; kwargs...) =
FieldBoundaryConditions(grid, field_type=:velocity, kwargs...)
TracerFieldBoundaryConditions(grid; kwargs...) =
FieldBoundaryConditions(grid, field_type=:tracer, kwargs...) This will allow us to properly set default velocity and tracer boundary conditions with a single @glwagner Let me know what you think. |
I added tests and updated the documentation so will merge once tests pass. I also added more readable (i.e. reads like how you would say it) boundary condition constructors, e.g. fbc = FluxBoundaryCondition(-1e-3)
vbc = ValueBoundaryCondition(25) |
Codecov Report
@@ Coverage Diff @@
## master #620 +/- ##
==========================================
- Coverage 74.59% 71.33% -3.26%
==========================================
Files 118 117 -1
Lines 2220 2327 +107
==========================================
+ Hits 1656 1660 +4
- Misses 564 667 +103
Continue to review full report at Codecov.
|
This PR aims to create a single
FieldBoundaryConditions
function to replaceHorizontallyPeriodicBCs
andChannelBCs
. This paves the way for boundary conditions to become aField
property (#606).It's a pretty big breaking change so would be good to agree on the design and API before refactoring.
Side note: Ideally we would set
default_bc(::Flat) = nothing
but until we elide operations and halo filling alongFlat
dimensions, I think we should usedefault_bc(::Flat) = PeriodicBC()
.@glwagner Let me know if this looks like what we discussed. If so, I'll go ahead and change all the tests and examples to use it instead of
HorizontallyPeriodicBCs
andChannelBCs
.Reminder to self: Documentation, especially model setup, will have to be revised as part of this PR.