-
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
Upgrade fields to store boundary conditions #631
Conversation
Just a note: I attempted this while working on abstract operations and was unable to solve the problem. But we are using new versions of julia + cuda tools all the time so it is / will be worth revisiting |
For tuples of closures, everything related to the closure is tupled, including the |
I don't understand this point, can it be elaborated? |
I'm okay with doing this but should we be worried that not all
Hmmm, do you remember what you got stuck on? I'd be in favor of reopening the issue and trying to adapt
Ah sorry I meant that passing a named tuple like boundary_conditions = (b=buoyancy_bcs, κₑ=(b=κₑ_bcs,)) to the model constructor will apply diffusivity boundary conditions as expected. But if you have two closures and you want to impose different diffusivity boundary conditions on each one then you can't do so by passing a named tuple to the model constructor. You can of course use We could upgrade the logic for interpreting boundary condition named tuples so that you can do something like κ1_bcs = (T=some_diffusivity_bcs, S=some_diffusivity_bcs)
κ2_bcs = (T=some_other_diffusivity_bcs, S=some_other_diffusivity_bcs)
boundary_conditions = (T=T_bcs, κₑ=(κ1_bcs, κ2_bcs)) but it felt like a pretty edge case (very very few users would probably use it) so I didn't do it. But we can do it, it's within scope for this PR. |
Ah the first one allows you to pass pre-constructed fields as keyword arguments. Users will probably not use this, it's mostly useful for custom diffusivity field tuple constructions. I had The second one allows you to pass non-default boundary conditions as a named tuple. Here It might be possible to merge the two constructors (which have different uses) into one but I couldn't figure out how to do it. I can add docstrings with these explanations. |
Another thing to potentially discuss if whether we want to switch from using EDIT: Well, actually this PR does quite a lot already. Better leave this for a future PR. |
Defaults are not about correctness, but convenience --- the concept of a "default" implies that it is not always correct (otherwise we would not use the word "default", because it would never have to change). I think defaults can make user's lives simpler. I think there may be a lot of cases in field definition where boundary conditions are unused.
Was just getting the dreaded "dynamic function invocation". It's worth a shot and documenting our progress. It seems unlikely there's a fundamental issue. We just have to figure it out.
Okay thank you I understand. The issue here is the ability to use defaults. For that, we would have to require users to name closures when used in a tuple. I am happy to leave this functionality as "unsupported". That said, I suspect we will have to confront this issue in the future if we want to do sophisticated GCM stuff, which can have complicated sets of turbulence closures. |
It may become common if users do realistic GCM stuff, because there will be many turbulence closures, but only one of them would require explicit boundary-diffusivity modeling to control field gradients and diffusivities along boundaries (for example, prescribing some interesting lateral diffusivity parameterization on top of a vertical mixing parameterization). EDIT: we don't need to confront these complexities now of course, but it's worth keeping in mind. One brute force solution is just to define convenience constructors for models themselves ( |
Ah interesting I didn't think of such cases. I believe we can set up arbitrarily complicated boundary conditions so anything is possible. It's just hard and probably unfeasible/undesirable to extend the user interface to handle all these cases. |
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.
Pre-emptive approval for planned changes.
Thought: we provide the sugary syntax .top
and .bottom
for boundary conditions in z
. Should we also provide east, west, south, and north, and avoid using .left
and .right
in the code for full clarity?
Sounds like a good idea. I think I'll open an issue to document this. |
Codecov Report
@@ Coverage Diff @@
## master #631 +/- ##
=========================================
Coverage ? 78.11%
=========================================
Files ? 119
Lines ? 2326
Branches ? 0
=========================================
Hits ? 1817
Misses ? 509
Partials ? 0
Continue to review full report at Codecov.
|
This PR (part 3/3) upgrades the field abstraction so fields store their own boundary conditions. This simplifies the model boundary condition hierarchy and generalizes the field and boundary conditions abstractions so they can be used for a compressible model (and any other model we come up with in the future). All future fields will have boundary conditions so PRs like #601 won't be necessary again.
The only change in user interface is that you pass a named tuple to the model constructor now instead of an instance of
SolutionBoundaryConditions
.This also works for LES diffusivities so the amount of convoluted scripting gymnastics is much reduced (see test from #601). Setting a diffusivity BC is now almost as easy as a tracer BC.
Internally: No surprise, this change ended up being pretty invasive. But note that we now have a more flexible and easier to use package with fewer lines of code!
I'm happy to discuss and iterate over the choices that were made in this PR. But glad that I was able to make these changes. Development of the compressible model can continue based on this branch.
Changes:
Fields has a new property:
field.boundary_conditions
.Better pretty printing for fields:
Field tuple constructors have two versions now. One that accepts boundary conditions (useful in model constructors) and another that accepts full fields (useful for checkpoint restoration and other custom functionality).
You can pass anything intoA check was added in Ability to construct field tuples with non-zero data #627.VelocityFields
andTracerFields
. Perhaps we should check whether everything passed into these functions is aField
orAbstractField
. But for now that's extra code and it's not an exported function so I'm okay leaving it as is (we might want the flexibility in the future?).Model no longer has the
boundary_conditions
property.There is no need for
SolutionBoundaryConditions
andModelBoundaryConditions
anymore: code is simpler 🎉TurbulentDiffusivities
has been renamed toDiffusivityFields
for consistency withVelocityFields
andTracerFields
, etc.For each turbulence closure,
DiffusivityFields
gets the same two versions (one if you want control over the full field, another if you just want to set boundary conditions).Halo filling and parts of the time stepping have been simplified as there is no need to juggle around and bundle up boundary conditions.
Some comments:
nothing
? I did this for the abstract operations tests and they all passed.Field
abstraction to be GPU compatible (The future of the Field abstraction #298).Resolves #606
Note: This PR branches off from #628.