Domain stretching #310
Domain stretching #310
Conversation
| land_domain = | ||
| LSMSingleColumnDomain(; zlim = (zmin, zmax), nelements = nelements) | ||
| dz_bottom = FT(0.5) | ||
| dz_top = FT(0.025) |
There was a problem hiding this comment.
Question / something to consider:
We define dz top and bottom here, and if I understand correctly the other dz in between are exponential.
Would it be helpful to add and option for other extrapolation, e.g., linear from top to bottom dz? (just to keep in mind, but no need to do this in this PR)
There was a problem hiding this comment.
yes! ok, i had the same question. right now there are 3 grid stretching options in ClimaCore: Uniform, Exponential, and GeneralizedExponential. The way I wrote this PR currently makes it so, as you wrote, we have to choose the GeneralizedExponential. From a user interface point of view I thought it was a little cleaner (the user passes dz_tuple = (dz_bottom, dz_top), instead of stretch = ClimaCore.Meshes.GeneralizedExponentialStretching(dz_bottom, dz_top). But, if they pass the stretch object itself, then we have more flexibility on what they can choose (including, in the future, maybe a linear stretching of the grid).
and then, actually, we do not need an if statement, because we can make the default value stretch = ClimaCore.Meshes.Uniform() and always pass stretch into the function that makes the mesh.
mesh = ClimaCore.Meshes.IntervalMesh(column; nelems = nelements, stretch = stretch)
Im open to the latter! what do you think?
There was a problem hiding this comment.
I am also ok with both options...
I guess the question is, do we need these options
For now I am ok to just keep it simple, as it is now.
There was a problem hiding this comment.
ok, let's leave it for now! Right now exponential stretching is the only option, and if we need other options, we'd need to add them in ClimaCore, and that would most likely be a low priority item anyways.
|
FYI, I edited the PR text |
add stretching and tests typo temporarily remove piracy tests fixed bug in tests and docs relax threshold rev comments
kmdeck
force-pushed
the
domain_stretching
branch
from
bee9972 to
099fcef
Compare
|
bors r+ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
310: Domain stretching r=kmdeck a=kmdeck ## Purpose Allows for variable domain stretching (finer resolution at the top of the domain compared to the bottom). Whenever we need dz in source code, we obtain it using the function `get_Δz`, which uses ClimaCore to extract the `dz` values. so we do not make the assumption of uniform spacing in src code. (we do in some tests, but that seems OK if those tests use uniform spacing.) ## To-do Future PRs: fix piracy and add the aqua test back in: #277 #311 ## Content 1. Adds domain stretching to all domains with a z coordinate. 2. Renames height to depth for the SphericalShell domain 3. uses the stretched domain in the ozark example 4. adds unit tests. Note that the user passes the target (dz_bottom, dz_top) desired, as a Tuple, or `nothing`, if no stretching is needed. We then create the ClimaCore.Meshes.GeneralizedExponentialStretching object in the domain constructor appropriately. Instead, the user could pass the stretching object itself, and the default could be `ClimaCore.Meshes.Uniform`. In a way this is nicer (it's not prescribed that they use Uniform or GeneralizedExponentialStretching, gives them all the options that ClimaCore develops in the future, no `if` statement in the constructor), but I felt this might be more confusing than supplying the target layer widths at the top and bottom. Happy to change if anyone feels strongly! Review checklist I have: - followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/ - followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/ - followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy - checked that this PR does not duplicate an open PR. In the Content, I have included - relevant unit tests, and integration tests, - appropriate docstrings on all functions, structs, and modules, and included relevant documentation. ---- - [X] I have read and checked the items on the review checklist. Co-authored-by: kmdeck <kdeck@caltech.edu>
Purpose
Allows for variable domain stretching (finer resolution at the top of the domain compared to the bottom).
Whenever we need dz in source code, we obtain it using the function
get_Δz, which uses ClimaCore to extract thedzvalues. so we do not make the assumption of uniform spacing in src code. (we do in some tests, but that seems OK if those tests use uniform spacing.)To-do
Future PRs: fix piracy and add the aqua test back in:
#277 #311
Content
Note that the user passes the target (dz_bottom, dz_top) desired, as a Tuple, or
nothing, if no stretching is needed. We then create the ClimaCore.Meshes.GeneralizedExponentialStretching object in the domain constructor appropriately. Instead, the user could pass the stretching object itself, and the default could beClimaCore.Meshes.Uniform. In a way this is nicer (it's not prescribed that they use Uniform or GeneralizedExponentialStretching, gives them all the options that ClimaCore develops in the future, noifstatement in the constructor), but I felt this might be more confusing than supplying the target layer widths at the top and bottom. Happy to change if anyone feels strongly!Review checklist
I have:
In the Content, I have included