PR to main (Jan 2022) #1555
Closed
PR to main (Jan 2022) #1555
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
Added the new runtime option MEKE_ADVECTION_BUG and corrected a bug in the calculation of the vertically integrated transport for the advection of the MEKE field when MEKE_ADVECTION_FACTOR > 0. The default is to fix the bug, so answers can change in some cases by default, and in those cases there are changes to the MOM_parameter_doc files, but this option is not widely used and there are no answer changes in the MOM6-examples test suite. This PR addresses MOM6 issue mom-ocean#1465, which can be closed once this PR is merged into dev/gfdl.
Added a runtime flag, BAROTROPIC_TIDAL_SAL_BUG, to fix a sign error in the tidal self-attraction and loading anomalies in the barotropic solver when tides are enabled. The default is to keep the previous bug so that answers do not change, but this default will be changed after solutions have been corrected. This commit partly addresses MOM6 issue mom-ocean#1496, but it should only be considered to be properly handled once the default has been changed to avoid this bug. This commit will change the MOM_parameter_doc files in cases where this bug matters, but by default all answers are bitwise identical.
…ugfix (+*)Fix MEKE advection bug
+Add BAROTROPIC_TIDAL_SAL_BUG to fix a tide bug
* first draft API based on Luyu's code * fixed various errors * Code for particles in MOM.F90 * moved particles_run into dynamics step * added particles_end * Fixed particle time * Fixed some documentation * Further documentation edits * converted pointers to allocatables in particles_gridded * Remove trailing space * Further doxygen tweaks * another trailing space * removed set_time Co-authored-by: Cory Spencer Jones <spencerjones@login4.cluster>
FMS mpp domain creation is done in the `clone_MD_to_d2D` function, and currently an IO domain is always created within the MPP domain. This has caused problems with single-PE runs in FMS1, where the `write_field` logic was not able to reach the part which removes halo data if an IO domain was present, and halo data was being written to the restart files. This issue arose when `PARALLEL_RESTARTFILES` was set to `True` for single-PE tests. This does not appear to be a problem with FMS2, and no action is needed by the FMS team.
* Revert "Implement changes suggested by @Hallberg-NOAA" This reverts commit 8f4af3d. * Revert "*Corrected the clock as seen by diabatic processes" This reverts commit bc6c6e6. * Revert "*Use rho_ref in finite volume PGF density calcs" This reverts commit 48e90d0. * Update of MOM6 code to allow SPEAR to reproduce previous answers. Updates MOM code to dev/gfdl as of 2/7/2019 (6dd6f52) and reverts 3 answer changing modifications. Update produced by git revert 8f4af3d git add src/tracer/MOM_neutral_diffusion.F90 git revert --continue git revert bc6c6e6 git add src/core/MOM.F90 git revert --continue git revert 48e90d0 git add src/equation_of_state/MOM_EOS.F90 git revert --continue Some conflict resolution was needed. * Optional use of differing restoring piston velocities for temp and salt * Correction to ePBL code to mitigate blowup. Also "corrects" some spelling differences in variables (MStar vs mstar) * add MJHarrison-GFDL salt_flux_add_fix to SPEAR codeset from https://github.com/MJHarrison-GFDL/MOM6/compare/salt_flux_add_fix * Added ability to change remapping scheme. Added call for parameter REMAPPING_SCHEME to allow changes to be made. Previously was hardwired to PLM. Call for MOM_grid_int was issing a global_indexing=F argument Tests indicated that oda clocks were being called out of order and crashing the test, so I commented them out. * Removed timers, Changes name for REMAPPING_SCHEME parameter Changes REMAPPING_SCHEME to ODA_REMAPPING_SCHEME Removed timers that were causing blowups * Adds documentation for ODA_remapping_scheme Changes second argument to get_param calls to the module name. Co-authored-by: Matthew Harrison <Matthew.Harrison@noaa.gov>
NOTE: Initial submission included 4 parameters. `PARALLEL_RESTARTFILES` was dropped after an issue was detected. Full commit log history below. * +*Change defaults for 4 parameters to better values Updated the defaults of 4 run-time parameters (INTERNAL_WAVE_SPEED_BETTER_EST, PARALLEL_RESTARTFILES, EPBL_MLD_BISECTION, and BBL_USE_EOS) to more appropriate values. In each case, the previous default was simply the older setting, and not the better recommendation. It also adds logic determining whether SIMPLE_TKE_TO_KD does anything, and only log its setting if it does. These default changes were discussed by the MOM6 consortium as a whole in June, 2021 and were widely agreed to. In addition this commit removes the old obsoleted runtime parameter ORIG_MLD_ITERATION, and obsoletes the runtime parameter LARGE_FILE_SUPPORT, and it adds comments describing several real variables and their units. Because this changes several default values, it will change answers unless these parameters are explicitly set in the MOM_input files. However, because MOM6-examples PR #344 does set these values to their old defaults where they are used, no answers are changed in the MOM6-examples regression suite, although there are changes to the MOM_parameter_doc files. * +Reverted default for PARALLEL_RESTARTFILES Reverted the default for PARALLEL_RESTARTFILES to False because the TC restart testing was having problems with this set to True. This is surprising because PARALLEL_RESTARTFILES = True has been used in production runs for many years now without any indication that MOM6 fails to reproduce across restarts in this mode, so this could be an issue with the TC testing. In the mean time, reverting this default will allow the other changes to be accepted while this curious behavior is explored separately. Several comments related to PARALLEL_RESTARTFILES were also updated for consistency. All answers are bitwise identical.
Add the new runtime parameter FATAL_INCONSISTENT_RESTART_TIME, which if true causes the model to compare the restart time read from a restart file with any value provided as a time_in to MOM_initialize_state and issue a fatal error if they differ. In ocean-only mode, this input time is read from a ocean_solo.res file, following FMS behavior. The default value simply uses the specified time, replicating the previous behavior. If set to true, this would prevent a problem with the time-history that recently occurred in a series of high-resolution runs that were shared between several groups, where the nominal times were repeated. All answers are bitwise identical, but many MOM_parameter_doc files have a new entry.
* +(*)Fix bug when RES_SCALE_MEKE_VISC = True Fix a bug that can lead to segmentation faults when RES_SCALE_MEKE_VISC is true, as noted in MOM6 issue mom-ocean#1464, and add better error handling to detect related problems. The refactoring that is a part of this may also avoid some problems with optimized code even when RES_SCALE_MEKE_VISC it false, as described in MOM6 issue mom-ocean#1463. In addition, logic was added so that the value of RES_SCALE_MEKE_VISC is only logged if USE_MEKE is true. All answers are bitwise identical in cases that worked, including the MOM6-examples test suite, but there are some changes in the MOM_parameter_doc files, due to an irrelevant parameter no longer being logged. * (*)Initialize CS%res_scale_MEKE Initialize the logical element res_scale_MEKE in the hor_visc_CS even if there is no Laplacian viscosity, so that a self-consistency test does not use an initialized value. Also improved some comments. All answers are bitwise identical for all cases that successfully ran before, including in all cases in the MOM6-examples test suite.
Made 37 optional arguments that are always present in calls into non-optional arguments to routines in the src/core directory. Many of these are pointer arguments related to things like open boundary conditions, so if these types are not to be used, they are simply not allocated. In several cases, this required the order of the arguments to be shifted around, but the various types of the arguments should prevent the model from compiling if the calls (e.g., in user-modified code that is not in the github repository) are not changed equivalently. Also eliminated 3 internal arguments in MOM_barotropic.F90 that are always hard-coded to the same values (the maximize argument to BT_cont_to_face_areas()) or are never used (the guess arguments to uhbt_to_ubt() and vhbt_to_vbt()), along with the code that they would exercise. All answers and output are bitwise identical.
Refactored solo_driver/MOM_driver.F90 to move logically self-contained blocks of code into separate subroutines within this file to improve the readability of the code and to reduce the number of global variables. This started out as an exercise to explore the use of the F2008 block / end block construct to reduce the scope of variables, but the code ended up being cleaner just using traditional subroutines contained in this file. All answers are bitwise identical and all output files are unaltered.
+Make 37 optional arguments in src/core mandatory
Made 44 previously optional just_read_params arguments into mandatory arguments and renamed them just_read to avoid duplicated variables. This change should be minimally disruptive, as these optional arguments were always being provided. Some argument documentation blocks were also reformatted or the comments in them were corrected. All answers are bitwise identical, but some arguments have changed.
Cleaned up 27 falsely optional or unused arguments in the vertical parameterization code, and related changes. This includes: - Eliminating the symmetrize arguments to set_viscous_BBL and set_viscous_ML, which are now effectively always true. - Making the Waves and OBC pointer arguments mandatory in several routines where they were always being supplied. These are pointers, so the test of whether they should be used can be based on whether they are associated. - Adding error messages about unassociated Waves types that would be used. - Eliminating the unused Waves argument to KPP_init. - Eliminating unused arguments that energetic_PBL inherited from the bulk mixed layer code, and simplified some disabled debugging code. - Making the optics argument to opacity_end mandatory. - Making the h argument to get_Langmuir_Number mandatory and rearranged the arguments to this routine to put the optional arguments last, following the practice elsewhere in the MOM6 code. Also standardized the case of several variables in the MOM_wave_interface.F90 code to facilitate searches. - Eliminating the unused US argument to CoriolisStokes. All answers are bitwise identical, and no output is changed.
This patch uses the FMS2 `file_exists` function when using the FMS2 infra. Previously, the FMS1 version of this function was being used. This patch also removes the `mpp_domain` and `no_domain` arguments from the direct `file_exist` calls, which were not used by any known MOM6 configurations. (Nor would we expect them to be, since explicit references to FMS should not exist outside of the infra layer.) Since FMS2 does not use these arguments, their removal also creates a more meaningful interface between the two frameworks. Motivation: An issue with the FMS1 `file_exists` under the FMS2 infra was discovered in the UFS model on Hera. It was only reproducible in submitted jobs, and not for interactive jobs, and only with the GCC 9.2 compiler. (Other GCC versions were not tested.) One potential explanation is that it is related to the `save` attribute of the domain pointer, `d_ptr`. In the case above, `d_ptr` pointed to the MOM input domain for the failed cases. For the other working cases, `d_ptr` pointed to a `NULL()` value and behavior was normal. It is possible that `d_ptr` is inconsisently updated when FMS1 and FMS2 IO operations are used together, which should probably be considered undefined behavior.
Eliminated the unused optional OBC argument to write_energy() and several unused optional arguments to wave_speed() and wave_speeds() that are set instead via arguments to wave_speed_init() that store these values in a wave_speed_CS type. Also made the optional row_scale argument to tridiag_det() and the tracer_CSp argument to write_energy() that were always present in calls into mandatory arguments. All answers are bitwise identical, and solutions do not change.
FMS1: Don't create IO domains for single-PE domain
Modified create_file to always specify that the threading will be for a single PE to do the writing to a single file if there is only a single ocean PE, to avoid some incorrect behavior inside of the FMS IO modules. This can fix restart problems in some single-processor cases with an inconsistent setting of the optional threading argument, but in all cases that ran correctly before, all answers are bitwise identical.
PR mom-ocean#1538 merge to dev/gfdl
Commented out the problematic lines that Andrew Shao flagged in his review of MOM6 dev/gfdl PR #37. The model runs perfectly well in short offline-tracer test runs, and even gives bitwise identical output, perhaps because no layers were being abruptly flooded to 10^13 times their previous values. These omitted lines could change answers in some cases, so the lines in question have been retained in case the offline tracer code needs to be debugged layer and these mysterious (and seemingly unhelpful) lines turn out to have been necessary. All answers in the non-offline-tracer runs are bitwise identical.
Replace `.gt.` with `>`
Corrected uninformative comments describing the some of the arguments to the stub routines in config_src/external/GFDL_ocean_BGC/generic_tracer.F90. The updated comments are consistent with how they are used in calls to these routines and with the underlying actual generic_tracer code if they are actually documented there. The previous comments had been added to existing undocumented code to satisfy the MOM6 requirement that there be a doxygen comment describing every argument to every routine, in the hopes that someone with familiarity with the generic tracer could work amend them to something more appropriate. However, "Unknown" is neither an accurate nor an informative description, and current MOM6 standards would demand that we reject any new code contributions with such poor interface documentation. All answers are bitwise identical, and only comments have changed.
- A do_not_log depends on a logical that is only set conditionally. This initializes that logical when the corresponding parameter is not being read. - Unfortunately, this change MOM_parameter_doc.all for the coupled models. The .all pipeline uses the gnu compiler which was initializing this logical as .true. and thus logging the new parameter when it should not have been. Intel and PGI were initializing with .false.
Eliminated 4 unnecessary 3-d allocatable arrays and 8 2-d diagnostic arrays in CorAdCalc, and simplified the code calculating these diagnostics by using the post_product_[uv] and post_product_sum_[uv] routines. Also grouped the calls that allocate the memory that is still needed for diagnostics. This commit also includes a few other minor changes to clean up the documentation of variable intents and unit documentation in a handful of other places in the code: - Add intent declarations to the arguments to chksum2d() and chksum3d() - Corrected incorrect scale arguments for two (untested) checksum calls. - Corrected the documented units in several comments. All answers and output are bitwise identical.
+(*)Rescale opacity and avoid segmentation faults
Deleted four lines in the offline tracer code that had recently been commented out, along with the comments describing them. Further conversations had led to a consensus that these lines served no useful purpose, and are not worth keeping in the code, even in comments. Several excess spaces were also eliminated in MOM_offline_aux.F90. All answers and output are bitwise identical.
Use the por_face_area[UV] in the effective thickness calculations in zonal_face_thickness and merid_face_thickness, so that they are more consistent with their use elsewhere in the code for the relative weights in calculating the barotropic accelerations. Because these por_face_area arrays are still 1 in all test cases, the answers are unchanged in any test cases from before a few weeks ago, but there could be answer changes in cases that are using the very recently added capability (in PR #3) to set fractional face areas. This change was discussed with Sam Ditkovsky, and agreed that there is no reason to keep the ability to recover the previous answers in any cases that use the recently added partial face width option. This commit also expanded the comments describing the h_u and h_v arguments to btcalc(), zonal_face_thickness(), and merid_face_thickness() routines, the diag_h[uv] elements of the accel_diag_ptrs type and the h_u and h_v elements of the BT_cont_type. All answers and output are bitwise identical in the MOM6-examples test suite and TC tests, but answer changes are possible in cases using a very recently added code option.
+(*)Get the offline tracer mode working again
* Adds option to homogenize forces and fluxes fields - Adds functions to do global averages on U and V grids in MOM_spatial_means - Adds functionality to average all forcing and fluxes fields in MOM_forcing_types - Adds flag to average all forcing and fluxes in MOM.F90 - This new feature is primarily for running in single column like configurations with the coupler, which requires perfectly equal forcing across all cells. * Fixing ustar calculation in homogenize_mech_forcing - Adds in irho0 and sqrt that were missing in homogenize mech forcing. * Updates to homogenize_forcings options. - Correct issues in global_area_mean_u and global_area_mean_v to work with symmetric and rotated grids. - Add options to compute mean ustar or compute ustar from mean tau. - Add subroutines to replace averaging blocks in MOM_forcing_type. * Minor formatting updates - Move a division and reformat alignment in MOM_spatial_means.F90. - Remove a unused parameter in MOM_forcing_type.F90 - Reformat misalignment of an "if-block" in MOM_forcing_type.F90 * Remove obsolete netSalt flux homogenization - netSalt has been removed so no longer needs homogenized in the fluxes. * Fix 2d mean on U grid to use U mask * Remove whitespacace * Add do_not_log option to UPDATE_USTAR get_param
* Hydrostatic initialization in ice cavities
- Iteratively solve for the initial ice shelf displacement
in cavities by calculating the pressure at the current
displacement depth using the unperturbed profile.
- This change should obsolete TRIM_IC_FOR_PSURF and DEPRESS_INITIAL_SURFACE for
ice shelf applications and should work for arbitrary equations of state.
- Existing implementations (e.g. ISOMIP) should turn off the above options
in order to exercise this feature.
- This code change should not impact non ice-shelf configurations
or those with either of the above two options.
* Addresses a number of issues identified in code review.
* add appropriate intent to ice_shelf_query
* fix unit scaling comments
Co-authored-by: Marshall Ward <marshall.ward@noaa.gov>
* Refresh attempt to get rid of NetCDF calls * Fix comments * Set netCDF attrs in MOM_horizontal_regridding This patch sets the following netCDF attributes in the function `horiz_interp_and_extrap_tracer_record` via `read_attribute`. * `missing_value` (as `_FillValue`) * `scale_factor` * `add_offset` This resolves some issues related to uninitialized values. * read_variable_2d in horizontal remapping This patch extends the `read_variable` interface to include 2d array support, in order to facilitate domainless I/O via netCDF calls. This is far from the best implementation (e.g. read_variable_2d introduces another `broadcast` alongside the original one in the horizontal regridding) but it addresses the immediate issues with `MOM_read_data()`. * set default scale factor to 1 * add missing start/count arguments * Update MOM_io.F90 * Manage optional args in read_variable_2d This patch modifies read_variable_2d so that the size() tests of the optional arguments are applied before the call to nf90_get_var. The tests are also wrapped inside present() flags to avoid checking unassigned variables. Thanks to Robert Hallberg for the suggestions. Co-authored-by: Matthew Harrison <Matthew.Harrison@noaa.gov>
Eliminated a number of instances of non-standard syntax from that described in https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide. The changes include enforcing the MOM6-standard 2-point indentation convention, replacing 'if(A)' with 'if (A)', and changing logical comparison syntax like '.gt.' to '>' or '.eq.' to '=='. An old commented out block of code for debugging (detected by its use of non-standard syntax) was also eliminated. All answers and output are bitwise identical.
Slightly modified the recently added subroutine calc_sfc_displacement to document the units of its variables and to follow the MOM6 code standards from https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide. The changes include white-space corrections, changing logical comparison syntax like '.gt.' to '>', and explicitly identifying where array syntax is used. All answers and output are bitwise identical.
Changed the name and sign convention for the dzInterface argument to remap_all_state_vars to reflect the convention used in the regridding code and to reflect the fact that this is always a vertical displacement. This change eliminates a subtle array-syntax whole-array multiplication (by -1.) in one call to remap_all_state_vars (this clearly violated MOM6 code standards), and it corrects an actual sign error that will change answers (perhaps from a state of catastrophic failure) in the code for the REGRID_ACCELERATE_INIT=True option if REMAP_UV_USING_OLD_ALG is also true and the initial velocities that are being remapped are non-zero. Also added comments describing the real variables inside of remap_all_state_vars to help clarify what they do. Fortunately the situation where answers change seems like a very uncommon combination of settings (it is possible that no one has ever tried this), and all answers in the MOM6-examples test suite are bitwise identical.
- I division by zero was encountered when using the back-scatter settings (negative viscosity) in NeverWorld2. It appears hrat_min(I,J) can be zero. Reading the code, it makes sense that hrat_min can be zero. The division was previously made conditional in 14971b4 also when using backscatter, but then only one part of the denomitor was used in the conditional. - I'm not sure why the backscatter setup is repeatedly hitting these edge cases or specific line of code. - This fix uses the entire denominator in the conditional.
- A previous re-factor for optimization introduced some inconsistent capitalization. This made it hard to understand the code, especially with some arrays being re-used at different grid locations.
* additions for stochastic physics and ePBL perts * cleanup of code and enhancement of ePBL perts * Update MOM_diabatic_driver.F90 remove conflict with dev/emc * Update MOM_diabatic_driver.F90 further resolve conflict * Update MOM_diabatic_driver.F90 put id_sppt_wts, etc back. * add stochy_restart writing to mom_cap * additions for stochy restarts * clean up debug statements * clean up code * fix non stochastic ePBL calculation * re-write of stochastic code to remove CPP directives * remove blank link in MOM_diagnostics * clean up MOM_domains * make stochastics optional * correct coupled_driver/ocean_model_MOM.F90 and other cleanup * clean up of code for MOM6 coding standards * remove stochastics container * revert MOM_domains.F90 * clean up of mom_ocean_model_nuopc.F90 * remove PE_here from mom_ocean_model_nuopc.F90 * remove debug statements * stochastic physics re-write * move stochastics to external directory * doxygen cleanup * add write_stoch_restart_ocn to MOM_stochastics * add logic to remove incrments from restart if outside IAU window * revert logic wrt increments * add comments * update to gfdl 20210806 (#74) * remove white space and fix comment * Update MOM_oda_incupd.F90 remove unused index bounds, and fix sum_h2 loop. Co-authored-by: pjpegion <Philip.Pegion@noaa.gov> Co-authored-by: Marshall Ward <marshall.ward@noaa.gov> * Fussing with zotero.bib. Getting a warning about a repeated bibliography entry for adcroft2004. Rob thinks this is a hash failure. * Still fussing with zotero.bib - it was complaining about the (unused) Kasahara reference. * Several little things, one is making sponge less verbose. - Pointing to OBC wiki file from the lateral parameterizations doc. - Using the MOM6 verbosity to control the time_interp verbosity. - Making the check for negative water depths more informative. * return a more accurate error message in MOM_stochasics * Working on boundary layer docs. * Done with EPBL docs? * Undoing some patches from others * Cleaning up too-new commits * Adding in that SAL commit again. * correction on type in directory name * Added some to vertical viscisity doc. * Cleaned up whitespace leftover from porous topomerge. - Spacing within expressions was uneven and made multiplation look like POW functions. Leftover from merging #3. - No answer changes. * Fix out-of-bounds k index in PPM flux - An errant use of the porous face area led to an out-of-bounds k-index reported in #19. - Closes #19 * Adding Channel drag figure * Take cite out of figure caption. * Copyright year 2022
Codecov Report
@@ Coverage Diff @@
## main #1555 +/- ##
==========================================
- Coverage 29.07% 28.94% -0.13%
==========================================
Files 239 242 +3
Lines 71740 71553 -187
==========================================
- Hits 20855 20714 -141
+ Misses 50885 50839 -46
Continue to review full report at Codecov.
|
|
Sorry, this was from the wrong source branch, and also I did not expect these regressions. Will close and reopen (hopefully soon). |
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.
January 2022 update to MOM6 main branch
Features
Bugfixes
Refactor
Infrastructure
Contributors: