-
Notifications
You must be signed in to change notification settings - Fork 32
CAM-SIMA diagnostics for all gravity wave parameterizations #338
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
base: development
Are you sure you want to change the base?
Conversation
nusbaume
left a comment
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.
Thanks for bringing in these diagnostics @jimmielin! I did have some some questions/concerns, mostly related to standard names and/or units, but of course if any of them are too much of a burden at this stage then we can likely just make an issue for it and deal with them later. Thanks again!
schemes/sima_diagnostics/gravity_wave_drag_common_diagnostics.F90
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,103 @@ | |||
| ! Diagnostics for gravity wave drag from deep convection (Beres scheme) | |||
| module gravity_wave_drag_convection_deep_diagnostics | |||
| use ccpp_kinds, only: kind_phys | |||
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 we move this use statement down to the run phase subroutine in order to better match the CAM-SIMA coding standards?
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.
Thanks, edited throughout
| call history_add_field('BTAUNET', 'net_eastward_reynolds_stress_due_to_deep_convective_gravity_wave_drag', 'ilev', 'avg', 'Pa') | ||
|
|
||
| call history_add_field('NETDT', 'tendency_of_air_temperature_due_to_deep_convection', 'lev', 'avg', 'K s-1') | ||
| call history_add_field('HDEPTH', 'convective_heating_depth_due_to_deep_convective_gravity_wave_drag', horiz_only, 'avg', 'km') |
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 double-checking if this is the heating depth "due" to gravity wave drag, or if it is just the convective heating depth, that is then used by the gravity wave drag scheme? If it is the latter then I think the long name here should just be convective_heating_depth, and we should maybe add a note/issue that the official standard name should be updated as well.
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.
Good point, it's the convective heating depth (that gets diagnosed inside the deep convective gravity waves code so it's an output from there).
After looking at the hdepth calculations between here and the convective schemes I decided to change due_to to for since they differ slightly, throughout the gw code.
There's a variant for shallow convection (that scheme is unused), would it be OK with you to change it to {deep,shallow}_convective_heating_depth? I think I can make all changes entirely within this atmos_phys PR
| standard_name = tendency_of_air_temperature_due_to_diffusion_due_to_deep_convective_gravity_wave_drag | ||
| units = J kg-1 s-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.
This standard name cannot be correct if the units are J kg-1 s-1. Is this the tendency of dry_air_enthalpy_at_constant_pressure instead of temperature?
Please note that I am happy to just make an issue for this change so that we can remember to deal with it later, instead of fixing it immediately in this PR.
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.
Yes you are correct, it should be tendency_of_ dry_air_enthalpy_at_constant_pressure_due_to_diffusion_due_to_deep_convective_gravity_wave_drag. It gets divided by cpair only when outputting diagnostics so the unit for that is J kg-1 s-1 until its output then it's K s-1 (the output diagnostic "standard name" is kept at tendency_of_air_temperature_due_to_diffusion_due_to_deep_convective_gravity_wave_drag).
I made the change throughout. Thanks!
| standard_name = tendency_of_air_temperature_due_to_kinetic_energy_dissipation_due_to_deep_convective_gravity_wave_drag | ||
| units = J kg-1 s-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.
I have the same standard name concern here (either the units are wrong, or the standard name is wrong and it should be dry_air_enthalpy_at_constant_pressure instead).
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.
Thanks, made the same change as above
| @@ -0,0 +1,126 @@ | |||
| ! Diagnostics for gravity wave drag from frontogenesis. | |||
| module gravity_wave_drag_frontogenesis_diagnostics | |||
| use ccpp_kinds, only: kind_phys | |||
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.
Move this use statement down to the *_run subroutine? This same request holds for the other diagnostics source code files in this PR as well.
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.
Thanks, edited throughout
| standard_name = tendency_of_air_temperature_due_to_diffusion_due_to_frontogenesis_gravity_wave_drag | ||
| units = J kg-1 s-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.
Another standard name that might not be correct according to the units? Please note that this same question holds for all of the tendency_of_air_temperature_due_to_XYZ variables with units of J kg-1 s-1 in this PR.
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.
Thanks, edited the .meta file standard names for dttdf and dttke throughout to reflect that it's tendency of dry air enthalpy at constant pressure (J kg-1 s-1) coming out of the scheme; the output is in K s-1 which is the tendency of air temperature.
| call history_add_field('UTGW_MOVMTN', 'tendency_of_eastward_wind_due_to_moving_mountain_gravity_wave_drag', 'lev', 'inst', 'm s-2') | ||
| call history_add_field('VTGW_MOVMTN', 'tendency_of_northward_wind_due_to_moving_mountain_gravity_wave_drag', 'lev', 'inst', 'm s-2') | ||
|
|
||
| call history_add_field('HDEPTH_MOVMTN', 'convective_heating_depth_due_to_moving_mountain_gravity_wave_drag', horiz_only, 'inst', 'km') |
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 double-checking if this heating depth is "due" to gravity wave drag, or just an input that is used by the gravity wave scheme? If it is just used by the scheme then I might change the due_to in the long/standard name to for, or something like that.
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.
Thanks for the question. After looking at the hdepth calculations between here and the convective schemes I decided to change due_to to for since they differ slightly, throughout the gw code.
| call history_add_field('UCELL_MOVMTN', 'eastward_wind_at_steering_level_due_to_moving_mountain_gravity_wave_drag', horiz_only, 'inst', 'm s-1') | ||
| call history_add_field('VCELL_MOVMTN', 'northward_wind_at_steering_level_due_to_moving_mountain_gravity_wave_drag', horiz_only, 'inst', 'm s-1') | ||
| call history_add_field('CS_MOVMTN', 'gravity_wave_phase_speed_in_source_direction_due_to_moving_mountain_gravity_wave_drag', horiz_only, 'inst', 'm s-1') | ||
| call history_add_field('XPWP_SRC_MOVMTN', 'momentum_flux_source_due_to_moving_mountain_gravity_wave_drag', horiz_only, 'inst', 'm2 s-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.
Just double-checking if this momentum flux is actually due_to, or caused by the gravity wave drag, or if it is for gravity waves, i.e. a source of gravity wave drag?
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 agree it should be for (it's constructed from a layer-averaged xpwp_clubb = sqrt(upwp_clubb**2 + vpwp_clubb**2) field.
I changed throughout to momentum_flux_source_for_moving_mountain_gravity_wave_drag
| intent = in | ||
| [ hdepth ] | ||
| standard_name = convective_heating_depth_due_to_moving_mountain_gravity_wave_drag | ||
| units = km |
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.
Should the units actually be meters (m) here? If not then I don't think we want the conversion in the source code.
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.
Thanks, should be m in the meta files, edited!
Co-authored-by: Jesse Nusbaumer <nusbaume@ucar.edu>
|
Thanks @nusbaume for the review! I edited some standard names throughout the GW parameterizations in particular to address concerns about the use of |
Originator(s): @jimmielin
Description (include issue title and the keyword ['closes', 'fixes', 'resolves'] and issue number):
List all namelist files that were added or changed: N/A
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:
New baselines will be created in the eventual CAM-SIMA PR
Is this an answer-changing PR? If so, is it a new physics package, algorithm change, tuning change, etc?
New diagnostic fields
Model "state" is b4b
If yes to the above question, describe how this code was validated with the new/modified features: