SegFault in ocean_only mode when RESOLN_SCALED_KH = False #1464
Labels
Comments
|
Hi Sam, this looks like a code issue that may need to be fixed so I will transfer it to the MOM6 repository. |
|
I think this code block is supposed to catch issues related to It's harder for me to comment on the other issue without a reproducibile test case, but it could be a vectorization problem similar to #1463. Perhaps rewriting those blocks to remove If you are using GFDL mkmf templates and have not already done so, then you could try replacing |
|
Thank you @marshallward. It does look like it might be the same issue as in #1463. And I am running on the Princeton cluster with a cluster-specific mkmf template, so that part is likely a compiler issue. |
Hallberg-NOAA
added a commit
to Hallberg-NOAA/MOM6
that referenced
this issue
Oct 8, 2021
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.
marshallward
pushed a commit
that referenced
this issue
Oct 15, 2021
* +(*)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 #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 #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.
marshallward
pushed a commit
to marshallward/MOM6
that referenced
this issue
Oct 20, 2021
* +(*)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.
guillaumevernieres
added a commit
to JCSDA-internal/MOM6
that referenced
this issue
Jan 25, 2022
* Don't need inout for ADp anymore * Update MOM_oda_incupd.F90 remove unused index bounds, and fix sum_h2 loop. * Minor fixes for the WW3 coupling option VR12-MA * Introduce changes needed for CVMix KPP module for the VR12-MA wave coupling option * fix doxygen error: add comments for wave method strings * fix omp error in KPP module: add lamult to shared clauses * logging/bulletproofing * fix indexing and potential division by small number * replace denominator of dlnCn_dxy with a more solid schme * add complementary test * Restrict printing warning message to root pe - This is restricting the warning message in MOM_restart.F90 to root pe per Bob. * Fix pointer initialization and other cleanup * +Add optional dZref argument to find_eta Added an optional dZref argument to the two find_eta routines so that the bathymetry can use a different reference height than is used for the interface heights. By default, they use the same reference height and all answers are bitwise identical. There is a new optional arguments (that is not yet being exercised with this commit, but has been tested and will be used in an upcoming commit) to two publicly visible interfaces. * Code cleanup in MOM_regridding Eliminated the depth argument to check_grid_column, along with some internal calculations that are never used. Also corrected some comments. All answers are bitwise identical. * Longer error string in get_polynomial_coordinate Lengthened a message string in get_polynomial_coordinate so that it will give a valid fatal error message in some cases of failures rather than resulting a segmentation fault with no message output. All answers are bitwise identical. * +Add optional Z_0p argument to int_density_dz Added an optional Z_0p argument to the various Boussinesq density integral routines as a new intercept for the linear expression between the (arbitrary) interface heights and pressure used for the equation of state routines. If omitted, this is equivalent to setting this intercept to 0, and all answers are bitwise identical. There are new optional arguments to several publicly visible interfaces (that are not yet being exercised with this commit, but have been tested and will be used in an upcoming commit). * Corrects the routine indicated in 2 error messages Corrected error messages to indicate the right subroutine in rescale_dyn_horgrid_bathymetry. All answers are bitwise identical. * Prep initialization for reference height changes Added the optional argument dZ_ref_eta to adjustEtaToFitBathymetry so that the bathymetry can use a different reference height than is used in this routine. Also changed the name and reversed the sign of the depth (now Z_bot) argument to find_interfaces, and a new Z_bottom array in MOM_temp_salt_initialize_from_Z. An extra unit conversion factor was eliminated from MOM_state_init_tests. All answers are bitwise identical, and all of these changes are internal to this module. * Cleanup of comments in MOM_ALE_sponge.F90 Extensive inconsequential cleanup in MOM_ALE_sponge.F90, including the elimination of a dozen unnecessary index-range elements of the ALE_sponge_CS and modifying a number of comments to be much more descriptive. This included the correction of numerous spelling errors and other typos in comments. All answers are bitwise identical. * (*)Fix dimensionally inconsistent MEKE beta calcs Corrected dimensional inconsistencies in the negligible thicknesses in the denominators of the expressions for the topographic betas in MEKE_equilibrium and MEKE_lengthScales. This could change answers in strange cases, but seems unlikely to do so (partly because it is in a max expression, and not added), and did not change any answers in the MOM6-examples test suite. * Added comments highlighting bugs Added comments denoted with "###" indicating bugs or conceptual errors in update_OBC_segment_data, horizontal_viscosity, thickness_diffuse and wave_speed. Only comments and the case of some indices are changed in this commit, and all answers are bitwise identical. Actually correcting these bugs would probably change answers in some cases. * more elegant formulation for cn_u/cn_v * better definition of cnmask * cosmetics * +Pass depth_tot to initialization routines Added depth_tot arguments to explicitly pass the total depth to various initialization routines for the thicknesses, sponges, or temperatures. These routines had previously used G%bathT for this role, but this change prepares to allow the reference depth for the bathymetry to change without requiring these routines to be changed to accommodate it. All answers are bitwise identical, but there are new arguments to about two dozen routines. * +Pass depth_tot around within MOM_MEKE Added a new argument, depth_tot, to three routines within MOM_MEKE.F90 to replace places where G%bathyT had been used. This new depth_tot variable is currently set to G%bathyT, but there is commented-out code suggesting a more appropriate expression based on the temporally evolving total thickness of the water column. All answers are bitwise identical, but there are new arguments to 3 private routines. * change VR12-MA wave_method string to EFACTOR * Refactor the way Langmuir entrainment and enhancement factor are computed and applied: (1) When available, pass the enhancement factor received from WW3. Otherwise, let CVmix compute the enhancement factor. (2) Instead of explicitly multiplying diffusivities and viscosities with the enhancement factor, let CVMix handle enhancement internally. * (*)Use summed thicknesses in OBC wave speed Changed the calculation of the external mode gravity wave speed used by the radiation open boundary conditions and in the Kelvin-wave test case to use the summed layer thicknesses rather than bathyT. This change will prevent taking the square root of negative thicknesses, even if there is wetting and drying so some of the bathymetry is above the mean sea level, and it is more physically accurate. This PR will at least partly address MOM6 issue mom-ocean#1447. Some solutions will exhibit changing answers, including the barotropic Kelvin wave test case, but after a broad discussion it was decided not to reproduce the previous solutions with a runtime parameter. * Testing: Replace bc with awk; framework support This patch adds three minor updates to the build and test systems: 1. A framework selection bug was fixed. The variable name was missing a $ token. 2. the FRAMEWORK variable was added to .testing/Makefile to facilitate testing of both FMS frameworks. 3. Restart half-periods no longer use `bc` to compute the duration in seconds; we now use awk, which is more commonly available. * Tiny typo. * Documented internal tidal mixing. - Also updated frazil ice documentation. * Fixed the \cite arguments? * Tiny typo. * Documented internal tidal mixing. - Also updated frazil ice documentation. * Fixed the \cite arguments? * Various dimension rescaling fixes correct scale argument in get_param and chksum calls add dimension scaling terms to some expressions add some chksum calls * dimension rescaling fixes for MOM_CFC_cap tracers * let register_tracer handle flux_units for MOM_CFC_cap tracers * correct prepending inputdir to CFC_BC_file * Fix units of *_visc_rem terms * Add run-time parameters CFC11_IC_VAL and CFC12_IC_VAL. * MOM_CFC_cap cleanup remove unused and unneeded local variables correct unit-related comments rework CFC_cap_fluxes to better align with Orr et al., GMD, 2017 * changes to ice_velocity_mask_update, initialization of ice velocity and ice thickness for new and restart simulations * removed tab from MOM_ice_shelf_initialize.F90 * (*)Fix discretization issues with USE_GME=True Corrected the inconsistently staggered expressions for the total depths used to scale away the GM+E backscatter scheme that is enabled with USE_GME. As a part of these changes, the nominal bathymetric depths have been replaced by the sum of the layer thicknesses, and parentheses were added to some of the GME expressions so that they will be rotationally invariant. Although there are no changes to the answers in the MOM6-examples test suite, answers will change in any cases where USE_GME is true, and GME_H0 is larger than the minimum ocean depth. This commit will address MOM6 issue mom-ocean#1466, which can now be closed. * (*)Use depths in DOME and dye tracer initialization Initialize the horizontal stripes of tracers in the DOME and dye examples based on the depth from the (flat) sea surface rather than the height above the seafloor. These are mathematically equivalent, but there could be small changes in the tracer distributions. Because these changes only impact passive tracers, all of the physical solutions are bitwise identical. * Restore Vt2 diagnostic calculation when requested and retain the functionality of prescribed constant enhancement. * (*+)New internal tide bathymetric parameters Replaced the nominal bathymetric depth with the sum of the layer thicknesses in the calculation of the quadratic drag acting on the internal wave field. Also replaced the hard-coded 1 m scale at which the drag is reduced with a new runtime parameter, INTERNAL_TIDE_DRAG_MIN_DEPTH, which is only logged if the internal tides are used and INTERNAL_TIDE_QUAD_DRAG=True, and made the maximum scale of the topographic roughness as a fraction of the bottom depth into the new runtime parameter INTERNAL_TIDE_ROUGHNESS_FRAC, whose default, 0.1, is the same as the previous hard-coded value. Several of the comments in the MOM_internal_tides module were also clarified or standardized. All answers in the MOM-examples test suite are bitwise identical, but answers could change and there could be changes in the MOM_parameter_doc files in some cases that use the internal tides module. * +(*)Add option for MEKE to calculate total depth Added the new run-time option, MEKE_FIXED_TOTAL_DEPTH, to use the actual total thickness instead the nominal depth in bathyT in several of the MEKE calculations. Also simplified and corrected a minor dimensional inconsistency in some expressions that effectively set masks when estimating the interface height diffusivities at tracer points for use in MEKE. By default, the answers in the MOM-examples test cases are bitwise identical, but answers could change in some cases due to the proper thickness weighting in the calculation of MEKE%Kh_diff. There are new entries in some MOM_parameter_doc files, so a minor spelling error correction in a MOM_parameter_doc entry was also included in this PR. * Testing: Disable 2018 answer flags The TC tests are still using 2018 answer reproducibility flags, which fail when aggressive initialization of allocatable arrays is enabled. In at least one case, the `mask_z` variable of the ALE sponge is incompletely initialized when `DEFAULT_2018_ANSWERS` is set. When unset, the array is fully initialized. This patch disables the 2018 answer flags and restores reproducibility on such platforms. * Go back to gfdl verison of MOM_boundary_update * More documentation - Jackson start. * Testing: Rotation support for MOM_read_data This patch adds an interface and several implementations of `MOM_read_data` and `MOM_read_vector` functions which support rotational testing. The `MOM_domain_type` now carries a scalar, `turns`, indicating the number of quarter-turns from the input grid to the model grid, and a pointer to the original unrotated `MOM_domain`. If this number is nonzero, then the input field is read into an array based on the input grid, and then rotated to a new array based on the model grid. This final result is returned by the function. If the domain's `turns` is zero, then it is assumed to be a call from a non-rotated domain and no rotation is applied. Functions outside of MOM (such as calls within drivers) do not apply this rotation. For the "domain-less" reads of 2d arrays, an explicit `turns` argument is supported. This only appears to be necessary in one part of grid initialization. This is now the third place where `turns` is tracked: the first is HI (horizontal index tracking) of the MOM grid, the second in `restart_CS`, and now in `MOM_Domain`. However, I believe this is a reasonable (if not necessary) place to track the domains while also avoiding a need for users to explicitly rotate fields every time `read_data` is called. * Always initialize drag_rate This patch now always initializes drag_rate, either to zero or to the bottom drag formula. It also changes the logic to always execute the second Strang splitting calculation. Removed unused code, including drag_rate_J15 and some commented code. * Add code that was deleted unintentionally * init local arrays, fix to axes and logging * (*)Offset G%bathyT by -G%Z_ref Added code to adjust the value of G%bathyT in the ocean_grid_type by -G%Z_ref, with changes scattered throughout the code to compensate. This does not impact the value of bathyT in the dyn_hor_grid. Using a non-zero value of REFERENCE_HEIGHT leads to mathematically equivalent solutions that differ at round-off but are demonstrably similar for values of REFERENCE_HEIGHT ranging from -1e4 m to 1e4 m, and in cases that do not use the less exact ..._2018_ANSWERS algorithms the solutions are qualitatively equivalent for values ranging from -1e8 m to 1e8 m. (For these more extreme values, the 64-bit roundoff in the free surface height calculation starts to become physically significant at of order 0.1 mm.) This new capability is useful for developing wetting and drying and for identifying algorithmic shortcomings, but because answers are not bitwise identical for various reference heights, it is not such a good candidate for fully automated testing. By default, all answers are bitwise identical and all output files are the same. * changed the block idetentation for consistency with MOM6 code style * changed identation MOM_ice_shelf_initialize.F90 to follow MOM6 convention * Testing: Set MALLOC_PERTURB_ to 1 The MALLOC_PERTURB_ parameter was used to initialize allocated arrays to unphysical values. Previously, we had set this to 256 which would initialize bytes to 0xff, and indirectly set all floats to NaN. However, MALLOC_PERTURB_ was undocumnted for values greater than 255, and we were relying on undefined behavior. In newer versions of glibc, a value of 256 does nothing and is equivalent to a value of 0. This patch changes the MALLOC_PERTURB_ value to 1, which sets bytes to 0xfe and tends to initialize them to an unphysically large value. Unfortunately we have temporarily lost the ability to initialize to NaN, but for now this is probably the best we can do. (Note that other compilers like Intel can explicitly initialize allocatables to NaN, so this update is more specific to the GCC configuration of our GitHub Actions setup than the test suite.) * Some background diffusivity text * Add documentation for correct_leap_year_inconsistency argument * clearer documentation for correct_leap_year_inconsistency flag * Changed identation in MOM6_ice_shelf_initialize.F90 to follow MOM6 convention * (*)Avoid uninitialized data use in find_interfaces Modified find_interfaces to avoid using uninitialized data when there is no input data in a column or only one layer of data and linear interpolation is nonsensical. This could change the initial conditions in some layer-mode cases, but in practice all solutions in the MOM6-examples test suite are bitwise identical, even though temporarily inserted error messages reveal that the conditions that triggered the modified code are being encountered. * More documentation - Jackson start. * Some background diffusivity text * add references to KPP enhancements methods * update all calls of KPP_compute_BLD and KPP_calculate to support the passage of lamult enhancement factor * More on background mixing. * Fixed forgotten \f$ * Fixing a citation * Still more syntax issues * ALE sponge mask_z halo fixes In the ALE sponge, the `mask_u` and `mask_v` masks are constructed from `mask_z`, but also require valid halo data on their E/W or N/S boundaries. Although a `pass_var` function is called on `mask_z` before computing these masks, this function will not populate the halos of `mask_z` if there is no adjacent data, e.g. a non-reentrant boundary or a land-masked tile. And even though `mask_z` was initialized to zero, this was undone by the internal dellocation/reallocation of the array inside of `horiz_interp_and_extrap_tracer` (although the actual result appears to be compiler dependent). There are two major changes in this patch: * The FMS-based `horiz_interp_and_extrap_tracer` function no longer does a deallocate/reallocate of its output arrays, and now simply assumes they are unallocated. The output arrays are also explicitly declared as intent(out). This change clarifies that only the compute domains of `mask_z` and associated fields are updated, although it doesn't fully resolve the issue described above. * The ALE sponge code now explicitly initializes the halo values of mask_z before interpolating the mask_u and mask_v masks. This ensures that `mask_[uv]` boundary values are disabled on points where no halo data is available (and hence no halo updates from `pass_var`. When the data is available, sensible values will replace these zeros. These changes prevent anomalous values of mask_z from entering the halos, and ensuring that `mask_[uv]` contain sensible values. A similar operation should not be required by the tracer fields, since the zero-halo values in the mask will correctly disable these values when no adjacent field is available for halo updates. * ALE sponge: Only update fields on uv over masks Currently, fields from `horiz_interp_and_extrap_tracer` are interpolated onto [uv] points under the assumption that halos have sensible values. This is generally true due to the preceding `pass_var` call, but it may not be valid if the halo is not updated, e.g. along a boundary or land-masked tile. The mask is designed to be set to zero when this happens, but it could still result in an assignment of an uninitialized value to the fields (`sp_val_[uv])`. Although the value is not used, it can produce compiler warnings and errors under strict debugging builds. This patch moves the calculation of `sp_val_[uv]` so that it is only conditionally computed when the mask is set to 1. * Infra: MOM_read_data rename to read_field This patch removes the `read_data_infra` alias in `MOM_io` to the infra's identically named `MOM_read_data`, and instead renames the infra function to `read_field`. This is primarily done to avert the inability of the PGI compiler to resolve the alias in the namespace. It also provides a slightly more consistent namespace: 1. It resembles the `write_field` functions in the infra layer. 2. It avoids reuse of the `read_data` name, used in FMS. 3. It allows the MOM framework layer to preserve its `MOM_` suffix, as expected for a layer intended for use within MOM. 4. `read_field` sheds any explicit reference to `MOM_`, helping to identify the infra layer as a generic interface to its framework. * Fix initialize_ice_thickness_from_file for ISOMIP Adds a test and warning message to avoid a fatal error when the ISOMIP (or other ice-shelf) test case ice shelf thicknesses are initialized from a file that has the ice thickness but not a mask variable, correcting the behavior that was recently changed back to its previous behavior. If the mask variable exists it is read, but if not it is set based on the thicknesses. All answers are bitwise identical in test cases that worked before, and the ISOMIP test cases once again work with the input files that they used previously. * Use allocate with source to initialize arrays Use the allocate call's source optional argument to initialize 611 arrays in 62 files. This change simplifies and shortens the code and will make it easier to identify uninitialized allocated arrays. This change was only made to arrays that were already being initialized on the same line as the allocate or shortly thereafter. All answers are bitwise identical. * Done with internal diffusion? * Adding information to MOM6 warning. - where is the water depth negative? * Several small things, including fix to sponge verbosity. - Also, added a link to OBC wiki page. - Working around bibtex hashing issue I don't understand, renaming some unused tags. - Making MOM_barotropic when the water depth goes negative. * Changing verbosity in MOM_horizontal_regridding.F90 * +(*)Add ALTERNATE_FIRST_DIRECTION Added a new runtime parameter, ALTERNATE_FIRST_DIRECTION, to cause the first direction in the directionally split operators (such as the continuity solver, the barotropic solver, and the tracer advection) to alternate with every dynamic timestep. This includes changes to record this direction in the restart file, so that the model will reproduce across restarts. In addition a previously uninitialized logical in offline_advection_layer is now being initialized similarly to other similar variables in the MOM_offline_main module. By default, all answers in the MOM6-examples are bitwise identical, but there is a new runtime parameter and a new non-mandatory field in the MOM6 restart files, and there might be changes in offline tracer advection runs. * refactoring reflection * Testing: Configurable list of test targets `make all` (the default rule) builds the executables specified by `BUILDS`. This was hard-coded, but can be promoted to a user-defined configuration for user-defined builds. This should be seen as a simple alias to `make build/${b}/MOM6`. The `repro` build was also incorrectly in the list, even though it was also conditionally added. It has been removed from the default list. A similar modification was made to CONFIGS, which select the "tc" experiments. The default is still to do a `tc*` glob. Documentation was also added. * Testing: Individual tc tests; no wildcards Macros for generating individual rules for the tc's were added. This was generalized to support the contents of CONFIGS, which is now a user-defined parameter. The wildcard rules have now been replaced with more explicit rules, in preparation for the merge of TESTS and TEST_TYPES. A method for excluding tests has been added for tc3 support, and could be extended to future tests. * Testing: Generalized dimension testing The Makefile rules were extended to support multiple iterations of dimension testing. Examples shown below: * test.dim Run all dimension tests * test.dim.l Run all L dimension tests * tc2.dim Run all tc2 dimension tests * tc2.dim.l Run the tc2 L dimension test Also, TESTS and TEST_TYPES were merged into a single variable, and the old plural test names (e.g. test.grids) were removed and are now handled as singular tests => test.grid. The GitHub actions testing was updated to reflect these new non-plural names. It will take some iteration to confirm that they are working. * fix units in variable declaration * (*) N2_floor init fix when FGNV streamfn disabled The `N2_floor` buoyancy frequency was left unset when `KHTH_USE_FGNV_STREAMFUNCTION` was disabled. This could potentially cause errors, such as floating point exceptions. Ideally we would restrict the calculations of `hN2_[uv]` to when the streamfunction is enabled. But due to propagation to these values to `hN2_[xy]_PE` fields, which may be used outside of the streamfunction, it is perhaps best to just initialize `N2_floor` to zero. Although this would mostly likely be initialized to zero in production, there is a chance that this could modify answers derived from random initialization. Thanks to @wfcooke for reporting this. It was also independently (and inexplicably) detected during removal of MEKE pointers, suggesting some memory volatility. * (+*)Fix MEKE advection bug 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. * +Add BAROTROPIC_TIDAL_SAL_BUG to fix a tide bug 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. * correct long_name for tracer_dfy for passive tracers when diag_form == 1 * Particle API (mom-ocean#1504) * 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 <[email protected]> * Recover topography clipping when not specifying MINIMUM_DEPTH PRs mom-ocean#1428 and mom-ocean#1457 extended the topography clipping to allow flooding but missed the use case for positive-only depths where the MASKING_DEPTH parameter alone was in use. There were two bugs: 1. The new code assumed that MINIMUM_DEPTH would be deeper than MASKING_DEPTH (which is intuitive). However, the point of MASKING_DEPTH was only to specify the determination of the land mask. The new code assigned depths the value of MASKING_DEPTH which broke cases that were using MASKING_DEPTH as documented and were leaving MINIMUM_DEPTH=0. 2. The values of variable masking_depth were altered and subsequently not consistent with the logged parameters. A warning was issued but the behavior was nevertheless not as intended. Changes: 1. Removed the test that masking_depth > min_depth, and warning 2. Adjusted the condition and assigned value when clipping depths. This now uses the shallower of min_depth and masking_depth to decide when to clip and for the value to use otherwise. The expression for the land mask is unaltered. 3. Corrected documentation to retain original purpose of MASKING_DEPTH 4. Added some comments for declaration with units 5. Added some clarifying comments in code Todo: - resolve the need for the alternative negative depth pathway associated with the 0.5*min_depth expression. * Correct logical associated with NW2 tracers - @klindsay28 spotted two issues for the NW2 tracers 1. Use of the wrong logical variable 2. Incorrect comment - Using the wrong logical meant that the ideal age pacakge was being called in addition to the NW2 package, but did not affect the NW2 tracers themselves. * adds missing _ (mom-ocean#1519) * Remove alternate topo-clipping for unexpected parameter combination - Following feedback from @herrwang0, we have removed the possibility for a user to try using negative depths without the MASKING_DEPTH parameter being set appropriately. This avoids the asymmetric use of MINIMUM_DEPTH that was proposed. A FATAL is now issued. - Corrected a spelling error in a comment. - Removed an unused "use" that should have been done in previous commit. * Working on boundary layer docs. * Done with EPBL docs? * FMS1: Don't create IO domains for single-PE domain 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. * User/wfc/remap scheme (mom-ocean#1503) * 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 <[email protected]> * +*Change defaults for 3 parameters to better values (mom-ocean#1509) 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 mom-ocean#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 new parameter FATAL_INCONSISTENT_RESTART_TIME (mom-ocean#1511) 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 (mom-ocean#1512) * +(*)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. * +Make 37 optional arguments in src/core mandatory 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 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. * +Optional just_read_params args are now 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. * +Argument cleanup in vertical parameterization code 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. * Use FMS2 `file_exists`, remove domain args 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. * fix low mode in tidal_mixing * +Eliminate unused arguments in diagnostics code 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. * Always create single-threaded files in 1 PE runs 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. * +Optional arg cleanup in horizontal param code Cleaned up 13 falsely optional or unused arguments in the horizontal parameterization code, and related changes. This includes: - Made the previously optional OBC pointer arguments that were always being used in calls to 3 routines in MOM_lateral_mixing_coeffs.F90 into mandatory arguments. Because these are pointers, the deciding factor of whether to use them is really whether they are associated. - Made an internal optional argument that was always being used mandatory in 2 routines in MOM_internal_tides.F90. - Made 2 internal optional arguments that were always being used mandatory in thickness_diffuse_full(). - Eliminated the unused deta_tidal_deta argument to calc_tidal_forcing() and made the m_to_Z argument to the same routine mandatory. The former value is instead obtained by a call to tidal_sensitivity. - Eliminated 3 unused arguments and made an optional argument that was always used mandatory for find_deficit_ratios() in MOM_regularize_layers.F90. * cpu_clock_id: synchro_flag arg changed to logical This patch modifies the `cpu_clock_id` interface so that the `synchro_flag` argument is converted into a platform-agnostic logical flag. The current implementation requires the synchronization flag to be defined using FMS norms (zero-bit) and also would force users to follow FMS predefined flags for other values. This patch changes the sync flag to a logical, and modifies the default flag to enable synced clocks (based on `clock_flag_default`). `synchro_flag` is also renamed to `sync` for simplicity. * Eliminate GET_ALL_PARAMS in hor_visc_init (mom-ocean#1536) * Eliminate GET_ALL_PARAMS in hor_visc_init Added do_not_log arguments to get_param calls in MOM_hor_visc.F90 that are only used conditionally, and eliminated the unlogged GET_ALL_PARAMS runtime parameter and get_all variable in hor_visc_init(). By design, all logging of parameters after this commit is identical to before, even for variables that are inactive and therefore should not be logged. In several places, there were some problems, mostly with the GME code, that have been noted in comments marked with '###'. Also cleaned up the code alignment and eliminated unneeded temporary variables in a few places in hor_visc(). All solutions are bitwise identical, and no output is changed. * Restore temporary variables Undid changes that eliminated temporary variables to facilitate performance profiling, and restored the "Knuth" convention about the placement of the line breaks relative to "+" in these expressions. Nothing changes. * Add weighted d[uv]_dt_str diagnostics (mom-ocean#1539) This adds four new diagnostics building on the wind stress acceleration diagnostics du_dt_str, dv_dt_str (from mom-ocean#1437) - their thickness-weighted versions: h_du_dt_str, h_dv_dt_str (completing the set of diags from 3D thickness x momentum diagnostics mom-ocean#1398) - their viscous remnant fraction: du_dt_str_visc_rem, dv_dt_str_visc_rem (completing the set of diags from Visc_rem_[uv] multiplied momentum budget diagnostics ocean-eddy-cpt#10) Nora did some quick tests with the CPT NeverWorld2 setup, which confirm that online and offline multiplication by 1) h or 2) visc_rem_[uv] coincide (up to 1) interpolation error and 2) numerical noise, respectively), and illustrated this beautifully with some figures that accompanied the first of the three commits that were squashed into one. * +Argument cleanup in vertical diffusivity code Cleaned up 26 falsely optional or unused arguments in the vertical diffusivity code, and related changes. Several descriptive comments were also corrected, including the correction of the units of 10 variables related to CVMix_KPP. This commit includes: - Made the Kd_int arguments to set_diffusivity() and 3 subsidiary routines mandatory and reordered the arguments so that the non-optional arguments come before the grid types - Made the halo_TS and double_diffuse arguments to set_diffusivity_init() mandatory. - Made the Time argument to ALE_sponge() mandatory. - Made the Kd and Kv arguments to calculate_CVMIX_conv() mandatory. - Removed the unused halo argument to adjust_salt(). - Removed the unused Kddt_convect argument to full_convection(). - Made the halo arguments to full_convection()and smoothed_dRdT_dRdS() mandatory. - Made the useALEalgorithm argument to geothermal_init() mandatory. - Removed the unused initialize_all arguments to Calculate_kappa_shear() and Calc_kappa_shear_vertex(). - Removed the unused I_Ld2_1d and dz_Int_1d arguments to kappa_shear_column(). - Made 3 arguments to calculate_projected_state() mandatory and reordered the arguments accordingly. - Eliminating the unused skip_diags arguments to calculateBuoyancyFlux() and extractFluxes(), which are now effectively always false. All answers are bitwise identical, and no output is changed. * +Move rotate_dyn_horgrid to MOM_dyn_horgrid module Moved the routine rotate_dyngrid() from the MOM_transcribe_grid module to rotate_dyn_horgrid() in the MOM_dyn_horgrid module so that this routine can also be used at some point by SIS2 to implement rotational consistency testing, and also to reflect that this routine only works with types from its new module. The two routines are the same apart from some added comments, and the old name of rotate_dyngrid() is still available from MOM_transcribe_grid via a module use statement. All answers are bitwise identical. * +Reduce use of dyn_horgrid in initialize_MOM Minimized the dependence on dyn_horgrid in initialize_MOM by working directly with the horizontal index type whereever possible and by moving the calls that create the MOM_grid_type earlier in the routine, to limit the duration of the dyn_horgrid_type, and to better co-locate grid-related parameters in the parameter_doc files. Also uses the new interface to rotate_dyn_horgrid from the MOM_dyn_horgrid module in place of the rotate_dyngrid interface from the MOM_transcribe_grid module. All answers are bitwise identical, but the order of some entries in the MOM_parameter_doc files has changed. * (*)Fix compile-time issues with MOM_sum_driver.F90 Modified drivers/unit_drivers/MOM_sum_driver.F90 to compile with the latest version of the rest of the MOM6 code by using the proper types in the various initialization calls, and verified that it runs as intended. * synch with gfdl * makeitwork Co-authored-by: NoraLoose <[email protected]> Co-authored-by: alperaltuntas <[email protected]> Co-authored-by: Alistair Adcroft <[email protected]> Co-authored-by: Phil Pegion <[email protected]> Co-authored-by: jiandewang <[email protected]> Co-authored-by: Marshall Ward <[email protected]> Co-authored-by: Gustavo Marques <[email protected]> Co-authored-by: Kate Hedstrom <[email protected]> Co-authored-by: Robert Hallberg <[email protected]> Co-authored-by: Raphael Dussin <[email protected]> Co-authored-by: Niki Zadeh <[email protected]> Co-authored-by: Matthew Harrison <[email protected]> Co-authored-by: Raphael Dussin <[email protected]> Co-authored-by: Marshall Ward <[email protected]> Co-authored-by: Keith Lindsay <[email protected]> Co-authored-by: Olga Sergienko <[email protected]> Co-authored-by: Spencer Jones <[email protected]> Co-authored-by: Cory Spencer Jones <[email protected]> Co-authored-by: Alistair Adcroft <[email protected]> Co-authored-by: Navid C. Constantinou <[email protected]> Co-authored-by: wfcooke <[email protected]> Co-authored-by: Nora Loose <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Issue Summary:
(1) Warning needed for conflicts in variable mixing parameter allocation logic
(2)
RES_SCALE_MEKE_VISCacts asTrueeven when set toFalseHi all,
I was running into an issue in
ocean_onlymode where I was getting the following SegFault error:The line in
MOM_hor_visc.F90is:942 if (CS%res_scale_MEKE) meke_res_fn = VarMix%Res_fn_h(i,j)Thus,
VarMix%Res_fn_h(i,j)is called without being allocated. The allocation would be inMOM_lateral_mixing_coeffs.F90:Indeed the issue is solved by setting any of
RESOLN_SCALED_KH,RESOLN_SCALED_KHTH,RESOLN_SCALED_KHTRtoTruein the input file. There should be a warning that one of these variables must be set ifRES_SCALE_MEKE_VISC = True.However, I'm confused because I get this error even when
CS%res_scale_MEKE = .false.(set by input paramRES_SCALE_MEKE_VISC). In debug mode, the SegFault does not occur and I can confirm thatCS%res_scale_MEKE = .false.. But with normal optimizations I get the SegFault. Does anyone know what could cause this to happen?Thanks,
Sam Ditkovsky
The text was updated successfully, but these errors were encountered: