Dev gfdl main candidate 2021 07 08 #1440
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 'dev/gfdl' of github.com:NOAA-GFDL/MOM6: Fix to out-of-bounds error Slight improvement in writeMessageAndDesc Corrected recent bug in writeMessageAndDesc Included H_subroundoff in a denominator +Added code to handle tab lengths in documentation Added explicit form of cpu_clock_id (*)Improve ePBL when EPBL_ORIGINAL_PE_CALC=False Alternate fix for divide-by-zero. Clean up some divide-by-zero issues in OBCs. Added a space to a description. Codecov: Set base as parent (target) branch Fixes an integer-kind mismatch in MOM_random, seed_from_time() (#1113)
Merge in dev/gfdl updates
Sync with MOM6 dev/gfdl
Sync with NOAA-GFDL dev/gfdl
sync with dev/gfdl
Sync with NOAA-GFDL dev/gfdl
Sync with NOAA-GFDL dev/gfdl
Sync with NOAA-GFDL
Sync with NOAA-GFDL dev/gfdl
…ons of the routines with the fms-io or fms2-io interfaces added module use statments for fms2_io and MOM_io helper routines to MOM_restart added use_fms2=.true. arguments to save_restart and restore_state calls added write_ic=.true. to the save_restart call in MOM.F90 added module MOM_axis with routines to define and register axes and their metadata added module MOM_read_data_fms2.F90 with wrappers for fms2_io read_data interfaces and required routines added module MOM_write_field_fms2.F90 with wrappers for fms2_io write_data interfaces updated module use statments in MOM_io and MOM_restart to reference routines in MOM_read_data_fms2, MOM_write_field_fms2, and MOM_axis made write_field and create_file interfaces in MOM_io added create_file routines to MOM_io that accept file names or file objects to create/overwrite netcdf files that will be written to via write_field calls fixed compile-time errors added new MOM_read_data routines to MOM_read_data interface in MOM_io added placeholder call for new write_field routines to MOM_io changed use_fms2 to a required first argument in save_restart_fms2 and restore_state_fms2 changed write_ic to a required argument in write_initial_conditions fixed the layer and interface checks in MOM_get_diagnostic_axis_data commented out manual checksum registration in save_restart_fms2 so that internal fms2-io checksum computation is used added checks for time units to restore_state and save_restart added logic to make the restart time 1 to save_restart_fms2 if there is an abnormally large value passed to the routine added interface routine file_exists_FMS2 that uses the fms2_io file_exists call added subroutine get_num_restart_files to MOM_restart that searches for known variants of the input file names and returns then number of restart files available for querying, and the optional list of filepaths added loop to search the files for all mandatory variables in the list of file paths returned to by cal to get_num_restart_files added calls to get the variable dimension names and pass them as arguments to register_restart_field in restore_state_fms2 removed exit from inner variable loop in restore_state_fms2 code cleanup moved missing_fields=0 outside of the CS loop in restore_state_fms2 moved missing_fields=0 outside of the CS loop in restore_state_fms2 converted save_restart and restore_state to interface that call versions of the routines with the fms-io or fms2-io interfaces added module use statments for fms2_io and MOM_io helper routines to MOM_restart added use_fms2=.true. arguments to save_restart and restore_state calls added write_ic=.true. to the save_restart call in MOM.F90 added module MOM_axis with routines to define and register axes and their metadata added module MOM_read_data_fms2.F90 with wrappers for fms2_io read_data interfaces and required routines added module MOM_write_field_fms2.F90 with wrappers for fms2_io write_data interfaces updated module use statments in MOM_io and MOM_restart to reference routines in MOM_read_data_fms2, MOM_write_field_fms2, and MOM_axis made write_field and create_file interfaces in MOM_io added create_file routines to MOM_io that accept file names or file objects to create/overwrite netcdf files that will be written to via write_field calls fixed compile-time errors added new MOM_read_data routines to MOM_read_data interface in MOM_io added placeholder call for new write_field routines to MOM_io changed use_fms2 to a required first argument in save_restart_fms2 and restore_state_fms2 changed write_ic to a required argument in write_initial_conditions fixed the layer and interface checks in MOM_get_diagnostic_axis_data commented out manual checksum registration in save_restart_fms2 so that internal fms2-io checksum computation is used added checks for time units to restore_state and save_restart added logic to make the restart time 1 to save_restart_fms2 if there is an abnormally large value passed to the routine added interface routine file_exists_FMS2 that uses the fms2_io file_exists call added subroutine get_num_restart_files to MOM_restart that searches for known variants of the input file names and returns then number of restart files available for querying, and the optional list of filepaths added loop to search the files for all mandatory variables in the list of file paths returned to by cal to get_num_restart_files added calls to get the variable dimension names and pass them as arguments to register_restart_field in restore_state_fms2 removed exit from inner variable loop in restore_state_fms2 code cleanup removed test workflow directory moved missing_fields=0 outside of the CS loop in restore_state_fms2 converted save_restart and restore_state to interface that call versions of the routines with the fms-io or fms2-io interfaces added module use statments for fms2_io and MOM_io helper routines to MOM_restart added use_fms2=.true. arguments to save_restart and restore_state calls added write_ic=.true. to the save_restart call in MOM.F90 added module MOM_axis with routines to define and register axes and their metadata added module MOM_read_data_fms2.F90 with wrappers for fms2_io read_data interfaces and required routines added module MOM_write_field_fms2.F90 with wrappers for fms2_io write_data interfaces updated module use statments in MOM_io and MOM_restart to reference routines in MOM_read_data_fms2, MOM_write_field_fms2, and MOM_axis made write_field and create_file interfaces in MOM_io added create_file routines to MOM_io that accept file names or file objects to create/overwrite netcdf files that will be written to via write_field calls fixed compile-time errors added new MOM_read_data routines to MOM_read_data interface in MOM_io added placeholder call for new write_field routines to MOM_io changed use_fms2 to a required first argument in save_restart_fms2 and restore_state_fms2 changed write_ic to a required argument in write_initial_conditions fixed the layer and interface checks in MOM_get_diagnostic_axis_data commented out manual checksum registration in save_restart_fms2 so that internal fms2-io checksum computation is used added checks for time units to restore_state and save_restart added logic to make the restart time 1 to save_restart_fms2 if there is an abnormally large value passed to the routine added interface routine file_exists_FMS2 that uses the fms2_io file_exists call added subroutine get_num_restart_files to MOM_restart that searches for known variants of the input file names and returns then number of restart files available for querying, and the optional list of filepaths added loop to search the files for all mandatory variables in the list of file paths returned to by cal to get_num_restart_files added calls to get the variable dimension names and pass them as arguments to register_restart_field in restore_state_fms2 removed exit from inner variable loop in restore_state_fms2 code cleanup removed test workflow directory removed white space updated FMS tag in .testing Makefile fixed argument comments to use doxygen style in MOM_write_field_fms2 code cleanup removed \TODO from MOM_restart fixed documentation for module variables in MOM_read_data_fms2 and MOM_write_field_fms2 removed doxygenized TODO statements removed new MOM_read_data routines from interface, and commented out calls in MOM_io fixed doxygen definitions more doxygen fixes changed save_restart and restore_state to wrapper routines with opitional use_fms2 and write_ic arguments added use_fms2=.true. and write_ic=.true. to save_restart and restore_state calls removed the error messages from append_substring tried reordering the write_ic and use_fms2 checks in save_restart to fix error with invalid memory reference in the MOM.F90 call to save_restart-write_initial_conditions
…ing the use_fms2 and write_ic flags if present to avoid invalid memory reference error added str_len argument to register_variable_attribute calls added support to for rotated fields to write_initial_conditions and save_restart_fms2 removed whitespace
removed use_fms2 optional arguments in save_restart, restore_state, and create_file made use_fms2 a MOM_restart module variable Update MOM_state_initialization.F90 Remove space. Update MOM_ice_shelf.F90 Add space. Update MOM_driver.F90 Add space Update mom_surface_forcing_mct.F90 remove whitespace Update ocean_model_MOM.F90 Add space Update mom_ocean_model_nuopc.F90 Add space Update MOM_surface_forcing.F90 Add space Changes needed work ESM4 to run with new io
Corrected three module use statements in infra/FMS1 to eliminate any dependencies of code in config_src/infra/FMS1 on code in src/framework. These use statements eventually point to the same place as before, but with less indirection. This change should facilitate later steps to compile everything in and under the config_src/infra directories as libraries. All answers are bitwise identical.
Duplicated all of the modules in infra/FMS1 into new copies in infra/FMS2, in preparation for merging in the FMS2 i/o changes to the new infra/FMS2 directory. All answers are bitwise identical, regardless of which config_src/infra directory is used.
Added code to explicitly set a (1x1) io_domain when no other io_layout is specified, complying with changing requirements for 2020 and later versions of FMS, and following the default behavior of previous versions. All answers are bitwise identical.
…om/wrongkindofdoctor/MOM6 into FMS2_io Merged in PR #1165, "Add fms2-io to MOM restart interfaces", but with modifications to move the new files to config_src/infra/FMS2, where they belong, and to avoid inappropriate changes to code in the src/framework directory. All existing tests are passing with config_src/infra/FMS1, but the new changes require that the version of FMS used with config_src/infra/FMS2 to at least 2020.03, and this disables all of our AM2-based coupled test cases (due to the deliberate decision from the GFDL Modeling Systems group to drop support for this older atmospheric model). Although the new code reproduces the previous answers in ocean_only and ice_ocean_SIS2 test cases with the intel and gnu compilers, with the pgi (19.10.0) compiler there are fatal errors in all ice_ocean_SIS2 cases and in ocean_only cases driven by data override. The last version of FMS where the pgi compiler works in all cases is 2019.01. The problem in some ocean_only cases appears to be related to the use of FMS2 i/o calls in the data_override routines. With the ice_ocean_SIS2 cases, the error message in with the pgi compiler is "mpp_broadcast_text: broadcasting from invalid PE.". (Note that with the newer versions of FMS (2020.03 and later), OM_1deg requires that the namelist file for data_override_nml has the added line {reproduce_null_char_bug = .true.} to reproduce the previous answers, but it is a fatal error to add this setting with FMS versions before 2020.03.)
Eliminated scale_data from MOM_read_data_fms2, as it was duplicative of rescale_comp_data from MOM_domain_infra.F90. Also eliminated the optional scale argument from the fms2 write_field routines, because this scaling is already handled in the framework/MOM_io.F90 routines, and because the way the scaling was implemented with a pointer to defeat the intent(in) of the data array to the write routines was a deviously terrible idea that would cause endless problems. Because the mpp_domain in the MOM_domain_types have been modified to always have an io_domain set, the test and calls setting an io_domain in all of the fms2 read_data and fms2 write_data routines. In addition, there is no longer a need for a routine to read specifically from the supergrid, so the fms2 MOM_read_data_2d_supergrid routine was eliminated. The code compiles with these changes, but none of this fms2 I/O code is exercised yet in MOM6, so of course all answers are bitwise identical.
Replaced the two optional arguments xPosition and yPosition to the two routines wrapped by the interface MOM_register_variable_axes in MOM_axis.F90 with a single optional argument, position. Also dramatically refactored the code in read_data_fms2.F90 to eliminate unused variants and adding more granular subroutines to dramatically reduce duplicative code, eliminating several (unexercised) bugs in the process; that file went from 1441 lines long to 924. The code compiles with these changes, but none of this fms2 I/O code is exercised yet in MOM6, so of course all answers are bitwise identical.
Cleaned up MOM_restart.F90 in the wake of the recent merge of FMS2 I/O related changes into MOM6. The changes include calling the recently added functions get_variable_byte_size and get_num_restart_files, and revising the names of several variables in open_restart_units and the order in which the count of files is incremented for greater clarity. There are also a number of duplicative comments that were removed while others were reformatted. All answers and output are bitwise identical.
Added the option to use FMS2 interfaces to read variables in the FMS2 version of MOM_io_infra.F90. For now, this is controlled by a hard-coded module variable, FMS2_reads, in MOM_io.F90. Also extensively revised MOM_read_fms2.F90 to add the new public subroutine prepare_to_read_var that is used from MOM_io_infra.F90, to systematically determine the directions that axes in files using the cartesian_axis attribute before resorting to using the axis names or units. All of the subroutines from MOM_read_fms2.F90 whose functionality has been merged into MOM_io_infra.F90 have now been removed. All answers are bitwise identical and reproduce the FMS1 test cases, but there is a new publicly visible routine.
Modified the routines that write out the checksums of various types related to the surface ocean state so that only the root PE writes the checksums. The modified routines are ocn_ice_bnd_type_chksum and ocean_public_type_chksum in config_src/infra/FMS_cap. The analogous routines with the nuopc_cap and mct_cap have not been modified; although doing so would be a very good idea, it should be done by someone who is actively testing them. In some large-PE tests cases this reduces the volume of output to stdout by over 90% without any loss of information. All solutions are bitwise identical, but there may be minor white-space reformatting of the output in some cases.
Use axistype for MOM_interp_infra directly from mpp_io_mod and add a copy of get_axis_data to both copies of infra/FMS[12]/MOM_interp_infra.F90, and then use these in framework/MOM_horizontal_regridding.F90, to permit the MOM6 I/O calls to use the FMS2 interfaces without simultaneously requiring changes to the horizontal interpolation code. All answers are bitwise identical, but there are changes to the interfaces offered by a public module.
Added a new optional logical argument to write_metadata_axis to indicate when an axis is staggered at the edges of the tracer grid, and changed calls to get_file_info to stop requesting the number of global attributes. Also eliminated some unused optional arguments to the FMS1 version of write_metadata field. All answers are bitwise identical, but there are minor changes to some I/O related interfaces.
Corrected the logic of a warning message in categorize_axis by adding parentheses. All answers are bitwise identical, and spurious warnings are no longer being issued.
Added a large number of calls to handle all of the writes via the FMS2 interfaces to infra/FMS2/MOM_io_infra.F90. There are newly defined private types in MOM_io_infra to wrap the axistype and fieldtype that had previously been offered from mpp_io_mod. All answers are bitwise identical and it has been verified that output files do not change and the restarts are still working.
Deleted the unused MOM_axis.F90 and MOM_write_field_fms2.F90 modules, now that MOM_io_infra.F90 has been updated to be able to use FMS2 interfaces for both reading and writing to files. As these modules had never been used, they do not change any answers.
|
I approve this PR. It reproduces my former restart with ice answers. |
| @@ -44,6 +47,10 @@ function register_tidal_bay_OBC(param_file, CS, OBC_Reg) | |||
| endif | |||
| allocate(CS) | |||
|
|
|||
| call get_param(param_file, mdl, "TIDAL_BAY_FLOW", CS%tide_flow, & | |||
| "Maximum total tidal volume flux.", & | |||
| units="m3 s-1", default=3.0d6, scale=US%m_s_to_L_T*US%m_to_L*US%m_to_Z) | |||
There was a problem hiding this comment.
With the gnu/10.1.0 compiler, we get a compilation error due to dummy arg specification default=3.0d6. Changing that to default=3.0e6 fixes the issue for us.
52 | units="m3 s-1", default=3.0d6, scale=US%m_s_to_L_T*US%m_to_L*US%m_to_Z)
| 1
Error: There is no specific subroutine for the generic ‘get_param’ at (1)
There was a problem hiding this comment.
I wonder if this is a compiler bug, maybe related to our kind-less real declarations and -r8 flag usage. Do you see this in 10.2? I see no issues with 10.2 and 11.1. Just asking out of interest, no need to investigate further.
Either way, easy enough to fix.
There was a problem hiding this comment.
My bad. Adding the -fdefault-double-8 compiler flag fixes the issue. It's interesting that this dummy arg is the only instance that requires the -fdefault-double-8 flag, but that's fine.
There was a problem hiding this comment.
Personally I do not see any reason why we should not change the dummy argument specification to default=3.0e6, as suggested by @alperaltuntas, to correct the problem without requiring any new compiler flags and to match the pattern used elsewhere in the MOM6 code. This seems to be a case where correcting the problem twice might be in order.
abozec
reviewed
Jul 9, 2021
| @@ -1006,7 +1021,8 @@ subroutine apply_ALE_sponge(h, dt, G, GV, US, CS, Time) | |||
|
|
|||
| call pass_var(sp_val,G%Domain) | |||
| do j=CS%jsc,CS%jec; do I=CS%iscB,CS%iecB | |||
| sp_val_u(I,j,1:nz_data) = 0.5*(sp_val(i,j,1:nz_data)+sp_val(i+1,j,1:nz_data)) | |||
| sp_val_u(I,j,1:nz_data) = 0.5*(sp_val(i,j,1:nz_data)+sp_val(i+1,j,1:nz_data)) | |||
| mask_u(I,j,1:nz_data) = max(mask_z(i,j,1:nz_data),mask_z(i+1,j,1:nz_data)) | |||
There was a problem hiding this comment.
Taking the average of the sp_val will potentially include a masked valued (huge value if set that way in the input file) and it will not be masked by taking the max of the mask_z. So I would suggest, if we want to keep the max of mask idea to weigh the sp_val by mask_z:
sp_val_u=(sp_val(i)*mask_z(i) + sp_val(i+1)*mask_z(i+1)) / max(mask_z(i)+mask_z(I+1),1e-10)
There was a problem hiding this comment.
There was a problem hiding this comment.
I assume the mask_z update is following the 0-or-1 convention as other MOM masks, so that mask_u is water (=1) if either east/west c-point is water.
I want to believe that a similar masking convention would have been applied to sp_val in horiz_interp_and_extrap_tracer, but I could be wrong here.
If there is some large value in sp_val caused by an input mask value, then perhaps this should be addressed in horiz_interp_and_extrap_tracer?
There was a problem hiding this comment.
I see from Matt's comment and #1434 that max was flipped to min, extending the mask. I also do not see any special handling of input mask values in horiz_interp_and_extrap_tracer.
Maybe the extended mask is clobbering out any potentially large values causing by a different input mask value?
There was a problem hiding this comment.
That piece of code in MOM_horizontal_regridding does not filter all the land values for u and v, I'm not sure why... so a mask value in sponge seems to be the solution.
I've tried the max and Matt's min method and both work in my example. It now depends on if users prefer an extended mask (using min) or using the 1 value available (using max) to calculate the u and v sponge values.
There was a problem hiding this comment.
@abozec Is this something you need to be fixed before we merge? Or can it be addressed in the next update to main?
There was a problem hiding this comment.
it can be addressed with #1434 of Matt's so I'm good with that.
There was a problem hiding this comment.
Ok great, we will merge #1434 in the next dev/gfdl -> main merge, which is expected to happen in a couple of weeks.
Added a new routine, query_wave_properties, that can be call to get information about the wave properties from the waves control structure, and added a get_param call for SURFBAND_WAVENUMBERS to MOM_wave_interface_init when it is using the options that are typical with NUOPC coupler. These changes should allow the NUOPC coupler to compile and work again, while still keeping the same level of opacity in the wave_parameters_CS. All answers should be bitwise identical, although the order of some entries in the MOM_parameter_doc files may change when coupled with waves is WAVE_METHOD=SURFACE_BANDS and SURFBAND_SOURCE=COUPLER.
|
@jiandewang, after some discussions with @breichl, I think that we may have a PR that fixes the issues with compiling the waves code with the NUOPC cap. Please test out https://github.com/NOAA-GFDL/MOM6/pull/1443. |
sanAkel
approved these changes
Jul 11, 2021
jedwards4b
referenced
this pull request
in ESMCI/cime
Jul 12, 2021
Fix auto r8 flag for CESM GNU builds Fixes the auto -r8 GNU compiler flag specification for MOM6+CESM by adding -fdefault-double-8 flag in addition to -fdefault-real-8. The issue was encountered while evaluating a MOM6 PR: NOAA-GFDL/MOM6#1440 (comment) Test suite: aux_mom Test baseline: 210706 Test namelist changes: n/a Test status: b4b Fixes NOAA-GFDL/MOM6#1440 (comment) User interface changes?: n/a Update gh-pages html (Y/N)?: N
Moved the recently added call to read SURFBAND_WAVENUMBERS into an array when SURFBAND_SOURCE=COUPLER down by a line to follow the allocate call for that array. This change should avert a memory access problem that would otherwise arise when exercising this newly added code. All answers are bitwise identical in any case that ran with the previous version, and they should reproduce answers from before this PR as a whole, although obviously this code is not as well tested as would be ideal, based on the fact that this bug was in the last commit, which also passed testing.
Added the new routine query_ocean_state to mom_ocean_model_nuopc to allow for the wave properties to be obtained by the mom_cap without having to rely on the elements of an otherwise opaque type being public. I believe that all elements of the ocean_state_type could now be declared as private, and that the version of MOM6 with the NUOPC coupler should now work as intended.
fix dim_names assignment in MOM_io:read_var_sizes
+Add query_wave_properties & fix NUOPC wave queries
|
The branch has been updated with two new PRs:
The latter was submitted to address the issues reported by EMC. |
|
@marshallward FYI: PR#1443 solved compiling issue but we are still having issue to run the model. |
initialize CS%WaveNum_Cen before read in this parameter in MOM_wave_interface.F90
|
This branch has been updated with the following PR: This should address the new issue reported by EMC. |
UFS works fine now |
|
The last round of updates have resolved the issues seen by EMC. Could I ask the other groups review the updated candidate? |
@marshallward I looked up the changes since my last check, these changes "do not apply" to GEOS, please ✅ me off! |
|
I still approve this PR. |
|
I approve also. |
|
I approve as well. |
|
Merge is complete, well done everyone! We will try to prepare another one soon with the OBC mask fixes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This release includes several new features and bugfixes to MOM6.
General improvements include updates to the sponges, surface forcing,
diagnostics, and the ice shelf.
Many issues related to FMS2 support have been resolved, particularly related to
handling of restart files.
Note that the
input_filenameparamenter of the SIS2 namelist group was notbeing respected in many cases. It is now being enforced. Coupled experiments
which were not managing this parameter should confirm that sea ice fields are
being appropriate initialized after restart.
This patch applies stricter memory management to the source code, in
preparation for more aggressive valgrind testing. This may cause unexpected
behavior in drivers and coupled models which reference MOM fields outside of
MOM functions.
Review
Features
Bugfixes
Framework
Testing
Refactoring
External
Contributors: