Bring in latest main changes (GFDL to main 2022-07-21)#223
Merged
Conversation
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
- The PGI compiler was complaining about some `intent(in) :: CS`
in MOM_restart.F90. This was because of a line that changes the
state of data pointed to from within `CS`, but not `CS` itself:
CS%restart_field(n)%initialized = .true.
The strict interpretation is that `CS` is not modified because
`CS%restart_field` is a pointer to memory elsewhere. However, the
`intent(in)` indicates to the user/programmer that nothing changes
and since all arguments to the functions are `intent(in)` most entities,
including the PGI compiler, should be surprised that something changed
as a result of a passive "query" function. This strict interpretation
allows a devious hidden-change-of-state to occur.
- Changing the intent to `intent(inout)` has the consequence that the
new intent has to be propagated upwards through the code. And why should
a type be `intent(out)` for query functions?
- This commit removes offending lines that change the state. Apparently
we didn't need them!?
- This commit splits the run stage (~40 mins) into four smaller jobs. - Prior to this commit, typical turn around for a pipeline ~1 hour but two consecutive tests of this re-factoring finished in 23 minutes - The old run stage used all executables in one run script and so could not start until the pgi executable was ready, even though the gnu executable was ready 10 minutes earlier - Breaking the run stage into tests grouped by compiler allows some "tetris" to be played to minimize wait time between jobs - Implemented by making four copies of MOM6-examples to allow concurrency across the three compilers (gnu, intel, pgi), and a fourth for restart tests (gnu only) - The results are copied into sub-directories under results/ for later comparison, no longer using tar files for caching output - Added "needs:" so jobs can start when their dependency is ready - Re-ordered jobs in the .gitlab-ci.yml files so that the slowest compilation starts first (pgi) Considerations: - We can't run two tests in the same directory at the same time because of colliding output. Therefore, the old CI would launch tests of all experiments/configurations concurrently but would cycle through each group of tests (compilers, layout, etc.) sequentially, copying the output and reusing the same work space. Making copies of the work space is slow, and running more concurrent jobs requires more nodes to be available at once, so the "four" has been found to be optimal for gaea and current work load. - We only have six runners (on the six compilation nodes) which limits the pipeline to six jobs at once. Allowing multiple jobs per runner could remove this limitation but would impact the system more. - The restart testing is the slowest section of the run stage (even though for a subset of experiments). Separating restarts out allows more concurrency. Doing restart tests for more experiments and all compilers would be very expensive.
Added the overloaded interface set_initialized() to the MOM_restart module, to record that fields have been initialized, despite not appearing in a restart file. This will allow for a second call to set_initialized() after a call to query_initialized() to replicate the existing behavior of query_initialized() after MOM6 PR #149 (NOAA-GFDL#149) has been accepted. All answers are bitwise identical, but there is a new public interface.
+Add option to apply bottom drag as a body force
- Works with system python (2.7) and later (tested with 3.9)
- Processes #include without invoking cpp
- Currently naive regex without honoring/interpretting CPP macros
- Calculates link dependencies directly rather than invoking "make"
to infer the link list
Changes relative to the bash version of makedep:
- No longer adding/using a macro "SRC_DIRS" since it was only used to
record the arguments for regenerating the makefile. Now we record the
command line to achieve the same re-run ability.
- Adds new option "-e" to link all externals.
- This was the default before but I think in time we can make this
approach more intelligent and figure out which functions/subroutines
are used elsewhere.
- Processes F90 `include` to build dependencies
- Currently not tested with nested `include`s
Comments:
- As usual, the bizarre self-referencing in drifters.F90 needed some
special handling. The self-referencing is associated with code in CPP
block to generate local test programs. Now, we filter out circular
self-references rather than complain like make does. A large-separation
circular dependence will not be caught and lead to unpreditable
behavior. Such circular dependence is not something that makes sense to
support or allow.
- Using only native python featues, no packages, to avoid the "package
dependence" we are trying to avoid with makedep.
- This python version appears to be of orders of magnitude faster than
the bash version. Should have started here ...
Todo:
[ ] Improve some list/dictionary comprehensions. We use list
comprehensions quite a lot but a few "clunky" functions remain when
the first attempt at a comprehension failed. I never figrued out a
working dictionary comprehension.
[ ] Add a solution for the need to always link "externals".
- @marshallward suggested this fix to recursively follow F90 includes when building the list of dependencies. - Renamed the function from nested_h() to nested_inc() to be better describe function Co-authored-by: Marshall Ward <marshall.ward@gmail.com>
- The function includes_in_path() had many unused arguments and was left over from development. It turned out to be easy and clean to replace with a list comprehension.
- @marshallward reported that FMS2 has multiple versions of the same file int he search path. To avoid fatal errors, we are now allowing an ambiguous outcome and simple throwing out a warning that two files of the same name were encountered. Co-authored-by: Marshall Ward <marshall.ward@gmail.com>
This patch makes several changes to to provide better support for MacOS and BSD-like systems. Autoconf now includes macros to determine the following: * The name of sigsetjmp: `sigsetjmp` (BSD) or `__sigsetjmp` (Linux) * The size of `jmp_buf` and `sigjmp_buf`. Also renamed to `SIZEOF_*` to align with autoconf macro name conventions. The Linux defaults are retained in `posix.h`, but autoconf will now override these values. Two CI tests for MacOS have also been added, replicating the "stencil" and "regression" tests. The testing-setup has also been restructured to account for multiple platforms. Currently only Ubuntu and MacOS are tested.
Copy scaling factors for tracers in scale_factor_from_name, to accommodate the possibility that calls to register_segment_tracer might occur before calls to initialize_segment_data, as they do for temperature and salinity, or afterwards as they do for many user-defined tracers. This commit addresses the issue with the dumbbell subdomain test case not exhibiting proper rescaling for salinity as described in NOAA-GFDL#148. Some incorrect unit descriptions in comments were also corrected, and the tracer name comparisons for setting rescaling were made case-insensitive to handle some inconsistently cased name declarations. All answers without dimensional rescaling are bitwise identical.
Fix to reading of partial OBCs from file.
Added missing unit conversion factors in two calls to register_diag_field and modified the comments describing the units of some temperature and salinity variables in code that was recently added to the recently main branch of MOM6 via dev/ncar, reflecting the rescaling of temperatures and salinities that is now in the dev/gfdl branch. Without this change, these diagnostics will fail the dimensional consistency testing for temperature and salinity. All answers are bitwise identical, but there is a case change of the units of a variable in the available_diags files.
Removed a line resetting temperatures to 0 for the ZSTAR and SIGMA vertical coordinates in DOME2d_initialize_temperature_salinity, after they had been set to the intended values. github.com//issues/1560 highlights this bug and can be closed after this commit is merged into the main branch of MOM6. This changes the temperature fields (and ocean.stats files) for the flow_downslopes/z and flow_downslopes/sigma test cases, but because temperature does not influence density in these cases, the flows and salinities are unchanged.
Modified the DOME initialization to simplify some expressions and to use run-time parameters, rather than hard-coded values, when initializing the DOME test case. By default, all expressions are mathematically equivalent, but there are roundoff level changes in the topography and sponges due to the use of more generally valid expressions with runtime parameters and the replacement of some divisions for unit conversions by multiplication by a reciprocal. Because the DOME test cases are strongly nonlinear, these small changes cascade up to macroscopic differences, but these are of comparable magnitude to the differences between compilers. There are 10 new runtime parameters that appear in the MOM_parameter_doc.all files for the DOME test cases.
* 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/NOAA-GFDL/pull/107, but it caused problems with some of GFDL's Earth System Models, as noted in NOAA-GFDL#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.
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 #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.
GFDL to main 2022-07-21
Codecov Report
@@ Coverage Diff @@
## dev/ncar #223 +/- ##
============================================
+ Coverage 28.62% 33.99% +5.37%
============================================
Files 252 259 +7
Lines 73515 70267 -3248
Branches 0 13021 +13021
============================================
+ Hits 21040 23885 +2845
+ Misses 52475 41880 -10595
- Partials 0 4502 +4502
Help us with your feedback. Take ten seconds to tell us how you rate us. |
gustavo-marques
approved these changes
Aug 5, 2022
alperaltuntas
pushed a commit
that referenced
this pull request
Apr 2, 2026
* Add Upterm Session * Install SVN * With Root * Add Update * Minor COmmet Change
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.
This PR brings in latest mom-ocean:main commits. Already tested when I evaluated mom-ocean#1577
Testing: aux_mom.cheyenne_intel
no answer changes.