Tendency application update#299
Conversation
…emove *_of_new_state (default name refers to 'current' value)
…medmfvdifq return new tendencies and new tendency application block in GFS_PBL_generic_post
- Added tendency variables (`ten_t`, `ten_u`, `ten_v`) for temperature and wind tendencies in SAMF deep and shallow convection schemes. - Updated relevant `.meta` files to reflect changes in variable intent (`inout` -> `in` where appropriate) and added tendencies. - Modified Interstitials (`GFS_DCNV_generic_post`, `GFS_DCNV_generic_pre`, `GFS_SCNV_generic_post`, `GFS_SCNV_generic_pre`) to directly use tendencies instead of saved state variables. - Removed redundant saved variables (`save_u`, `save_v`, `save_t`). - Added `delt` to post interstitials.
…' into feature/tendency_cleanup
…_dimension in GFS_time_vary_pre.scm
…endency paradigm; cleanup saving of states no longer needed
…as removed recently
|
@rhaesung @yangfanglin Here are some updated plots from testing: https://docs.google.com/presentation/d/1NXsgRd_2M-zAZIPlHagYhKa8W7TcJEftaR2sxjRvkfY/edit?slide=id.p#slide=id.p These results were obtained last week, after the code had been updated to the latest develop branch. I've added a test using the conus13km_control because it uses the FV3_HRRR suite with smoke/dust active (this wasn't tested last time). Based on these results, I'm still satisfied that these code changes aren't changing the scientific results at all. The exception is for the RAS scheme, which we're removing anyway, so I have not debugged the failed RTs. |
|
@grantfirl Thanks so much for the great work and for sharing the latest test results — I’ve started taking a look. |
@rhaesung Sure, I emailed you both offline to set up a time. |
rhaesung
left a comment
rhaesung
left a comment
There was a problem hiding this comment.
@grantfirl I've had a chance to go through the convection schemes in this PR. Here is a summary of the key points I've flagged.
- Bugs: found a few indexing errors (
ntr+1vsntr+2), missing/dt, and skippedten_uupdates. - Consistency: the internal state updates (like
qv) and diagnostic units (likeQLCN/QICN) - just want to ensure they all align with the new tendency framework. - Metadata minor cleanup
Thanks and looking forward to your thoughts!
| [pu] | ||
| standard_name = x_wind_of_new_state | ||
| standard_name = x_wind | ||
| long_name = updated x-direction wind |
There was a problem hiding this comment.
removing 'updated' would be more accurate. The same should be applied to pv, pt, and other meta files.
There was a problem hiding this comment.
Good point. I agree and I remember thinking it when I was making metadata edits, but I didn't bother due to long_names not being used to match anything. But, it makes sense to fix them because it can be misleading as-is. Will do.
Thanks @rhaesung for the thorough review. I'll address these right away. |
|
@grantfirl @rhaesung Have the requested changes been addressed? The parent PRs were marked ready for review, so I wanted to check on this. |
No, I'm making the changes now. There will likely be some more testing that needs to happen beyond RTs yet too, so this shouldn't be scheduled for the merge queue yet. |
rhaesung
left a comment
rhaesung
left a comment
There was a problem hiding this comment.
@grantfirl The overall direction of the refactor looks great! To bring GWD fully in line with your new framework, just a couple of points:
- Since
generic postnow handles diagnostic accumulation, please remove the manual updates in the primary schemes to avoid double-counting. - Let's keep temperature tendencies alongside the wind to ensure all variables follow the same logic and maintain physical symmetry.
…omputing diagnostic tendencies in drag_suite
rhaesung
left a comment
rhaesung
left a comment
There was a problem hiding this comment.
@grantfirl For MP, I just have a few questions:
- Since you mentioned the tracer array issue in convection, I was curious about
ten_qin MP. Is the mapping from individual species (dqc, dqr, etc.) to the generalten_qarray handled automatically now, or do we still need to keep an eye on manual updates? Just want to make sure we don't hit the same snag there! - On the precip variables (prcp, PREC), I noticed they're still intent(inout) without a guard flag like GWD's
ugwp_seq_update. Are we worried about double-counting whengeneric_postkicks in, or is that handled elsewhere in the current UFS suites? - Just a small safety check - I noticed some optional variables (like
nc,nwfa) are accessed withoutpresent()checks.
There was a problem hiding this comment.
Noticed the Nc (Number concentration) tendencies are missing from the post. Was this an intentional call, or did it just slip through? Just curious about keeping the 2-moment physical balance for the next step. Thoughts?
There was a problem hiding this comment.
Originally, the number concentrations are only being set from a temporary variable from m_micro. Now, the tendency is being set in m_micro, so there isn't any need to do anything with the tendency in this post scheme. It does raise the point about consistency of the number concentrations when any of rain/snow/graupel mixing ratios would be made negative by the tendencies coming from m_micro. The original post scheme doesn't address this, and neither does the updated code.
rhaesung
left a comment
rhaesung
left a comment
There was a problem hiding this comment.
@grantfirl A few questions for the PBL schemes:
- Most schemes still modify legacy tracer arrays (rtg, dqdt) directly. Shouldn't we move these to ten_q to match the T, U, V strategy and your metadata?
- For 2D surface fluxes, some use = or += on intent(out). I saw a similar pattern in the MP schemes as well. Wouldn't it be better to handle these 2D variables using the same tendency guards as the 3D states?
- Many optional diagnostic arrays (like dtend) are missing present() checks.
There was a problem hiding this comment.
ten_u/v are initialized to 0 but never updated after shoc_work, effectively disabling wind mixing.
| do i=1,nx | ||
| gq0(i,k,ntlnc) = ncpl(i,k) | ||
| ten_q(i,k,ntlnc) = (ncpl(i,k) - gq0(i,k,ntlnc))/dtp | ||
| gq0(i,k,ntinc) = ncpi(i,k) |
There was a problem hiding this comment.
This should use ten_q instead.
| enddo | ||
| endif | ||
| if(ldiag3d .and. dtidx(100+ntoz,index_of_process_pbl)>1) then | ||
| allocate(old_ozone(im,levs)) |
There was a problem hiding this comment.
We need to deallocate this.
| real(kind_phys), dimension(:,:), intent(out) :: & | ||
| & dtdt, dudt, dvdt, dqdt_water_vapor, dqdt_liquid_cloud, & | ||
| & dqdt_ice, dqdt_snow, dqdt_ozone | ||
| real(kind_phys), dimension(:,:,:), intent(out) :: dqdt_all |
There was a problem hiding this comment.
The dqdt_all is defined but doesn't seem to be populated with the actual tendencies.
| & allocate( EDDYVESTX ( MAXCAN ) ) | ||
| if(.not.allocated(ZCANX)) | ||
| & allocate( ZCANX ( MAXCAN ) ) |
There was a problem hiding this comment.
Pre-existing leak in EDDYVESTX/ZCANX. Needs deallocation before return.
There was a problem hiding this comment.
Since dtsfc/dusfc are intent(out), won't += access uninitialized memory? Also, wouldn't populating both ten_q and qtnp cause double updates in the host model?
There was a problem hiding this comment.
Tracers are still modifying rtg directly here. Shouldn't we switch to ten_q to stay consistent with the T, U, V refactoring? Also, won't direct assignment to 2D fluxes risk overwriting data from previous schemes?
|
@rhaesung @yangfanglin FYI, I've added results for tests to cover the 6 additional schemes that we were interested in double-checking (RRTMGP, GF convection, nTiedtke convection, GFDL MP v3, NSSL MP, and TEMPO MP) to slides that I've shared before: https://docs.google.com/presentation/d/1NXsgRd_2M-zAZIPlHagYhKa8W7TcJEftaR2sxjRvkfY/edit?usp=sharing See slides 20-43 for the new results. All of these schemes look to be behaving as expected with result changes within the same magnitude as obtained during a compiler change or optimization level change. Because a RT that exercised nTiedtke didn't exist, a new RT was created for the nTiedtke scheme based off of a HAFS suite (since that application seems to have the most interest in this scheme). I'll have a separate PR in UFS WM and UFSATM that has the new HAFS RT setup that I'll link to this set of PRs. The HAFS team may want to look at the new test to doublecheck it; it's using a new SDF based off of the FV3_HAFS_v1_thompson_tedmf_gfdlsf suite but replaces SAMF convection with nTiedtke (only). |
| intent = inout | ||
| [gq0] | ||
| standard_name = specific_humidity_of_new_state | ||
| standard_name = specific_humidity |
There was a problem hiding this comment.
name mismatch w other meta files. tracer concentration?
There was a problem hiding this comment.
Is there a reason for using dtp for state updates but dtf for dtend diagnostics? If they aren't identical, the diagnostics won't match the actual state change?
|
|
||
| ! add radar temp tendency | ||
| ! there is radar coverage | ||
| gt0(i,k) = save_t(i,k) + ttend*dtp |
There was a problem hiding this comment.
Looks like this is a legacy overwrite bug. This should be changed to gt0(i,k)=gt0(i,k)+ttend*dtp.
There was a problem hiding this comment.
similar dtp vs dtf question here.
There was a problem hiding this comment.
A few quick notes on the new post-routine:
- Including U, V, and T in this update logic seems to add unnecessary overhead, since photochemistry only affects tracers?
- I'm concerned about the missing zero-out safeguard here, similar to what we discussed for MG microphysics. Shouldn't we ensure O3 and H2O tendencies don't lead to negative concentrations?
| endif | ||
|
|
||
| !htrsw is calculated in rrtmg_sw_post if using RRTMG and above if using RRTMGP | ||
| ten_t = htrsw |
There was a problem hiding this comment.
Won't ten_t=htrsw overwrite the LW contribution from htrlw? Should these be accumulated instead?
There was a problem hiding this comment.
- Are the U,V, and Q loops necessary?
- You switched the diagnostics input from tgrs to gt0. Since gt0 might already be updated by LW radiation, won't this cause inconsistency in the diagnostic output?
There was a problem hiding this comment.
- I noticed that the else block for non-lsidea runs (which accumulates htrlw/htrsw into dtend) has been removed. Is this diagnostic logic being moved to a different routine, like the new radiation_post? If this is removed without a replacement, won't we lose the 3D budget diagnostics for lw/sw heating?
- ten_t, ten_u, ten_v, and ten_q are never used.
11 participants
Description of Changes:
See https://docs.google.com/presentation/d/1LJbCrCVGYDvm0UPo8SR4KP6fT1VFocg-xLG399qGQ1w/edit?slide=id.g371ddcb3c06_0_238#slide=id.g371ddcb3c06_0_238 for a complete description.
Main points:
Tests Conducted:
See the linked slides. This was tested in the following ways:
Dependencies:
None
Documentation:
TODO
Issue (optional):
None
Contributors (optional):
@grantfirl @VanderleiVargas-NOAA