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

dev-master-candidate-2019-12-17 #1046

Merged
merged 366 commits into from
Jan 8, 2020
Merged

Conversation

adcroft
Copy link
Collaborator

@adcroft adcroft commented Dec 17, 2019

This is the PR onto dev/master promised on last weeks dev call. The main changes are:

  • More test mechanisms added to Travis-CI
  • Two new TCs (test configurations)
  • Further dimensional consistency of expressions
  • Bug fixes:
    • Units of MEKE advection
    • Multiple OBC
    • Downsampled transport fix
    • OMP directives
    • Multiple diagnostics

The parameter documentation is updated so I've pushed a corresponding branch to MOM6-examples: dev-master-candidate-2019-12-17

The length of the list of PRs is misleading. I suspect almost every file has been touched at some point since the last PR to dev/master. There are 365 commits hidden in here:
#1040 from Hallberg-NOAA/rescale_diag_fix
#1039 from adcroft/fix-units-for-meke-advection
#1037 from Hallberg-NOAA/revise_OBCs
#1031 from marshallward/tc4_fix
#1034 from marshallward/adiabatic_clock_id_fix
#1032 from Hallberg-NOAA/rescale_cleanup
#1030 from marshallward/travis_regression_fix
#1019 from Hallberg-NOAA/density_rescale
#1026 from marshallward/nan_merge
#1023 from marshallward/visc_max_chksums
#1021 from marshallward/variable_init_fixes
#1022 from nikizadehgfdl/fix_downsampled_transport_diags
#1025 from marshallward/dev_merge
#1004 from adcroft/tc-reconfig
#1020 from marshallward/mk_fixes
#1018 from marshallward/ale_ineq_1016
#1014 from marshallward/meke_diag_arrays
#1012 from marshallward/omp_test
#1010 from nikizadehgfdl/fix_openmp_compile
#1011 from marshallward/popcnt
#1009 from marshallward/negzero_minmax_log
#1005 from marshallward/make_parallel
#1003 from adcroft/make-label-output
#1002 from marshallward/diag_scaling_p2
#1000 from MJHarrison-GFDL/ALE_sponges_ongrid
#1001 from adcroft/make-src-dependency
#996 from marshallward/codecov_config
#993 from marshallward/travis_stderr
#994 from wfcooke/user/wfc/ePBL_fix
#991 from marshallward/tendency_scaling
#989 from adcroft/tc-reconfig

If you have conflicts it will most likely be due to the dimensional scaling. If you need assistance resolving a conflict reach out to @Hallberg-NOAA .

As usual we'd like reviews from the leads on the major forks: @jiandewang, @awallcraft, @kshedstrom, @gustavo-marques . The latter two have the ability to use the review button - we're trying to get you others that permission too.

marshallward and others added 30 commits September 19, 2019 22:14
This patch consists of three changes to the test suite:

- Tests are now run in dedicated work directories.  This allows for
  parallel execution of tests inside of make.

  Unexpected improvements are a 3-4x speedup on test time, and the
  disappearance of several anomalous "negative zero" issues which were
  otherwise unreproducible outside of the test, suggesting that
  replacing the serial runs inside of the tc directories has resolved
  some otherwise unknown problems.

- Makefile rules for running tests are now defined relative to the
  .testing directory, rather than an explicit base directory based on
  the path of the Makefile.

  Although this method is slightly less robust and slightly more prone
  to error, it makes it easier to build and run individual files within
  the project.

  Build paths are still absolute, and will be updated in a separate
  commit.

- Test output is now stored in a `results` directory, rather than inside
  the tc* configuration directories.
Test run work and results dir; relative path rules
In the discontinuous mode of neutral diffusion, the edge values
need to be evaluated in the same way that the 'polynomial' evaluation
returns which might be different at roundoff. This was accidentally
outside of the `if CS%discontinuous` block and was moved inside
This patch addresses the inconsistency of signed zero in the minimum and
maximum values used in checksum report.

The behavior of the Fortran intrinsic min() and our MPI library's
implementation of MPI_Reduce with MPI_MIN can give different results for
different values of signed zero, e.g. min(0,-0) vs min(-0,0).
Additionally, the MPI_Reduce result appears to not consistenty follow
these rules in more complex MPI calculations.

Due to these issues, we add the result to positive zero to ensure that
any negative zero results are reported as positive.
  Added a new optional scale argument to calculate_density, calculate_spec_vel
calculate_density_derivs, calculate_density_second_derivs, and
calculate_specific_vol_derivs, to rescale the densities or related variables.
All answers are bitwise identical, but there are new optional arguments to
public interfaces.
  Rescale bulkmixedlayer densities and their derivatives via the calls to
calculate_density and calculate_density_derivs.  All answers are bitwise
identical.
  Rescaled density units in MOM_entrain_diffusive for dimensional consistency
testing.  All answers are bitwise identical.
  Changed the units of GV%Rlay from [kg m-3] to [R] for dimensional consistency
testing.  This required the addition of unit_scale_type arguments to several
interfaces.  All answers are bitwise identical, but new arguments have been
added to several public interfaces.
  Moved rescaling of Rlay to [R] into the various set_coord routines.  This
required the addition of unit_scale_type arguments to two interfaces.  All
answers are bitwise identical, but new arguments have been added to two
public interfaces.
  Changed the units of GV%Rho0 from [kg m-3] to [R] for dimensional consistency
testing.  This required the addition of unit_scale_type arguments to several
interfaces.  All answers are bitwise identical, but new arguments have been
added to several public interfaces and the units of an element in a public
type have changed.
  Rescaled density units in MOM_regularize_layers for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled density units in MOM_set_viscosity for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled density units in MOM_set_diffusivity for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled density units in MOM_kappa_shear for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled density units in MOM_internal_tide_input for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled density units in diagnoseMLDbyDensityDifference in MOM_diabatic_aux
for dimensional consistency testing.  All answers are bitwise identical.
  Rescaled density units in MOM_tidal_mixing for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled density units in MOM_geothermal for dimensional consistency
testing.  This required adding a unit_scale_type argument to geothermal_init.
All answers are bitwise identical, but a public interface has a new argument.
  Corrected dimensions in comments.  All answers are bitwise identical.
  Partially rescaled the units of itide%TKE_itidal_input for dimensional
consistency testing.  All answers are bitwise identical, but the units of an
element of a transparent public type have changed.
  Rescaled density units in MOM_energetic_PBL for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled the density units of the cTKE or TKE_forced variables passed to
energetic_PBL and applyBoundaryFluxesInOut for dimensional consistency testing.
All answers are bitwise identical, but the units of an argument to two public
interfaces have changed.
  Rescaled the specific volume (density) units of the dSV_dT and dSV_dS
variables passed to energetic_PBL, applyBoundaryFluxesInOut, and
absorbRemainingSW for dimensional consistency testing.  Also rescaled the
dimensions of TKE returned from absorbRemainingSW.  All answers are bitwise
identical, but the units of a arguments to 3 public interfaces have changed.
  Used GV%H_to_RZ to simplify rescalings in applyBoundaryFluxesInOut and
absorbRemainingSW.  All answer are bitwise identical.
Hallberg-NOAA and others added 17 commits December 4, 2019 15:08
  Added conversion factors to 4 mass-flux diagnostics and comments to 4 others
on why no conversion factors are needed.  All answers are bitwise identical.
  Added scale arguments to 5 chksum calls and grouped another two chksum calls
while also adding the right scaling argument. All answers are bitwise identical.
  Undoes the dimensional scaling of the cell areas before taking their global
sum, so that the reproducing sum does not overflow when there is dimensional
rescaling.  All answers are bitwise identical when there is no rescaling, but
this eliminates a source of inadvertent overflows or underflows in the global
sums, and there is a new optional argument to compute_global_grid_integrals.
  Corrects the dimensionally inconsistent expressions for the CFL number in
the tracer advection code, in which a negligible thickness had been added to
the cell volume to avoid division by zero.  This change does not alter the
solutions in the MOM6-examples test cases, but now it permits dimensional
rescaling of lengths over a much larger range, and it could change answers if
the minimum layer thicknesses are small enough.
  Unscale interface heights before taking a global average via a reproducing sum
in non-Boussinesq mode global diagnostics to permit dimensional consistency
testing over a larger range.  All answers are bitwise identical.
  Added an optional tmp_scale argument to global_i_mean and global_j_mean to
specify an internal rescaling of variables being averaged before the reproducing
sum.  All answers are bitwise identical, but there are new optional arguments
to two public interfaces.
  Use tmp_scale when taking the i-mean interface heights for i-mean sponges, to
give a greatly expanded range of dimensional consistency testing.  All answers
are bitwise identical.
MOM6: +(*)Dimensional consistency completion
  Refactored how time-averaging of fluxes in forcing types that span multiple
timesteps and flux diagnostics are handled, and rescaled the units of
fluxes%dt_buoy_accum from [s] to [T].  This involved changing the arguments to
fluxes_accumulate, forcing_accumulate, mech_forcing_diags and
forcing_diagnostics, but because of the differing types of the arguments, an
incompatible mix of code will not compile.  Also changed the units of dt as
passed to accumulate_net_input, and made a minor change to extractFluxes1d to
avoid the possibilty of a division by zero.  All answers are bitwise identical,
but there are public interface changes, including changes that impact the mct
and nuopc driver codes.
  Added the new runtime parameters KAPPA_SHEAR_ITER_BUG and KD_TRUNC_KAPPA_SHEAR
to permit correction of a dimensionally inconsistent expression in the Newton's
method solver code of kappa_shear, and to allow the value of shear mixing that
is neglected compared with the background mixing to be set at run-time instead
of being hard-coded.  By default, all answers are bitwise identical, but there
are two new runtime parameters and the MOM_parameter_doc files change.
  Added the new runtime parameter VERT_FRICTION_2018_ANSWERS that avoids the use
of the hard-coded maximum viscous mixing length per timestep in the vertical
viscosity code, and added h_neglect in the denominators of several terms in the
viscosity code.  All answers in the MOM6-examples test cases are bitwise
identical, but the answers will change if ANGSTROM is set to 0, and there is a
new entry in the MOM_parameter_doc files.
@adcroft adcroft self-assigned this Dec 17, 2019
@kshedstrom
Copy link
Collaborator

I approve this PR, though I do feel left out. It changes answers (ever so slightly) for the dumbbells and the seamounts.

@adcroft
Copy link
Collaborator Author

adcroft commented Dec 17, 2019

Yes, I felt bad about not handling #1045 before making the PR but when I prepared it last night #1045 had a red X (because Travis-CI failed to launch, not because of the code).

@jiandewang
Copy link
Collaborator

works fine on NCEP WCOSS machine, testing on HEAR now (may need to wait for 1-2 days as HEAR is unstable today)

@awallcraft
Copy link
Collaborator

awallcraft commented Dec 20, 2019 via email

@jiandewang
Copy link
Collaborator

works fine on NCEP HERA machine, answer changes, approve this PR.

@gustavo-marques
Copy link
Collaborator

Mct tests are failing due to an error in mct_driver/mom_ocean_model_mct.F90, see
NCAR#136 (review)

The equivalent call in coupled_driver does not have this extra argument. We will wait for @Hallberg-NOAA's confirmation before deleting it.

  Corrected arguments to iceberg_forces and iceberg_fluxes in the mct version of
update_ocean_model.  This should correct the recently introduced problems with
compiling MOM6 with the mct coupler.
@gustavo-marques
Copy link
Collaborator

We approve this PR.

@adcroft adcroft merged commit 9269820 into dev/master Jan 8, 2020
@adcroft adcroft deleted the dev-master-candidate-2019-12-17 branch May 15, 2020 20:53
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.

10 participants