GFDL to main 2022-07-21 #1577
Merged
GFDL to main 2022-07-21 #1577
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removed unused module use statements for EOS_type, calculate_density or calculate_density_derivs in 20 files. All answers are bitwise identical.
Added comments documenting the units of the variables in diagnoseMLDbyEnergy and slightly refactored this routine to clean up its call to calculate_density and eliminated a redundant array of interface depths. Also fixed several spelling errors in comments. All answers and diagnostics are bitwise identical.
Documented the units of variables as they actually appear in subroutine calls to various equation of state or density integral routines, eliminating the potentially confusing lists of alternative units in comments. Only comments are changed, and all answers are bitwise identical.
Revised the calculation of the drho_dT and drho_dS diagnostics to use dimensional consistency testing, along with the newer interface to calculate_density that takes dimensionally rescaled arguments. With this change, the units of most the variables in this section of code match their descriptions in comments, although there is still the local re-use of some 3-d arrays as temporaries with units that do not match. All answers and output are bitwise identical.
Refactored MOM_density_integrals to use the newer calculate_density_1d() and calculated_stanley_density_1d() interfaces to the equation of state routines, and to thereby shift all related dimensional rescaling into MOM_EOS.F90. Also revised the comments describing the arguments to a number of the equation of state routines to eliminate confusing options and clearly indicate the units of each input and output variable. As a part of this change, the units of the rho_ref argument to calculate_stanley_density_1d were changed from [kg m-3] to [R ~> kg m-3] to match the equivalent routine calculate_density_1d(). Because this does not appear to have been used previously, this should not be a problem, and answers will not change unless a dimensional consistency test is underway. All answers are bitwise identical, but there is one minor change to the rescaled units of one apparently unused optional argument.
Modified the comments describing the units of the arguments to int_density_dz_wright, int_spec_vol_dp_wright int_density_dz_linear and int_spec_vol_dp_linear so that they reflect the units as they are used in practice where they are called from analytic_int_density_dz or analytic_int_specific_vol_dp. All answers are bitwise identical.
This commit has several interface changes to some little-called equation of state routines to follow the patterns set by the most commonly used equation of state routines. All answers in test cases are bitwise identical. - Added calculate_TFreeze_1d to the overloaded interface calculate_TFreeze, with dimensional rescaling of its arguments taken from its EOS_type argument and an optional two-element domain, rather than two mandatory integer arguments used with calculate_TFreeze_array. The older interface is also retained within the overloaded interface to calculate_TFreeze. - Modified calculate_density_scalar and calculate_stanley_density_scalar to use units of [R ~> kg m-3] for its rho_ref optional argument, following the pattern from calculate_density_1d. These arguments were not previously used. - Renamed the internally visible routine calculate_density_second_derivs_array to calculate_density_second_derivs_1d and changed its argument list to take an optional two-element domain, rather than two mandatory integer arguments, to follow the pattern set by calculate_density_derivs_1d. Because this routine was only being called in two places the older interface is not being preserved in the overloaded interface calculate_density_second_derivs. - Renamed the internally visible routine calculate_compress_array to calculate_compress_1d and changed its argument list to take an optional two-element domain, rather than two mandatory integer arguments, to follow the pattern set by calculate_density_derivs_1d. Because this routine was only being called in one place the older interface is not being preserved in the overloaded interface calculate_compress. - Eliminated some unnecessary local variables (mostly p_scale) for brevity and code clarity. - Modified two calls to calculate_density_second_derivs in thickness_diffuse_full to use its revised interface. - Modified one call to calculate_compress in build_slight_column to use its revised interface.
Corrected a bug in the calculation of drho_dS_dP and drho_dT_dP in the calculate_density_second_derivs routines, where the inverse of the correct rescaling was being used. However, these routines are only called in a very few places and these particular output fields are not being used, so this bug does not alter any existing MOM6 solutions.
Modified comments in 20 files to prepare for the addition of dimensional rescaling of temperature and salinity. All answers are bitwise identical.
Added rescaling conversion factors for temperature and salinity to the EOS_type and added code to all of the EOS routines that work with dimensionally rescaled arguments to handle these new rescaling factors. Also added new optional arguments to int_density_dz_wright and int_spec_vol_dp_wright to handle rescaling temperature and salinity. There are also many places in MOM_EOS.F90 where comments are altered to reflect the new rescaled units. However, for now these new rescaling factors are hard-coded to 1, so there is no new rescaling yet, and all answers are bitwise identical. There are, however, new optional arguments in two public interfaces.
Use the new, clearer interfaces for calculate_TFreeze in MOM6.F90 and MOM_diabatic_aux.F90, and use tv%C_p instead of fluxes%C_p in several places. tv%C_p is not used outside of the code under the MOM6 src directory, whereas fluxes%C_p is, so it is preferable to use tv%C_p to permit clean rescaling of the temperature-related variables without touching anything outside of the src directories. All answers are bitwise identical.
Modified comments in 5 more files to prepare for the addition of dimensional rescaling of temperature and salinity. All answers are bitwise identical.
Added optional scaling arguments to the set_up_ALE_sponge routines, to allow input fields to be rescaled before use. This change is necessary to permit the dimensional rescaling of temperature, salinity, and other tracers because of the way that some versions repeatedly read new values from files as the runs progress. All answers are bitwise identical, but there are new optional arguments to public interfaces.
Added dimensional rescaling for temperature and salinity, as determined by the new runtime parameters C_RESCALE_POWER and S_RESCALE_POWER. With this change there are 4 new elements in the transparent unit_scale_type, and these are widely used in the code. In addition, ### other files were added that had checksum calls or diagnostics rescaled by these new factors, and where comments were changed, but were otherwise unaltered as a result of the new dimensional rescaling. There will be another commit very shortly that completes the changes and leads to fully functional dimensional rescaling for temperatures and salinities, but these will involve more extensive code or interface changes, but this commit will be useful for any possible git-bisection of any potential changes that do not involve dimensional rescaling. All solutions in existing test cases are bitwise identical.
This commit completes the dimensional rescaling for temperature and salinity, and it has been confirmed that the solutions for the existing test cases pass these tests. There are new unit_scale_type arguments to several publicly visible interfaces, mostly related to temperature initialization. There is also a new optional argument, conc_scale to the register_tracer calls to specify the conversion that should be done to tracer concentrations during output. Additionally, there are new entries in the incorrect units on some runtime parameters for user code were corrected in the MOM_parameter_doc files for some test cases. All answers are bitwise identical in the MOM6 regression suite, including when the temperature and salinity rescaling are enabled.
Corrected the units in comments describing some temperature and salinity variables that had been accidentally omitted from the previous commits in this sequence. Also rescaled some local temperature and salinity variables used in seamount_initialize_thickness and added missing unit conversion factors for several diagnostics in MOM_oda_incupd. All answers are bitwise identical.
Works with rescaled temperatures and salinities for the internal calculations in MOM_temp_salt_initialize_from_Z, taking advantage of the recently corrected rescaling capabilities in horiz_interp_and_extrap_tracer. This commit also includes these other closely related changes. - Modified convert_temp_salt_for_TEOS10 to work with rescaled temperature and salinity units - Eliminated unused land_fill argument from determine_temperature - Slightly refactored tracer_z_init_array to avoid needing an extra set of do loop through the 3-d array when a scale argument is present All answers are bitwise identical, but there are changes in the arguments to publicly visible interfaces.
The wrong MOM_read_data interface was being used: a 2D slice of a 3D field was expected, but the interface for a 2D field was being called.
This commit adds new functionality to the MOM_EOS module to support the dimensional rescaling of temperatures and salinities. - Added the new routines cons_temp_to_pot_temp and abs_saln_to_prac_saln to convert between forms of temperature and salinity variables, respectively. These work on arrays of rescaled variables. - Added the new optional argument scale_from_EOS to calculate_TFreeze_scalar, to indicate that this routine should use the unit scaling stored in their EOS_type arguments. - Also corrected some comments throughout MOM_EOS.F90. All answers are bitwise identical, but there are new public interfaces.
Rescaled the units of some salinities in dumbbell_initialize_thickness and added comments or corrected the unit descriptions in comments describing several variables. All answers are bitwise identical.
+Revise equation of state interfaces for consistency
This patch introduces new features to support unit testing of the MOM6 source code. The patch includes two new modules (MOM_unit_testing, MOM_file_parser_tests), two new classes (UnitTest, TestSuite), and a new driver (unit_testing). A UnitTest object consists of the following: * The test subroutine * Test name (for reporting) * A flag indicating whether the test should fail (FATAL) * An optional cleanup subroutine The UnitTest objects are gathered into a TestSuite object, which provides a batch job for running all of its tests. The use of these features is demonstrated in a driver, unit_tests, which runs the tests provided in the MOM_file_parser_tests module This patch also includes changes to the ".testing" build system. * The optional FCFLAGS_COVERAGE has been removed from the testing Makefile. Instead, a new "cov" target is optionally built if one wants to check the coverage. It is currently based on "symmetric". * A new "unit" target has been added to run the unit testing driver and report its code coverage. * GitHub Actions has been modified to include the unit driver test. * The gcov output now includes branching (-b), which allows reporting of partial line coverage in some cases. * codecov.io "smart" report searching has been replaced with an explicit setting of the root directory (-R) and *.gcda paths. Other minor changes: * MOM_coms include an infra-level sync function (sync_PEs) as a wrapper to mpp_sync (or others in the future).
MOM_file_parser unit test implementation
Fixed two minor bugs in the MOM6 output when run with Z_RESCALE_POWER not equal to 0. All solutions are bitwise identical, but some of the diagnostic output files have minor changes. With these corrections, several output files are now unaltered by internal dimensional rescaling. The specific bug fixes are: - Avoid using a rescaled depth as the vertical coordinate label in z-space output files. Only the coordinate label is impacted, but this bug fix avoids the chance of having silly values for this coordinate. - Rescale the depths back to mks units before taking their checksum for storage in the MOM_sum_output file. With this bug, the depth list file might be unnecessarily recreated with a new run with different scaling, but the file itself is fine.
Add dimensional rescaling of temperature and salinity
Fix data read for on-grid interpolation
* Change dumbbell initialization * Change in Dumbbell Layer Mode
Made post_data_1d_k publicly visible once again, rather than requiring calls to use post_data, to support backward compatibility with older versions of the ocean_BGC code. This interface was removed from public visibility as a part of github.com//pull/107, but it caused problems with some of GFDL's Earth System Models, as noted in #168. All answers are bitwise identical for any cases that compiled before.
Overloaded MOM_tracer_chksum and MOM_tracer_chkinv with a simpler interface that takes the tracer registry as an input argument, rather than requiring that its elements be unpacked outside of the call. This was done as an overload to the existing interface to avoid breaking backward compatibility, but it seems likely that in due course the older, more complicated interface can be obsoleted. All answers are bitwise identical, but there are new interfaces to provide tracer debugging capabilities.
Added calls to write tracer checksums from step_MOM_tracer_dyn when DEBUG=True. Also updated a number of MOM_tracer_chksum and MOM_tracer_chkinv calls in MOM_offline_main and MOM_tracer_hor_diff to use the new and streamlined form of the interfaces. All answers are bitwise identical, but there are additional output lines when debugging is enabled.
Corrected two allocated tests for OBC-related arrays in advect_y, bringing them into conformity with what was already being done in advect_x. Given the nature of these changes, it seems likely that any case that would have worked before will give bitwise identical answers, but that segmentation faults might now be avoided in certain configurations using OBCs.
|
The regression tests and performance monitor are failing because they are looking for |
|
Sorry I didn't test this before, but trying all the rescalings in my Bering Sea domain causes trouble. It is only the temp and salt scalings which are problematic. It looks like the wind stress is different before SIS_C_dynamics when changing only the temperature scaling, same when changing only the salinity scaling. Also the apply_sponge tv%T and tv%S. |
|
The stresses are a red herring, not leading to differences in ice velocity. The sponges do lead to trouble, though. |
Codecov Report
@@ Coverage Diff @@
## main #1577 +/- ##
==========================================
+ Coverage 28.62% 33.99% +5.37%
==========================================
Files 252 259 +7
Lines 73515 70267 -3248
Branches 0 13022 +13022
==========================================
+ Hits 21040 23885 +2845
+ Misses 52475 41880 -10595
- Partials 0 4502 +4502
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Hallberg-NOAA
added a commit
to Hallberg-NOAA/MOM6
that referenced
this pull request
Jul 22, 2022
Corrected the dimensional rescaling factors in two calls to set_up_ALE_sponge_field for temperature and salinity for time-varying fields being read in from an input file. These had been given the inverse of the correct values. An optional scale argument was also added (with its default value) in the call to set up ALE sponge velocities, for greater clarity of what this call is doing. This commit should address an issue noted by Kate Hedstrom when evaluating the first draft of PR mom-ocean#1577 from dev/gfdl to main. All answers are bitwise identical in cases where dimensional rescaling of temperature and salinity are not being applied, and answers with the rescaling of temperature and salinity should now reproduce those without the rescaling.
|
@kshedstrom, I think that I know what happened with the ALE sponges, and I have added a PR to dev/gfdl at NOAA-GFDL#173 that I think should fix the problem you are seeing. |
|
Yes, with that PR I will approve this PR. Thanks! |
kshedstrom
approved these changes
Jul 22, 2022
Corrected the dimensional rescaling factors in two calls to set_up_ALE_sponge_field for temperature and salinity for time-varying fields being read in from an input file. These had been given the inverse of the correct values. An optional scale argument was also added (with its default value) in the call to set up ALE sponge velocities, for greater clarity of what this call is doing. This commit should address an issue noted by Kate Hedstrom when evaluating the first draft of PR mom-ocean#1577 from dev/gfdl to main. All answers are bitwise identical in cases where dimensional rescaling of temperature and salinity are not being applied, and answers with the rescaling of temperature and salinity should now reproduce those without the rescaling.
jiandewang
approved these changes
Jul 28, 2022
alperaltuntas
approved these changes
Jul 28, 2022
@marshallward, would you please clarify this a bit:
Thank you! |
|
Yes, mkmf still works and no one needs to change their workflows. The only change here is that the autoconf build (and the test suite in |
sanAkel
approved these changes
Jul 29, 2022
|
All approved, this will be merged in. Thanks for your help, everyone. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GFDL to main candidate branch (2022-07-18)
Features
Bugfixes
Usability
Refactoring
+Revise equation of state interfaces for consistency NOAA-GFDL/MOM6#118 (@Hallberg-NOAA)
MOM_file_parser unit test implementation NOAA-GFDL/MOM6#119 (@marshallward)
Simplify set_grid_metrics_from_mosaic NOAA-GFDL/MOM6#127 (@Hallberg-NOAA)
Replace findloc() with user-defined find_index NOAA-GFDL/MOM6#136 (@marshallward)
Re-factored GFDL gitlab pipeline NOAA-GFDL/MOM6#144 (@adcroft)
+Eliminated 3 unused elements of the surface type NOAA-GFDL/MOM6#145 (@Hallberg-NOAA)
Fix PGI warnings about intent for restart_CS NOAA-GFDL/MOM6#149 (@adcroft)
gitlab-ci: add concurrent jobs in run stage NOAA-GFDL/MOM6#150 (@adcroft)
+Add set_initialized NOAA-GFDL/MOM6#152 (@Hallberg-NOAA)
*+DOME initialization cleanup and bug fix NOAA-GFDL/MOM6#165 (@Hallberg-NOAA)
+(*)Improve tracer debugging NOAA-GFDL/MOM6#162 (@Hallberg-NOAA)
Dimensional Consistency
Meta
Contributors: