-
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
New abstractions for buoyancy and rotation #412
Conversation
…ents for VerticalRotationAxis type ;
Codecov Report
@@ Coverage Diff @@
## master #412 +/- ##
==========================================
+ Coverage 73.28% 74.85% +1.57%
==========================================
Files 25 25
Lines 1426 1416 -10
==========================================
+ Hits 1045 1060 +15
+ Misses 381 356 -25
Continue to review full report at Codecov.
|
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.
This PR looks great to me! Should especially help when we want to move away from the f-plane (#28).
Might be good to discuss in person with @jm-c and @christophernhill to get their thoughts on this PR. I'll keep from explicitly approving this PR for now.
Kind of surprised tests passed this easily! Also nice to see new features resulting in code size going down!
@inline fu(g::RegularCartesianGrid{T}, u, f, i, j, k) where T = T(0.5) * f * (avgx_f2c(g, u, i, j-1, k) + avgx_f2c(g, u, i, j, k)) | ||
@inline ∂x_p(i, j, k, grid::RegularCartesianGrid, p) = δx_c2f(grid, p, i, j, k) / grid.Δx | ||
@inline ∂y_p(i, j, k, grid::RegularCartesianGrid, p) = δy_c2f(grid, p, i, j, k) / grid.Δy | ||
@inline ∂z_p(i, j, k, grid::RegularCartesianGrid, p) = δz_c2f(grid, p, i, j, k) / grid.Δz |
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.
Hmmm, do we really need extra operators named specifically for pressure? Seems fine to me to call δx_c2f(grid, p, i, j, k) / grid.Δx
, etc.
But maybe this point is moot as the operators will all be replaced by PR #283 at some point.
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.
Maybe just ∂x
etc. would be a better name as it's not particular to just pressure.
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.
The operator for pressure serves no functional purpose, as you point out --- it is purely introduced to enhance readability. We certainly can use ∂x_faa
(which is defined in turbulence_closures/closure_operators.jl
). I think we should use functions as much as possible, so I'd prefer using a function here over hard-coding the derivative --- if this pattern is consistent with the finite volume implementation of these equations.
In TurbulenceClosures
, I introduced a lot of such functions (see ∂x_u
, etc) in the hope that it would enhance the readability and clarity of the hyper-complicated functions involved in the turbulence closures.
I adopted the same philosophy here.
Some improvement could be had by defining ∂x_p
as an alias via
const ∂x_p = ∂x_faa
To avoid a proliferation of redundant functions (I think I did this in TurbulenceClosures
, but forget when I was implementing it here).
src/rotation.jl
Outdated
return VerticalRotationAxis{T}(f) | ||
end | ||
|
||
const FPlane = VerticalRotationAxis |
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.
Maybe it's a GFD thing but would FPlane
be a better default name than VerticalRotationAxis
? I find it's pretty common to say f-plane but not very common to say vertical rotation axis to describe a constant f scenario.
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.
That's fine with me. I started out that way, but decided that VerticalRotationAxis
was more descriptive and precise and thus deserved to fill the primary role here (similar to using Value
instead of Dirichlet
). But if you would like to change back to FPlane
, go ahead.
@@ -1,3 +1,5 @@ | |||
using .TurbulenceClosures: ▶z_aaf |
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.
Hmmm, it's somewhat concerning that we're starting to mix turbulence closure operators with regular operators (when the two should really be merged), but this is probably fine for now and will be resolved by PR #283 when we get to it.
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.
There is no interpolation operator for function within the "regular" operators, however... ?
src/time_steppers.jl
Outdated
@loop for k in (1:grid.Nz; (blockIdx().z - 1) * blockDim().z + threadIdx().z) | ||
@loop for j in (1:grid.Ny; (blockIdx().y - 1) * blockDim().y + threadIdx().y) | ||
@loop for i in (1:grid.Nx; (blockIdx().x - 1) * blockDim().x + threadIdx().x) | ||
@inbounds Gw[i, j, k] = (-u∇w(grid, U.u, U.v, U.w, i, j, k) | ||
- z_f_cross_U(i, j, k, grid, rotation, U) |
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.
🚀
We've decided:
|
Notes:
|
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.
Cool stuff!
Do you think this will be ready to merge soon-ish? From implementing the nonlinear equation of state, seems that the features/API needed has become clearer.
We should probably add tests for the Roquet and others (2015) equations of state. Not sure what to test exactly though. At the very least, they should run and not crash. Beyond that could compare with hand calculations but that seems kind of useless?
PS: Sorry I've committed a great sin and edited your original comment to include "Resolves #191."
src/buoyancy.jl
Outdated
`ρ′ = R₁₀₀ * T + R₀₁₀ * S + R₀₂₀ * T^2 + R₀₁₁ * T * D` | ||
+ R₂₀₀ * S^2 + R₁₀₁ * S * D + R₁₁₀ * S * T` | ||
""" | ||
function RoquetIdealizedNonlinearEOS(T, flavor=:cabbeling_thermobaricity; coeffs=roquet_coeffs[flavor], ρ₀=1025) |
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.
Does it matter that we've been using ρ₀=1027
? I guess not as this only applies to RoquetIdealizedNonlinearEOS
while we've been using 1027 with LinearEquationOfState
.
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.
ρ₀
has no affect on dynamics unless you're using a nonlinear equation of state (or if you use it for thermodynamics calculations).
We will need to put some thought in how to ensure that, if users do use ρ₀
for boundary conditions or thermodynamics calculations, that the same value is used for both the equation of state and those thermodynamics calculations (which in principle could occur even before declaring Model
).
Possibly we can write up some 'Density' type that takes an equation of state as an argument and knows how to do the right thing (requires ρ₀
when given a LinearEquationOfState, or uses the default if its not specified and given a nonlinear equation of state.)
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.
Hmmm, yeah seems like the proper thing to do is to construct the buoyancy
struct before you need to use the value of ρ₀
and then pass buoyancy
to the Model
constructor.
It's not the prettiest solution though.
Yes I think so. The main clarifications here are that
|
What are feelings about the nomenclature "IdealizedNonlinearEOS" versus "IdealizedNonlinearEquationOfState"? |
…re defined at cell interfaces, use buoyancy frequency squared function in turbulence closures
I'm happy to start working on that in October. I think we should reverse the k-index first, because I think at least part of the variable delz algorithm will depend on the direction of @ali-ramadhan on |
This is probably just me being ignorant or maybe just different conventions in different fielss but I guess I remember seeing
To me it seems that |
Completely agree. @suyashbire1 and I are hoping this gets merged ASAP because all our scripts are depending on old versions right now (which have other issues) and we need to update them but can't until the buoyancy and rotation API is stable (well we can, but we want to refactor all our scripts at once). |
Might also add changing |
…et yet, comment tweaking
…ators.jl as topo-level file for Operators module
Hmm, well gravitational acceleration x (volume of object x density of displaced fluid - mass of the object) which gives the forcing acting on the body (positive upwards due to the sign of the terms). Here we deal in densities, so the buoyant forced defined in the same manner would essentially be gravitational acceleration x ( reference density - density of the buoyant fluid ) which of course is precisely I think that from its usage the meaning of Something to keep in mind as we continue to discuss this topic outside this PR is that atmospheric scientists go even a step further and define 'buoyancy' as |
@ali-ramadhan please merge if tests pass. |
Tests are still failing but I'll fix them. |
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.
Just some suggested changes but nothing important. We can clean things up later. I think it's more important to merge this and tag a new stable release.
@inline x_f_cross_U(i, j, k, grid::AbstractGrid{T}, ::Nothing, U) where T = zero(T) | ||
@inline y_f_cross_U(i, j, k, grid::AbstractGrid{T}, ::Nothing, U) where T = zero(T) | ||
@inline z_f_cross_U(i, j, k, grid::AbstractGrid{T}, ::Nothing, U) where T = zero(T) | ||
|
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.
Needs another section header here maybe.
Also called `FPlane`, after the "f-plane" approximation for the local effect of | ||
Earth's rotation in a planar coordinate system tangent to the Earth's surface. | ||
""" | ||
function FPlane(T::DataType=Float64; f) |
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 thought we weren't using types in function signatures unless it's for dispatch? (Following the YASGuide)
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.
Hmm... I think this was originally needed for dispatch because f
came with a default; however it looks like it may not be needed anymore.
New abstractions for buoyancy and rotation Former-commit-id: 577b225
This PR rewrites the user interface for specifying rotation rates and buoyancy parameters, such as gravitational acceleration, and the relationship between tracers and density via an equation of state.
The main purpose of this PR is to move towards a more logical, functionality-driven encapsulation of parameters.
For rotation, this PR defines a new type called
AbstractRotation
whose subtypes are intended to describe parameters related to the background rotation rate of the model. At the moment there is only one such type:VerticalRotationAxis
, orFPlane
. In the future we expect to haveBetaPlane
andTiltedRotationAxis
orFullCoriolis
. TheAbstractRotation
types are uniquely associated with a function that computes the x, y, and z components of "f X U", associated with the background vorticityf
.For density, equations of state, and buoyancy, a new abstract type
AbstractBuoyancy
is defined. This type is intended to encapsulate both a gravitational acceleration parameter (when applicable), and an equation of state that contains parameters associated with the relationship between tracer values and density.The advantage of this design is that parameters logically associated with certain terms in the equation of motion are now associated with a particular abstract type. This permits an easy extension of the rotational of buoyancy/equation of state models currently contained in the code; for example in the future we hope to support a
TracerBuoyancy
type for the case that buoyancy is itself a tracer and gravitational acceleration is no longer a parameter, and perhaps aTemperatureBuoyancy
type for the case that only temperature contributes to buoyancy, rather than both temperature and salinity as is currently the case.In addition, our hydrostatic and non-hydrostatic pressure fields now have the same units and can be directly summed to obtain the total pressure.
Resolves #191.
Resolves #217.
Resolves #403.