Skip to content
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

Document units in 9 vertical param modules #253

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

Hallberg-NOAA
Copy link
Member

Documented numerous internal variables and their units in 9 vertical parameterization modules (MOM_bkgnd_mixing, MOM_bulk_mixed_layer, MOM_diabatic_aux, MOM_diabatic_driver, MOM_entrain_diffusive, MOM_kappa_shear, MOM_regularize_layers, MOM_set_diffusivity and MOM_set_viscosity). This commit includes the addition of units arguments in 9 unlogged get_param calls. A number of spelling errors were also corrected in comments. All answers and output are bitwise identical.

@Hallberg-NOAA Hallberg-NOAA added the documentation Improvements or additions to documentation label Nov 23, 2022
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #253 (b54ff49) into dev/gfdl (798ba7a) will decrease coverage by 0.09%.
The diff coverage is 66.66%.

❗ Current head b54ff49 differs from pull request most recent head 6e1a2eb. Consider uploading reports for the commit 6e1a2eb to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #253      +/-   ##
============================================
- Coverage     37.18%   37.08%   -0.10%     
============================================
  Files           263      263              
  Lines         72793    73397     +604     
  Branches      13578    13677      +99     
============================================
+ Hits          27065    27222     +157     
- Misses        40718    41143     +425     
- Partials       5010     5032      +22     
Impacted Files Coverage Δ
...rameterizations/vertical/MOM_regularize_layers.F90 4.85% <0.00%> (-0.02%) ⬇️
...arameterizations/vertical/MOM_bulk_mixed_layer.F90 48.30% <30.00%> (-0.15%) ⬇️
...rc/parameterizations/vertical/MOM_diabatic_aux.F90 61.33% <33.33%> (ø)
...parameterizations/vertical/MOM_set_diffusivity.F90 53.30% <66.66%> (ø)
...rameterizations/vertical/MOM_entrain_diffusive.F90 68.96% <73.07%> (-0.09%) ⬇️
...rc/parameterizations/vertical/MOM_bkgnd_mixing.F90 31.78% <100.00%> (ø)
...parameterizations/vertical/MOM_diabatic_driver.F90 46.30% <100.00%> (ø)
src/parameterizations/vertical/MOM_kappa_shear.F90 74.07% <100.00%> (ø)
...c/parameterizations/vertical/MOM_set_viscosity.F90 59.55% <100.00%> (ø)
src/ALE/MOM_regridding.F90 26.87% <0.00%> (-1.25%) ⬇️
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@adcroft adcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but I'm uncomfortable about the precedent of four exceptions to the usual pattern when specifying units and scaling of run-time parameters.

The four exceptions are reading hidden diffusivity values, for checking or defaults, and not following the pattern for all other parameters which would normally have scale=US%m2_s_to_Z2_T . However, the new code instead (and correctly) sets scale=1 because these parameters are being used in MKS units instead of MOM6 units. In each case it seems a different approach could have avoided using MKS inside the model which would avoid breaking the pattern (which is documentation of how to do things). It would be all too easy (though unlikely) for one of these exceptions to the pattern to used as a template by someone else.

src/parameterizations/vertical/MOM_bkgnd_mixing.F90 Outdated Show resolved Hide resolved
src/parameterizations/vertical/MOM_bkgnd_mixing.F90 Outdated Show resolved Hide resolved
src/parameterizations/vertical/MOM_diabatic_driver.F90 Outdated Show resolved Hide resolved
src/parameterizations/vertical/MOM_kappa_shear.F90 Outdated Show resolved Hide resolved
  Documented numerous internal variables and their units in 9 vertical
parameterization modules (MOM_bkgnd_mixing, MOM_bulk_mixed_layer,
MOM_diabatic_aux, MOM_diabatic_driver, MOM_entrain_diffusive, MOM_kappa_shear,
MOM_regularize_layers, MOM_set_diffusivity and MOM_set_viscosity). This commit
includes the addition of units arguments in 9 unlogged get_param calls while 4
of these calls are now being scaled into the units used in MOM6 and then
unscaled when used as the default for other parameters.  A number of spelling
errors were also corrected in comments.  All answers and output are bitwise
identical.
Copy link
Member

@adcroft adcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adcroft
Copy link
Member

adcroft commented Nov 29, 2022

As expected, the difference

< MAX_RINO_IT = 25                !   [nondim] default = 50
---
> MAX_RINO_IT = 25                ! default = 50

appeared in following doc files:
coupled_AM2_LM3_SIS2/Intersperse_ice_1deg/MOM_parameter_doc.short coupled_AM2_LM3_SIS2/Intersperse_ice_1deg/MOM_parameter_doc.all coupled_AM2_LM3_SIS2/AM2_SIS2_MOM6i_1deg/MOM_parameter_doc.short coupled_AM2_LM3_SIS2/AM2_SIS2_MOM6i_1deg/MOM_parameter_doc.all coupled_AM2_LM3_SIS2/Concurrent_ice_1deg/MOM_parameter_doc.short coupled_AM2_LM3_SIS2/Concurrent_ice_1deg/MOM_parameter_doc.all ice_ocean_SIS2/SIS2_bergs_cgrid/MOM_parameter_doc.all ice_ocean_SIS2/SIS2_bergs_cgrid/MOM_parameter_doc.short ice_ocean_SIS2/SIS2_cgrid/MOM_parameter_doc.all ice_ocean_SIS2/SIS2_cgrid/MOM_parameter_doc.short ice_ocean_SIS2/Baltic_ALE_z_offline_tracers/MOM_parameter_doc.short ice_ocean_SIS2/Baltic_ALE_z_offline_tracers/MOM_parameter_doc.all ice_ocean_SIS2/OM_1deg/MOM_parameter_doc.all ice_ocean_SIS2/OM_1deg/MOM_parameter_doc.short ice_ocean_SIS2/Baltic/MOM_parameter_doc.all ice_ocean_SIS2/Baltic/MOM_parameter_doc.short ice_ocean_SIS2/OM4_025/MOM_parameter_doc.all ice_ocean_SIS2/OM4_025/MOM_parameter_doc.short ice_ocean_SIS2/Baltic_OM4_05/MOM_parameter_doc.short ice_ocean_SIS2/Baltic_OM4_05/MOM_parameter_doc.all ice_ocean_SIS2/OM4_05/MOM_parameter_doc.short ice_ocean_SIS2/OM4_05/MOM_parameter_doc.all ice_ocean_SIS2/SIS2/MOM_parameter_doc.all ice_ocean_SIS2/SIS2/MOM_parameter_doc.short ice_ocean_SIS2/Baltic_OM4_025/MOM_parameter_doc.short ice_ocean_SIS2/Baltic_OM4_025/MOM_parameter_doc.all ocean_only/nonBous_global/MOM_parameter_doc.short ocean_only/nonBous_global/MOM_parameter_doc.all ocean_only/DOME/MOM_parameter_doc.all ocean_only/DOME/MOM_parameter_doc.short ocean_only/benchmark/MOM_parameter_doc.all ocean_only/benchmark/MOM_parameter_doc.short ocean_only/global_ALE/z/MOM_parameter_doc.short ocean_only/global_ALE/z/MOM_parameter_doc.all ocean_only/global_ALE/layer/MOM_parameter_doc.short ocean_only/global_ALE/layer/MOM_parameter_doc.all ocean_only/global_ALE/hycom/MOM_parameter_doc.all ocean_only/global_ALE/hycom/MOM_parameter_doc.short ocean_only/single_column/KPP/MOM_parameter_doc.all ocean_only/single_column/KPP/MOM_parameter_doc.short ocean_only/single_column/BML/MOM_parameter_doc.short ocean_only/single_column/BML/MOM_parameter_doc.all ocean_only/single_column/EPBL/MOM_parameter_doc.all ocean_only/single_column/EPBL/MOM_parameter_doc.short ocean_only/circle_obcs/MOM_parameter_doc.all ocean_only/circle_obcs/MOM_parameter_doc.short

@adcroft adcroft merged commit 7055b7c into NOAA-GFDL:dev/gfdl Nov 29, 2022
@Hallberg-NOAA Hallberg-NOAA deleted the vert_param_doc_units branch February 2, 2023 13:31
marshallward pushed a commit that referenced this pull request Oct 25, 2023
Changes needed for introducing 3D tracer diffusivities
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants