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

+(*)Make unit_scale_type arguments non-optional #29

Merged
merged 4 commits into from
Dec 12, 2021

Conversation

Hallberg-NOAA
Copy link
Member

This PR is a series of commits that make the previously optional
unit_scale_type arguments to 28 routines mandatory, mostly related to
the initialization of the bathymetry or the grid metrics. This change realizes
something that was envisioned when these arguments were originally introduced,
but coordination with SIS2 means that they had to be optional when added to
routines that are also used by SIS2. Now that SIS2 is using all of these
arguments, they can be made non-optional. The resulting code should be much
easier to understand. Because of a specific ODA initialization bug that was
corrected in these commits, answers might change slightly in nearly massless
layers for non-Boussinesq models with ODA enabled using the src/ocean_data_assim
code, but no such case exists in any of our test cases. All answers and output
in the MOM6-examples test suite are bitwise identical.

The commits in this PR include:

  • 3162bd086 +Make US arguments non-optional for 28 routines
  • 59c592649 (*)Provide US arguments to 4 existing calls
  • e48f4a7f4 +Add the new routine unit_no_scaling_init

  Added the public interface unit_no_scaling_init() to the MOM_unit_scaling
module to initialize a non-scaling unit_scale_type without requiring a
param_file_type argument.  This should be useful for certain trivial unit tests,
and it makes it more plausible to make the unit_scale_type arguments mandatory.
As a part of this change, the new internal subroutine set_unit_scaling_combos()
was carved out of unit_scaling_init to avoid code duplication.  Also added
comments describing the effective units of the various scaling factors with the
standard MOM6 notation.  All answers and output are bitwise identical.
  Provide optional US arguments to 4 existing calls to set_grid_metrics() and
MOM_initialize_topography(), enabling these arguments to become mandatory in the
next commit.  In some cases this requires a call to unit_no_scaling_init() or
the removal of a call to rescale_dyn_horgrid_bathymetry().  In addition, a
mis-scaled value was corrected in the initialization of the ODA control
structures private thickness array; this latter issue is not a problem for
Boussinesq models without dimensional rescaling (i.e. the tested cases), but
could change answers in other cases with data assimilation.  All answers in
the MOM6-examples or TC test cases are bitwise identical.
  Made the unit_scale_type arguments non-optional for 28 routines.  These
arguments had been optional in the first place to manage the coordination
between the MOM6 and SIS2 repositories, but SIS2 has been using these optional
arguments for several years now, and they can be made mandatory without imposing
any disruptions.  This change simplifies and clarifies the code.  All answers
and output are bitwise identical.
@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #29 (fbafcb1) into dev/gfdl (2319139) will decrease coverage by 0.04%.
The diff coverage is 36.09%.

Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl      #29      +/-   ##
============================================
- Coverage     29.05%   29.00%   -0.05%     
============================================
  Files           240      240              
  Lines         71293    71235      -58     
============================================
- Hits          20714    20665      -49     
+ Misses        50579    50570       -9     
Impacted Files Coverage Δ
src/core/MOM_checksum_packages.F90 31.29% <0.00%> (-1.04%) ⬇️
src/ice_shelf/MOM_ice_shelf.F90 0.00% <0.00%> (ø)
src/initialization/MOM_state_initialization.F90 20.73% <ø> (ø)
src/ocean_data_assim/MOM_oda_driver.F90 0.00% <0.00%> (ø)
src/user/DOME_initialization.F90 0.00% <0.00%> (ø)
src/user/ISOMIP_initialization.F90 0.00% <0.00%> (ø)
src/user/Kelvin_initialization.F90 0.00% <0.00%> (ø)
src/user/Phillips_initialization.F90 0.00% <ø> (ø)
src/user/shelfwave_initialization.F90 0.00% <0.00%> (ø)
src/user/user_initialization.F90 0.00% <ø> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2319139...fbafcb1. Read the comment docs.

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14331 ✔️

@marshallward marshallward merged commit 86b91dc into NOAA-GFDL:dev/gfdl Dec 12, 2021
@Hallberg-NOAA Hallberg-NOAA deleted the mandatory_US_args branch January 3, 2022 13:35
@Hallberg-NOAA Hallberg-NOAA added refactor Code cleanup with no changes in functionality or results bug Something isn't working labels Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor Code cleanup with no changes in functionality or results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants