Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(*) Merge dev/gfdl as of 06 Nov 2021 #18

Merged

Conversation

gustavo-marques
Copy link
Collaborator

@gustavo-marques gustavo-marques commented Nov 8, 2021

This PR includes changes from the latest main candidate NOAA-GFDL#1507. I also includes the new wind stress acceleration diagnostics introduced in https://github.com/NOAA-GFDL/MOM6/pull/1539.

Answers for the NW2 test are identical to those obtained before #12

From July to October dev/gfdl was not correct (changing answers) and we incorrectly merged that into dev/cpt in August. The bug was in the initial topography and masking. It's a small detail in the topography that changes so no need to worry about science results - just not bitwise. This PR will bring the NW2 answers back to what they were prior to #12

MJHarrison-GFDL and others added 30 commits July 26, 2021 10:12
…L-20210723

update to GFDL 20210723 main branch
Several axes_grp pointers in register_diag_field were null()-initialized
at declaration, e.g.

  type(axes_grp), pointer :: axes => null()

This implicitly declares the pointers with `save`, which preserves its
value across calls.   First call excepted, these axes will not be
initialized to null().

In this instance, this was causing d2 axes to be created when they
should not have existed.

This patch eliminates the implicit `save` attributes and applies
explicit initialization when required.
…change_in_CFC_PR

* Recover answer change from CFC PR
  - Testing with an ice shelf (e.g. ISOMIP) and using
    the SIGMA_SHELF_ZSTAR coordinate would have caught this bug.
remove unused index bounds, and fix sum_h2 loop.
Update MOM_oda_incupd.F90
Fix to commit 893a06e which broke the SIGMA_SHELF_ZSTAR option
Hallberg-NOAA and others added 26 commits October 14, 2021 19:27
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.
…ate-2021-10-04

Dev gfdl main candidate 2021 10 04
+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.
  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.
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.
Use FMS2 `file_exists`, remove domain args
cpu_clock_id: synchro_flag arg changed to logical
* 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.
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.
@codecov-commenter
Copy link

Codecov Report

Merging #18 (8b9d2d4) into dev/cpt (7868fdd) will decrease coverage by 0.14%.
The diff coverage is 18.80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           dev/cpt      #18      +/-   ##
===========================================
- Coverage    29.29%   29.15%   -0.15%     
===========================================
  Files          235      239       +4     
  Lines        70799    71533     +734     
===========================================
+ Hits         20742    20855     +113     
- Misses       50057    50678     +621     
Impacted Files Coverage Δ
...g_src/drivers/solo_driver/MESO_surface_forcing.F90 0.00% <0.00%> (ø)
config_src/external/drifters/MOM_particles.F90 0.00% <0.00%> (ø)
...nfig_src/external/drifters/MOM_particles_types.F90 0.00% <0.00%> (ø)
src/ALE/MOM_regridding.F90 21.45% <0.00%> (+0.09%) ⬆️
src/ALE/coord_adapt.F90 0.00% <0.00%> (ø)
src/ALE/regrid_interp.F90 1.41% <ø> (ø)
src/core/MOM_PressureForce.F90 62.50% <ø> (+1.63%) ⬆️
src/core/MOM_PressureForce_Montgomery.F90 6.41% <0.00%> (+0.03%) ⬆️
src/core/MOM_grid.F90 61.68% <0.00%> (ø)
src/core/MOM_transcribe_grid.F90 24.63% <0.00%> (ø)
... and 167 more

Continue to review full report at Codecov.

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

@gustavo-marques gustavo-marques merged commit f81aa85 into ocean-eddy-cpt:dev/cpt Nov 8, 2021
gustavo-marques pushed a commit that referenced this pull request Feb 14, 2022
Bug fix for reading First_direction from restart
@gustavo-marques gustavo-marques deleted the merge_dev_gfdl_06nov2021 branch February 17, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.