dev/master candidate 2020-05-15#1111
Conversation
This fixes a bug in the SOURCE variable which was used to track files during development. It is generally unused for test builds. This was reworked to include support for FMS and the target repository. `SOURCE` is now a Makefile function which support MOM6 and FMS trees. We also extended it to catch other extensions: .inc .c .h These variables are used to link the top Makefile to the mkmf-generated makefiles, in order to trigger a rebuild via mkmf-makefile changes. I would not expect these `SOURCE` dependents to persist if we move to the autoconf build system.
Source tree variable fix
Vertically extensive diagnostics like advective transports, diffusive fluxes, etc. were being remapped onto a diagnostic grid prior to the physical process that chagne the tracer and/or layer thicknesses. However this means that the effective operator used for each component of a tracer budget was not the same, and so budgets could never be closed cell-by-cell in a diagnostic coordinate. This commit fixes one part of the inconsistency by setting the target diagnostic grid for all vertically extensive quantities to be one constructed at the very beginning of the timestep. However, problems with closing the budget should still be expected 1. Conservation of column integrals cannot be expected because the target grid can be smaller than the source grid (layer thicknesses at the native grid at the current point in the algorithm). This leads to some fluxes being 'thrown away' due to the reintegrate algorithm. 2. To truly have a consistent operator for all terms of a budget, the source grid for all vertically extensive grids should also be the same
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.
Travis bionic test
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.
Fix "no lib" builds
Doxygen 1.8.17 documentation fixes
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
This reverts commit 9abcb10.
The model thicknesses
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.
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.
…into Hallberg-NOAA-units_cleanup
…g-NOAA/MOM6 into Hallberg-NOAA-improved_ePBL_iterations
…/MOM6 into Hallberg-NOAA-fix_angular_advect
Reduced output during testing
|
I approve this PR. |
| s1 = sc + 61*(mn + 61*hr) + 379 ! Range 379 .. 89620 | ||
| ! Fun fact: 2147483647 is the eighth Mersenne prime. | ||
| ! This is not the reason for using 2147483647+1 here. | ||
| s2 = mod(dy + 32*(mo + 13*yr), 2147483648) ! Range 0 .. 2147483647 |
There was a problem hiding this comment.
I am not able to build with gnu/8.3.0 because of the following error:
Error: Integer too big for its kind at (1).
Should it be 2147483647 (default value of HUGE for 32-bit integers, see post below) instead of 2147483648?
https://stackoverflow.com/questions/34972531/gfortran-error-integer-too-big-for-its-kind-at-1?rq=1
There was a problem hiding this comment.
I agree, this should have at least raised a warning somewhere.
I'm more confused that no one else (including me) is having the same problem. Many of us need to run at 32-bit integers to keep compatibility with our libraries (MPI, etc).
There was a problem hiding this comment.
Is the mod required? 2147483648/(32*13) > 5M (years)
There was a problem hiding this comment.
@gustavo-marques Does using mod(dy + 32*(mo + 13*yr), 2147483647) work with gnu/8.3.0? I think the mod is required for negative years.
There was a problem hiding this comment.
If you're dealing with negative years, don't you need to use modulo rather than mod?
There was a problem hiding this comment.
Just a FYI, kind=8 is not always 8bytes (e.g. NAG).
There was a problem hiding this comment.
- The number was chosen to fit into 4-bytes. I believe all our integers are 4-byte integers and not only due to a limitation in netCDF. I made the mistake of assuming integers were 8-bytes before and think I was avoiding that mistake here.
- The intent was create a positive integer so the
modshould indeed be amodulo - I'm trying to find somewhere I have access to gcc-8.3.0 to test a patch.
There was a problem hiding this comment.
@gustavo-marques Does using
mod(dy + 32*(mo + 13*yr), 2147483647)work with gnu/8.3.0? I think the mod is required for negative years.
Yes, it does.
There was a problem hiding this comment.
Excellent. I'm sending a patch.
There was a problem hiding this comment.
#1113 should fix this problem. We'll merge it into this PR if @gustavo-marques can confirm it works.
|
compiling fine on HERA and Orion, will test in coupled system, this will take some time. |
|
I approve this PR. |
- gcc/8.3.0 issued `Error: Integer too big for its kind` reported in feedback on PR #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.
|
This PR does not change answers (ocean.stats) for us. However, it does change the thicknesses (h) and other diagnostics that depend on that (e.g., umo and uhGM) in the outputs remapped to ocean_model_z and ocean_model_rho2. We are okay with these changes but I want to make everyone aware of that. |
@ashao Is this consistent with #1070 ? Is h changed because it is now the initial h within the stepping? |
|
Yes those diagnostics should have changed. All vertically extensive diagnostics are remapped using the thicknesses at the very beginning of baroclinic timestep and not the incrementally update thicknesses. |
|
approve this PR |
|
Now that this is merged, don't forget to merge dev/master into your own branches. |
* Diag buffer: Set fill_value_3d ks/ke size The `fill_value_3d` unit test of `diag_buffer_unit_tests_3d` was not setting its vertical extent, causing inaccurate allocation sizes and potential segmentation faults. (Or, in my case, complete memory consumption and OS meltdowns.) This patch adds `set_vertical_extent` and uses the same 10-element width. This appears to fix the issue for me. * Diag buffers: Streamline testing Some unused variables and incorrect comments have been removed from the unit testing. Assisted-by: copilot
11 participants
The is a PR from GFDL (dev/gfdl -> dev/master). GitHub calls it big but it is immense! I didn't even realize we had that many files.
What is in here:
As per usual, we need feedback from @awallcraft , @kshedstrom , @gustavo-marques and @jiandewang.
There is a corresponding branch (dev-master-candidate-2020-05-15) of NOAA-GFDL/MOM6-examples which has new doc strings. Regression answers are unchanged.