-
Notifications
You must be signed in to change notification settings - Fork 193
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
A new boundary condition API #118
Conversation
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
==========================================
+ Coverage 52.71% 57.29% +4.58%
==========================================
Files 19 19
Lines 645 644 -1
==========================================
+ Hits 340 369 +29
+ Misses 305 275 -30
Continue to review full report at Codecov.
|
@ali-ramadhan @jm-c I realized that I may have specified the flux boundary condition wrong --- I think that I am double counting the flux coming in/out of the cell from the interior (this part is calculated during the main time-stepping loop. I guess the sanity check is that it shouldn't do anything if I'll add you as collaborators on my repo in case you want to change anything (I probably won't have time till Monday). Also looks like Travis is failing; not sure why that is. Feel free to criticize my design. This is just a first pass as I see it. |
I propose changing the struct |
Thanks for embarking on this @glwagner!
Just added "WIP" (work in progress) to the PR title. We can work on the tests together. For testing, we should be able to extend the 1D column model example: https://github.com/climate-machine/Oceananigans.jl/blob/master/examples/column_model.jl
Looks like it's just the one test in the time stepping section that's failing. Maybe something to do with how the boundary conditions are called during the time stepping? The Model test passes so it's not erroring during model initialization/construction.
I'll always vote for clarity! Just one initial question: I might be misunderstanding the purpose of Have to run out but will have a more detailed look later today. Otherwise, looks really neat! On a more practical note, might end up being easier to discuss this PR in person. |
…ation of flux boundary condition in bottom cell to account for baked-in default
Just another thought. If it's implemented in this way with
with the final boundary condition being a function, would there be 30+ nested parameterized types for |
It's not obvious to me what a field boundary condition is, vs a coordinate boundary condition. |
@johncmarshall54 each field ( We could restrict ourselves to specific combinations of boundary conditions instead, which would reduce the number of possibilities. For example, we might have just doubly periodic in ( The current design is meant to give the user maximum flexibility, but I'm open to simplifying it. That would restrict what users can do, but perhaps that's ok? @jm-c feedback welcome! |
@ali-ramadhan do you mean with regards to performance? I'm not sure. With multiple dispatch being core to julia it seems this scenario is not uncommon (30+ may not be very large).
The function |
It should be noted that this PR also makes progress on addressing #100. |
Agree that model.boundary conditions += HorizontallyPeriodic()
I'm still pretty new to Julia so yeah don't know if that will be an issue, especially on the GPU. Only way to find out is to try and benchmark! Maybe you're right and 30+ isn't a lot. @vchuravy any idea on whether 30+ parameterized types for a struct is too many? Would this hurt performance on the GPU?
I see. So if no parameterizations are being used, are the boundary conditions actually being imposed then? Even with KPP, isn't the boundary condition still being imposed only to later be modified by KPP? I still feel like |
I believe yes: if the viscosity/diffusivity is 0, then flux or no-slip boundary conditions cannot be imposed (mathematically, the order of the PDE is reduced in that case). We then can only impose no-penetration or periodic conditions.
My primitive logic: for a flux boundary condition, |
Other names:
precisely, and I like the syntax you propose. We can do lots of stuff here to make our user's lives easy. There are two issues: the backend, and the user interface. Maybe the title of this PR is confusing, because I think it's primarily about the backend. |
It may not stand up to mathematical rigor but I still like If something = bc.calc(args...) while bc.impose!(args...) But now we're just arguing semantics instead of what's important.
I think so too. API suggests more front-end. I also think discussing these dense and complicated issues (e.g. this PR and #120) among multiple busy people is difficult on GitHub. Little far ahead but maybe the Monday CliMA meetings are a good place to get high-level feedback? |
Sure, we can talk about code design at the CliMa meeting. Let me summarize what we've discussed so far: The current designA "boundary condition" is a flux, gradient, or value of a field ( Pros
Concerns
Other miscellaneous thoughts
|
@SandreOuza has requested a Also here are some proposed tests:
|
That sounds like a potentially nice abstraction. Can we get together
tomorrow to sync up on details?
Chris
…On Thu, Mar 14, 2019 at 20:27 Gregory L. Wagner ***@***.***> wrote:
@SandreOuza <https://github.com/SandreOuza> has requested a Gradient
BCType.
Also here are some proposed tests:
1. Constant initial conditions + zero flux = average value of quantity
doesn't change
2. Linear temperature profile + prescribed identical fluxes at top and
bottom = linear temperature profile
3. Diffusive solution to the free-convection example
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#118 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADXx4GPeeGVVSIRI-T7YAVmhrd4tnTj9ks5vWujkgaJpZM4bmbWT>
.
|
Ok, sounds good! I’m going to try to write some of those tests this morning. |
… can be used. Defines TimeVaryingBoundaryCondition for Boundary conditions that are only a function of time.
…values to be changed
…s, fixes bugs in boundary condition implementation
src/time_steppers.jl
Outdated
apply_bcs!(::Val{:GPU}, ::Val{:y}, args...) = ( | ||
@hascuda @cuda threads=(Tx, Ty) blocks=(Bx, By, Bz) apply_y_bcs!(Val(:GPU), args...)) | ||
apply_bcs!(::Val{:GPU}, ::Val{:z}, args...) = ( | ||
@hascuda @cuda threads=(Tx, Ty) blocks=(Bx, By, Bz) apply_x_bcs!(Val(:GPU), 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.
Typo here? Should it be apply_z_bcs!
instead of apply_x_bcs!
?
This PR implements a new boundary condition API.
It's a bit of a work in progress. Before merging we need:
Criticism welcome!
Summary of implementation: the
BoundaryCondition
The core type of this PR is
BoundaryCondition
. TheBoundaryCondition
has one field,calc
, and is made callable viaA
BoundaryCondition
has one parameter:BCType
. TheBCType
can beDefault
,Flux
, orValue
. We includeDefault
because the specification of boundary conditions is baked in to the algorithm. In the case of 'default' boundary conditions, no calculations are made.nboundary_conditions = nfields x ndims x 2
.In principle, the user can specify
5 x 3 x 2
boundary conditions: on each of the 5 fields (which we currently have --- unfortunately, we may want more in the future), the user can specify a condition on any of the 6 boundaries. The model is initialized with all 30 boundary conditions set toDefault
.We allow for all possible boundaries to have conditions by introducing three structs:
CoordinateBoundaryConditions
with fieldsleft
andright
FieldBoundaryConditions
with fieldsx
,y
, andz
BoundaryConditions
with fieldsu
,v
,w
,T
, andS
.Specifying boundary conditions and user API
A boundary condition is now specified by defining a function that calculates it within a kernel that loops over the boundary in question. The arguments of a flux boundary conditions on a top or bottom boundary (the only situation supported so far) are
This will probably change in the future.
The user API is rather threadbare at the moment. To define a constant flux boundary condition, for example, the user might write (to specify the boundary condition as a function)
to specify the boundary condition as a constant,
There are probably some sugary things that we can implement to smooth this specification.
Caveats
A side note
I think that, at some point, it would be preferable to move the implementation of boundary conditions and equations to a new file / module so that new physics can be easily implemented my modifying that module / file.