-
Notifications
You must be signed in to change notification settings - Fork 82
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
One Equation RANS Model with Wall Function Support #1299
base: main
Are you sure you want to change the base?
Conversation
@hgopalan A note on the naming of the model. One-equation RANS model is usually used to refer to a family of models (https://turbmodels.larc.nasa.gov/), so naming this as one-eq RANS and the source term as |
(ustar * ustar / (0.556 * 0.556) + rans_b - tke_arr(i, j, k)) / | ||
dt; | ||
} | ||
if (cell_blank == 1) { |
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.
In GPU mode, prefer to just multiply by cell_blank
(i.e., drag_forcing = - Cd * m * tke_arr(i, j, k, 0) * cell_blank
)
You'll most likely need a cast to to keep the linters happy.
amrex::Real dragforcing = 0; | ||
amrex::Real ux = vel(i, j, k, 0); | ||
amrex::Real uy = vel(i, j, k, 1); | ||
const int cell_drag = (has_terrain) ? drag_arr(i, j, k) : 0; |
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.
If the cell_drag
value is always 1 and the switch is has_terrain
, why do we need drag_arr
?
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.
drag_arr purpose is to apply the wall function as a forcing term at first cell above the terrain. So added it to mark those cells.
amrex::Real ux = vel(i, j, k, 0); | ||
amrex::Real uy = vel(i, j, k, 1); | ||
const int cell_drag = (has_terrain) ? drag_arr(i, j, k) : 0; | ||
const int cell_blank = (has_terrain) ? blank_arr(i, j, k) : 0; |
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.
Can't this be just blank_arr(i, j, k)
?
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.
blank_arr is available only if the terrain drag physics is enabled. Not sure if amrex::Array4() is always zero?
const bool has_terrain = | ||
this->m_sim.repo().int_field_exists("terrain_blank"); | ||
const auto* const m_terrain_blank = | ||
has_terrain ? &this->m_sim.repo().get_int_field("terrain_blank") | ||
: nullptr; | ||
const auto* const m_terrain_drag = | ||
has_terrain ? &this->m_sim.repo().get_int_field("terrain_drag") | ||
: nullptr; | ||
const auto& blank_arr = has_terrain | ||
? (*m_terrain_blank)(lev).const_array(mfi) | ||
: amrex::Array4<int>(); | ||
const auto& drag_arr = has_terrain ? (*m_terrain_drag)(lev).const_array(mfi) | ||
: amrex::Array4<int>(); |
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.
Would it be possible to simplify this?
…ghts. Added a reg test.
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.
My main comment would be to figure out a way to simplify the code so that it is easier to follow. Maybe even totally different parallelfors depending on the path (with shared device function calls) or always defining terrain (default to 0) to minimize if conditions.
@@ -1,4 +1,5 @@ | |||
target_sources(${amr_wind_lib_name} PRIVATE | |||
KsgsM84Src.cpp | |||
KwSSTSrc.cpp | |||
Krans.cpp |
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.
Agreed with @sayerhs. Lets have a more specific name for this.
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.
Let me finish the other changes and before final commit, change the name.
} | ||
dissip_arr(i, j, k) = std::pow(Cmu, 3) * | ||
std::pow(tke_arr(i, j, k), 1.5) / | ||
(tlscale_arr(i, j, k) + 1e-3); |
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 + 1e-3
feels arbitrary? is that to avoid a division by zero? If so then it feels very large. There was another PR where we suggested the right way to do this with numeric_limits.
amrex::Real uz = vel(i, j, k, 2); | ||
const amrex::Real m = std::sqrt(ux * ux + uy * uy + uz * uz); | ||
const amrex::Real Cd = | ||
std::min(10 / (dx[2] * m + 1e-5), 100 / dx[2]); |
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.
1e-5 -> see my comment about 1e-3. Also, let's see if we can abstract out/generalize the direction with an axis variable so we are not hardcoding for z-dir.
TurbulenceModel::CoeffsDictType model_coeffs() const override; | ||
|
||
private: | ||
Field& m_vel; |
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 bet a bunch of these can be const refs
const amrex::Array4<amrex::Real>& src_term) const override; | ||
|
||
private: | ||
Field& m_turb_lscale; |
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 bet a bunch of these can be const refs
gradT_arr(i, j, k, 1) * gravity[1] + | ||
gradT_arr(i, j, k, 2) * gravity[2]) * | ||
beta; | ||
amrex::Real epsilon = std::pow(Cmu, 3) * |
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.
const?
beta; | ||
amrex::Real epsilon = std::pow(Cmu, 3) * | ||
std::pow(tke_arr(i, j, k), 1.5) / | ||
tlscale_arr(i, j, k); |
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.
here you don't have that + 1e-3. Maybe instead of adjusting it everywhere, and forgetting sometimes, it is worth just having a clip step on tlscale at some point so you know it is never going to be 0?
const amrex::Real lscale_b = | ||
Cb_stable * | ||
std::sqrt( | ||
tke_arr(i, j, k) / std::max(stratification, 1e-10)); |
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 arbitrary small number.
Summary
This draft PR implements an one-equation RANS model based on the work of Axel et. al (https://link.springer.com/article/10.1023/A:1011560202388) for canonical ABL simulation. Terrain support is added similar to the Kosovic LES model. Wall function support is added through forcing term in the K-Equation. Not recommended for use as a low-Reynolds number RANS model with y+<30.
It is possible to convert the model into a hybrid RANS-LES model by changing the length scale function.
To be added:
Documentation
Pull request type
Draft
Please check the type of change introduced:
Checklist
The following is included:
This PR was tested by running:
Additional background
Developed to provide a single interface for running wall-function aware RANS and LES using AMR-Wind.