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

Feature/update to gfdl 20200515 2bug fix #29

Merged

Conversation

jiandewang
Copy link
Collaborator

require to update MOM6 to GFDL latest dev/master (#28)
It turned out that there are two bugs in the original code, see detail discussion at
https://docs.google.com/document/d/1soIwV56zFK8-ISvXFN_fNTdFn2VzynJZ_XoIdvI97j0/edit

Andrew Shao and others added 30 commits March 6, 2020 09:56
Moving FMS and mkmf to the `build` directory led to conflicts in the
`build/%/Makefile` and `build/%/path_names` rules, which were causing
redundant builds.

This patch moves these repositories back into a local `deps` directory,
now kept inside of `.testing`.
The Travis environment was updated in order to get a newer GCC compiler
version.  This was done to enable more aggressive initialization.
The FMS version has been updated in order to permit more aggressive
initialization settings in MOM6.  This requires a few bugfixes in the
newer FMS release.
Flags for initializing reals on stack as signaling NaNs (SNaN) and
integers as 2**31 - 1 have been added in this build.
This resolves several new issues raised by the current version of
Doxygen (1.8.17).  Primarily it addresses the stricter enforcement of
grouping syntax, e.g.

!>@{
...
!>@}

The current code is somewhat inconsistent in its use of !!@{ vs !>@{ ,
and moreso in the closing token.  This patch updates these to always
lead with starting Doxygen comment tokens !> .

Exposing these groups also revealed a few undocumented variables, which
have been updated with minimal descriptions for now.

Finally, one subroutine in MOM_horizontal_regridding was causing a
segfault, so it had to be reworked to remove its grouping, so that
individual indices now have descriptions.
- The new FMS has test_ programs that are found by list_paths which
  do not build properly unless using the FMS Makefile.am method.
- The "no libs" test was added to detect namespace collisions that
  are hidden when building with libraries. For now we'll retain
  this test but to do so requires a one-line hack to edit the
  pathnames file.
The target grid for the diagnostic grid update is now based on a
assigning a differrent pointer based on the boolean input argument
'intensive'. This is done so that this update is done in a more
'object-oriented' way
  Corrected the units reported for 9 diagnostics, and altered the code so that
the diagnostics N2_u and N2_v are only offered if the can be calculated and the
proper diagnostics are written if these diagnostics are requested (previously
arrays of zeros were always output).  All solutions are bitwise identical, but
some diagostics in output files change and the available_diags files have
altered entries.
  Rewrote the subroutine doc_param_time to work like the other doc_param
routines, including making the units argument optional, removing the argument
layout_param, and adding the new internally visible routine time_string.
Because time variables are currently logged as real values using the timeunit
argument to log_param_time, these changes do not have a widespread impact.  All
answers are bitwise identical, but there are some limited interface changes.
- Rename `h_dest` to `h_target` in routine and in signature
- Remove extraneous logic
  Added or corrected comments describing the units of many of the variables in
the ALE code.  All solutions are bitwise identical, although there are some
unit changes in arguments to unused subroutines.
Originally, the diagnostic arrays were being allocated on the first
call generating the diagnostic grid. This seems overly clunky because
the size of the grids are known before that. This was also causing
a segfault in updates to the new routine. The allocate statements
for these arrays are now done right after the number of levels is
known
  Corrected documented units in comments, corrected spelling errors in comments
and removed unused variables.  All answers are bitwise identical.
  Rescaled the calculations of diagnostics of the integrated mass transports, column integrated
temperature and salinity, cell thicknesses and column mass for dimensional consistency
testing.  All answers are bitwise identical.
  Rescaled the density derivatives used in full_convection, smoothed_dRdT_dRdS
and user_change_diff.  This change requires that new unit_scale_type arguments
be passed to these three subroutines.  All answers are bitwise identical.
  Converted find_interfaces from a function to a subroutine and revised
determine_temperature to rescale arguments and internal variables.  Also made
the declaration of array sizes consistent with other MOM6 code.  All answers
are bitwise identical, but there are changes to public interfaces.
  Removed an unneeded halo update in iceberg_forces.  All answers are bitwise
identical.
+Corrected diagnostics and diagnostic units
Hallberg-NOAA and others added 22 commits May 11, 2020 15:51
  Added grid type arguments to calls to update_ALE_sponge_field so that the
internal array pointers set by this routine will use the same indexing
conventions as the rest of the MOM6 code.  Also added comments describing some
arguments and other variables and got rid of some unneeded line continuations in
MOM.F90.  All answers are bitwise identical, but there are two new arguments to
update_ALE_sponge_field.
  Added array-syntax notation for a full-array copy in ISOMIP_Tracer.F90. All
answers are bitwise identical.
  Initialized dMLD_min and dMLD_max in ePBL_column, and corrected a comment in
response to helpful reviews from Brandon Reichl and Andrew Shao.  Because these
two arrays are not used until after the 3rd iteration, this may not matter to
the solution, although it should help make the code clearer and avoids unused
variables.  All answers in the MOM6-examples test cases are bitwise identical.
…anup

MOM6: +Corrected the use of array syntax calculations
  Corrected the negative CFL branch of PPM_angular_advect in MOM_internal_tides.
Simultaneously there was some revision to match other equivalent PPM advection
schemes in the MOM6 code and to replace some divisions by a multiplication by a
reciprocal.  The previous version was sufficiently wrong that it could not ever
have been used in any scientifically meaningful solutions, including anything in
MOM6-examples.  Accordingly, the PPM_angular_advect code was changed without a
flag to retain the previous answers.  All answers in the MOM6-examples test
cases are bitwise identical, and output files are unchanged.
The Makefile has been modified to reduce the amount of output during
testing.  Output is generally omitted on a successful test.  When a test
fails, we only display a small portion of the total output.

We also now run all tests, even if they fail, in order to give a
complete profile of the test failures.

Regression testing rules have been integrated into the general rules.

Finally, Travis config has been modified to further reduce output, and
to run all of tests (make -k).
Test logging is now much shorter, so folding is less important.
CodeCov reporting log is now saved to results/ rather than piped to
stdout, further reducing test logging output.
We were saving codecov output to results, but this was breaking the
`test.summary` test, which only returns true if `results` is empty.

This change should resolve this issue.
…cean#1113)

- gcc/8.3.0 issued `Error: Integer too big for its kind` reported in
  feedback on PR mom-ocean#1111. The intent was to assume kind=4 in these
  calculations but apparently our compilers were promoting
  `mod(dy + 32*(mo + 13*yr), 2147483648)` to kind=8. There were two
  mistakes in the expression:
  - the use of `2147483648` in the `mod` is not representable with kind=4;
  - the `mod` produces negative values and should have been a `modulo`.
- This commit reduces the range of the results by one number on the
  positive side and removes all the negatives.
…er to dev-master-candidate-2020-05-15

solve conflict in:
(1) config_src/coupled_driver/MOM_surface_forcing_gfdl.F90
(2) src/core/MOM_forcing_type.F90
(1) add missing halo in MOM_full_convection.F90
(2) remove wrong logic "not" in MOM.F90 at line 2669
@jiandewang
Copy link
Collaborator Author

MOM6 update RP.pdf
with the 2 bug fixings, 12 RT runs passed, 18 failed. The failure are purely due to the changing of checksum character in MOM6 restart file header. This will require re-creation of baseline for ufs-s2s-model. See full discussion in the attached file.

Copy link
Collaborator

@DeniseWorthen DeniseWorthen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am satisfied that the answers do not change in a meaningful way and that this PR reproduces the actual data in the current baselines.

@jiandewang jiandewang merged commit 728c429 into NOAA-EMC:dev/emc Jul 14, 2020
@jiandewang jiandewang deleted the feature/update-to-GFDL-20200515-2bug-fix branch April 26, 2021 21:21
jiandewang pushed a commit to jiandewang/MOM6 that referenced this pull request Feb 1, 2022
+(*)Make unit_scale_type arguments non-optional
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.