Conversation
…emi-implicit solver. Input correct exner pressure to horizontal molecular diffusion solver.
…be applied after remapping to pressure surfaces.
…ng rather than on warped Lagrangian levels during acoustic steps.
…ed_sphere into nco_debug_changes
…e into nco_debug_changes
…e into nco_debug_changes
Add compiler flags for cmake -DFASTER build option
Fix intel debug warnings (implicit i,j,k; explicit mpi interface)
lharris4
left a comment
There was a problem hiding this comment.
Thank you for submitting this PR. I think this is a nice addition to FV3 and an important first step towards deep atmosphere dynamics. The implementation is done well, mostly self-contained and unobtrusive, and largely follows proper FV3 coding standards. I have a few comments about specific implementation details.
Think carefully about how frequently to update var_grav. The Lagrangian vertical coordinate does deform every acoustic timestep, but the variation in gravity g*(r/r0)**2 even over a dt_atmos dynamical timestep will be weak: for an extreme 100 m/s updraft and a 180 s timestep, the elevation change is only 18 km representing a 0.05% change in gravity. My first suggestion would be to simply re-calculate grav_var at the beginning of each dynamics timestep and hold it constant through the acoustic and remapping timesteps; this would reduce the amount of re-calculation and data movement, but stability and accuracy would need to be tested.
We also need to verify reproducibility for both hydrostatic and nonhydrostatic cases when var_grav == .F..
| grav_var_h = grav | ||
| grav_var = grav | ||
|
|
||
| if(flagstruct%var_grav)then |
There was a problem hiding this comment.
This is the best place to put the calculation of variable gravity. We only should recalculate at the beginning of a dynamics timestep dt_atmos and not every remapping or acoustic timestep, as we can expect the variation of grav due to deformation of the Lagrangian vertical coordinate to be small during a dynamical timestep.
driver/UFS/atmosphere.F90
Outdated
| call fv_io_register_nudge_restart ( Atm ) | ||
|
|
||
| if(.not.allocated(grav_var))then | ||
| allocate(grav_var(isd:ied,jsd:jed,npz),grav_var_h(isd:ied,jsd:jed,npz+1)) |
There was a problem hiding this comment.
I would strongly suggest defining grav_var(:,:,:) as a variable in fv_atmos_type instead of as a module variable in atmosphere_mod. Also, is there a need to have grav_var_h available outside of fv_dynamics()?
There was a problem hiding this comment.
The code has been changed as recommended (seen in updated fv_arrys.F90 and atmosphere.F90)
model/fv_dynamics.F90
Outdated
| endif ! md_tadj_layers>0 and md_time and GFDL_interstitial%last_step | ||
| endif | ||
|
|
||
| if(flagstruct%var_grav)then |
There was a problem hiding this comment.
Is there a good case to be made for re-calculating grav_var after every remapping? See my comment above. Do also note that since FV3 uses a vertically-Lagrangian coordinate, the geometric height of each interface will change on each acoustic timestep.
There was a problem hiding this comment.
I guess it is better to update grav-var after every remapping. The magnitude of both wind speed (could be hundred m/s to thousand m/s) and temperature (thousand degree) are very high. And species density changes rapidly with height and magnitude of geopotential height deviations increase with increasing altitude, which could reach ~3km at 800km. The WAM needs to coupled with an empirical model of Ionosphere Plasmasphere Electrodynamics. A relative precise scale height would get a better performance of the solar activities. Without this implementation, the scale height needs a correction before coupling to IPE while with current implementation, model turns a good result and allows the correction of the scale height to be removed.
There was a problem hiding this comment.
After some discussion, updating on the remapping timestep is probably the best.
model/dyn_core.F90
Outdated
| do k=1,npz | ||
| do j=jsd,jed | ||
| do i=isd,ied | ||
| grav_var(i,j,k) = (grav_var_h(i,j,k+1)+grav_var_h(i,j,k))/2. |
There was a problem hiding this comment.
Why does this recalculation need to be done?
There was a problem hiding this comment.
To my understanding, this is not recalculation. The author only passed the updated gravity at interface into "dyn-core" and "grav_var" is taken as a local variable seen at line 184 and line 309.
There was a problem hiding this comment.
by the way, the grav_var_h updated at each remapping loop
There was a problem hiding this comment.
OK. I presume this is just to remap grav_var at layer interfaces onto layer centroids, done once per remapping loop.
| grav_var = grav | ||
|
|
||
| if(flagstruct%var_grav)then | ||
| do j=js,je |
There was a problem hiding this comment.
This loop can be refactored for better locality and cache utilization. I would recommend computing the surface (npz+1) values in the first loop, then compute from npz to 1 by -1 using a new k-j-i loop. This would also allow you to define newrad as a 2D array, reducing memory usage and possibly better fitting in cache.
| den(i,k) = -delp(i,j,k)/(grav_var(i,j,k)*delz(i,j,k)) | ||
| w0(i,k) = w(i,j,k) | ||
| gz(i,k) = gzh(i) - g2*delz(i,j,k) | ||
| gz(i,k) = gzh(i) - grav_var(i,j,k)*delz(i,j,k)/2.0 |
There was a problem hiding this comment.
Try to avoid divisions whenever possible; changing /2.0 to *0.5 is more efficient.
| den(i,k) = -delp(i,j,k)/(grav_var(i,j,k)*delz(i,j,k)) | ||
| w0(i,k) = w(i,j,k) | ||
| gz(i,k) = gzh(i) - g2*delz(i,j,k) | ||
| gz(i,k) = gzh(i) - grav_var(i,j,k)*delz(i,j,k)/2.0 |
| ice_wat, snowwat, graupel, hailwat, q, qc, cvm, pt(is:ie,j,k) ) | ||
| do i=is,ie | ||
| delz(i,j,k) = delz(i,j,k) / pt(i,j,k) | ||
| #ifdef MULTI_GASES |
There was a problem hiding this comment.
Good catch. Do note in the PR message that there are some bugfixes for MULTI_GASES (and for molecular_diffusion too).
model/nh_utils.F90
Outdated
| do j=jsd,jed | ||
| do i=isd,ied | ||
| rgrav(i,j,k) = 1./grav_var(i,j,k) | ||
| rdg(i,j,k) = -rdgas/grav_var(i,j,k) |
There was a problem hiding this comment.
This line can be replaced with rdg(i,j,k) = -rdgas*rgrav(i,j,k) to avoid divisions.
There was a problem hiding this comment.
Thanks and replaced with suggestion
|
Lucas, Thank you for taking the time to carefully review this PR. One thing comes to my mind is that wind speed in the thermosphere is normally a few hundred m/s but can reach thousands during geomagnetic storms. Updating var_grav at each acoustic timestep may become necessary. @XiaqiongZhou-NOAA @WChen-NOAA
|
OK. While the wind speed may necessitate a small timestep, I don't know if this would necessarily translate into rapid variations in altitude that would lead to significant local changes in gravity. However again this would have to be determined through testing. The simplest approach would indeed be to simply update each timestep; the method @WChen-NOAA uses is not particularly expensive. |
Remove local copy of FindESMF.cmake and rename esmf target to ESMF::ESMF
…e into nco_debug_changes
Bring in -check all and -ftrapuv to DEBUG intell builds
|
Find a small err in one subroutine, and made a correction (works while using subroutine compute_total_energy). See the differences in attached file. |
Good catch---thanks for finding and fixing this. Thanks also for uploading the demonstration of this improvement as a world-visible GitHub attachment. |
|
It seems grav in ~/tools is still only constant. Just wondering if this impacts restart, diagnostics and others? |
|
@XiaqiongZhou-NOAA, Good question. I am checking again and think over it. |
|
@XiaqiongZhou-NOAA
|
Description
The model uses the shallow atmosphere approximation with constant gravity
This is problematic for a space weather model (WAM) where Ztop/r0 )~10%
Implement a scaling factor wherever gravity is applied to account for variations with height:
g(r0/r)^2 while setting var_grav = .true. in the name list
the new gravity as grav_var(i,j,k)
Fixes # (394)
How Has This Been Tested?
The test results are available vi doc
Checklist:
Please check all whether they apply or not