Complete CCPPization of Holtslag-Boville PBL scheme and vertical diffusion solver #270
Conversation
|
Thanks @nusbaume for the detailed review! I've updated most of the standard names to match the ones just recently assigned by Peter and Adam in the CCPP standard names spreadsheet, then "merged in" some of your suggested changes to further quantify these names. I think a good chunk of I've kicked off another round of regression tests to make sure the changes are still bit-for-bit with CAM. I tested in SIMA and they still match b4b with the snapshots. Some of the questions might need the input of scientists. Since there'll likely be another round of cleanup after this PR (to finalize standard names and to incorporate Michael's refactoring changes) I would vote to try to get this merged into ESCOMP/CAM first if the queue is ready then we can address the remaining issues (which are mostly metadata) as we get closer to the code freeze. |
There was a problem hiding this comment.
Thanks for responding to all of my concerns @jimmielin! I just had a few remaining questions and requests.
schemes/holtslag_boville/holtslag_boville_diff_interstitials.meta
Outdated
Show resolved
Hide resolved
| dimensions = (horizontal_loop_extent, vertical_interface_dimension) | ||
| intent = in | ||
| [ ti ] | ||
| standard_name = air_temperature_at_interface_for_vertical_diffusion_tbd |
There was a problem hiding this comment.
It looks like there are still some _tbd suffixes in the file. Just double-checking that those are there on purpose?
| ! will be added to this later as they are computed. | ||
| !> \section arg_table_vertical_diffusion_diffuse_horizontal_momentum_timestep_init Argument Table | ||
| !! \htmlinclude arg_table_vertical_diffusion_diffuse_horizontal_momentum_timestep_init.html | ||
| subroutine vertical_diffusion_diffuse_horizontal_momentum_timestep_init( & |
There was a problem hiding this comment.
Thanks for the explanation! I think it is slightly unconventional in the sense that one scheme is being used to calculate a quantity at runtime while another, different scheme is being used to initialized it. However given that these are all under the same Fortran module, and that for now I am not sure there will ever be a situation where vertical_diffusion_wind_damping_rate will be included in an SDF without vertical_diffusion_diffuse_horizontal_momentum, then I am happy to leave this as-is.
(I'll keep this un-resolved in case this issue comes be up again, but we can consider it "resolved" for the sake of this PR!)
Co-authored-by: Jesse Nusbaumer <[email protected]>
| type = real | kind = kind_phys | ||
| intent = in | ||
| [ taux ] | ||
| standard_name = eastward_stress_at_surface_for_vertical_diffusion_tbd |
There was a problem hiding this comment.
Looking at the spreadsheet, it seems we're still waiting for this standard name and others. I guess I'd vote for removing the "_tbd"s anyway, since, as Jesse mentioned, the standard names seem reasonable.
…for atmos_phys (#310) Tag name (The PR title should also include the tag name): atmos_phys0_18_000 Originator(s): @jimmielin @nusbaume List all `development` PR numbers included in this PR and the title of each: - Add Github action to use git-fleximod for externals checking #306 by @nusbaume - Complete CCPPization of Holtslag-Boville PBL scheme and vertical diffusion solver #270 by @jimmielin List all automated tests that failed, as well as an explanation for why they weren't fixed: All B4B in CAM verified via Izumi/Nag Izumi/GNU (previously via Derecho/Intel) B4B in SIMA with HB testcase see ESCOMP/CAM-SIMA#410
Companion PRs: ESCOMP/CAM#1361 and ESCOMP/CAM-SIMA#410
Originator(s): @jimmielin
Description (include issue title and the keyword ['closes', 'fixes', 'resolves'] and issue number):
hb_diff.F90) bothcompute_hb_diffandcompute_hb_free_atm_diffto atmospheric_physicsholtslag_boville.F90.holtslag_boville_interstitials.F90.compute_vdiff) without molecular diffusion (i.e., not WACCM-X) into CCPPized schemes indiffusion_solver.F90.diffusion_stubs.F90-- schemes in this file will eventually be moved to their corresponding schemes, e.g., tms, beljaars, ubc, ... when they are fully CCPPized.List all namelist files that were added or changed:
List all files eliminated and why: N/A
List all files added and what they do:
List all existing files that have been modified, and describe the changes:
(Helpful git command:
git diff --name-status development...<your_branch_name>)List all automated tests that failed, as well as an explanation for why they weren't fixed:
to_be_ccppized/nlte_fomichev.F90due to dependency on CAM-only modulechem_surfvalsIs this an answer-changing PR? If so, is it a new physics package, algorithm change, tuning change, etc?
B4B with CAM - new physics package
If yes to the above question, describe how this code was validated with the new/modified features:
bit-for-bit unchanged, except for Derecho GNU physics tests:
and updated baselines in CAM4 tests due to adding HB PBL and vertical diffusion solver.