-
Notifications
You must be signed in to change notification settings - Fork 268
GFDL to main 2025-04-23 #1664
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
GFDL to main 2025-04-23 #1664
Conversation
Fix a bug with subroutine write_energy when using a DT<2. Otherwise, the energy outputs are written at wrong time steps. The reason was that time type divide is essentially a floor. So DT/2 = 0 if DT<2.
The subroutine compute_global_grid_integrals appeared in both the MOM_state_initialization and MOM_shared_initialization modules, but was only being called from the latter. This commit removes the extra copy in MOM_state_initialization. It also removes some unnecessary parentheses in the copy that is being retained, in part to facilitate the review of this commit. All answers are bitwise identical, and no publicly visible interfaces are altered.
In addition to REMAPPING_USE_OM4_SUBCELLS, for ALE remapping, there are several parameters of the form XXX_REMAPPING_USE_OM4_SUBCELLS, where XXX identifies the target, and they all currently default to True. To simplify setting them all to False, which is recommended, the defaults for the XXX versions is changed to the value of REMAPPING_USE_OM4_SUBCELLS. Answers are only changed if REMAPPING_USE_OM4_SUBCELLS is set to False and the default (now False) is used for one or more of the other parameters. In such cases the original behaviour can be recovered by explicitly setting the other parameters to True.
Removed two instances of `fail_if_missing=.true., default=0.` which are contradictory: a default value is meaningless if the parameter must be specified. I encountered this when adding the `defaults=` option to `get_param_real_array()`.
The `default=` optional argument to get_param() only provides a uniform value to initialize an array of reals. This commit adds the optional `defaults=` argument that must have the same length as the `values` argument. I've also added a few instances of this optional argument: - by adding the `initialize_thickness_param()` procedure, selected by `THICKNESS_CONFIG = "param"`. The procedure was based on the "uniform" method, and uses the parameter `THICKNESS_INIT_VALUES` which defaults to uniform values derived from `MAXIMUM_DEPTH` - the setting of MLD_EN_VALS in MOM_diabatic_driver.F90 which was previously using a work around to set defaults to 25, 2500, 250000 J/m2. - two vectors of 4 values in user/user_change_diffusivity.F90 There will be some doc file changes, but no answer changes.
Two latent heat constants are imported directly from FMS, which is built independently of MOM6. Previously, it was a safe assumption that both would be built with double precision, but this is no longer the case since FMS now supports both single and double precision. This could cause conflicts with mixed-precision builds. This patch converts the values from FMS-precision to MOM-precision. Single->double should not affect reproducibility since every single-precision number can be exactly represented in double precision. Double->single could affect reproducibility, but this is not an issue since MOM6 does not run in single precision.
* Inline harmonic analysis
Important bug fix:
1) The Cholesky decomposition was operating on entries below
the main diagonal of FtF, whereas in the accumulator of FtF, only
entries along and above the main diagonal were calculated. In this
revision, I modified HA_accum_FtF so that entries below the main
diagonal are accumulated instead.
2) In the accumulator of FtSSH, the first entry for the mean
(zero frequency) is moved out of the loop over different tidal
constituents, so that it is not accumulated multiple times within
a single time step.
* Inline harmonic analysis
Another bug fix: initial state added back to the mean state.
* Inline harmonic analysis
Minor update to HA_solver
* Tidal angular frequency has units [rad s-1]
Tidal frequencies are always angular frequencies to simplify applying
sine and cosine. These have MKS units [rad s-1] but they are all
currently listed as [s-1].
Updated dOxygen comments for variables, e.g. [T-1 ~> s-1] becomes
[rad T-1 ~> rad s-1]. Updated get_param units. e.g. units="s-1"
becomes units="rad s-1".
No answers are changed, but the logged parameter units are different.
There are frequencies in MOM_internal_tides.F90 but these have not
been updated because they may be specified incorrectly. They are
used as if they are [T-1] but they are calculated as 2PI/period [rad T-1].
real, allocatable, dimension(:) :: frequency !< The frequency of each band [T-1 ~> s-1].
real :: period ! A tidal period read from namelist [T ~> s]
! The periods of the tidal constituents for internal tides raytracing
call read_param(param_file, "TIDAL_PERIODS", periods)
do fr=1,num_freq
period = US%s_to_T*extract_real(periods, " ,", fr, 0.)
CS%frequency(fr) = 8.0*atan(1.0)/period
enddo
All MOM6-examples cases have INTERNAL_TIDES=False and so can't
resolve this issue.
* fixed too-long line
* add diagnostic for Kd_Work related to the Kd_add part * also corrects some capitalization typos * fix id init --------- Co-authored-by: Raphael Dussin <[email protected]> Co-authored-by: Alistair Adcroft <[email protected]>
Changed the code to issue a fatal error message when WIND_CONFIG = "SCM_ideal_hurr", with the message including instructions on how to recover mathematically equivalent solutions, and eliminated the subroutine SCM_idealized_hurricane_wind_forcing() from the Idealized_hurricane module. The ocean_only MOM_parameter_doc files have also been modified to reflect that "SCM_ideal_hurr" is no longer a valid setting for WIND_CONFIG. All answers are bitwise identical in any cases that run, but some cases may fail during initialization with instructions on how to fix them.
Corrected a bug that causes ePBL column to set the wrong variable and then use an uninitialized variable when EPBL_ORIGINAL_PE_CALC is set to false. This bug was present when the EPBL_ORIGINAL_PE_CALC was first added on Sept. 30, 2016, but it was not detected because only the default case with EPBL_ORIGINAL_PE_CALC = True appears to being used or tested. Any runs that used this code with debugging compile options would have trapped it immediately. This will change answers when EPBL_ORIGINAL_PE_CALC is false.
Added the new runtime parameter EPBL_MLD_ITER_BUG that can be set to false to correct buggy logic that gives the wrong bounds for the next iteration when USE_MLD_ITERATION is true and successive guesses increase by exactly EPBL_MLD_TOLERANCE. By default all answers are bitwise identical, but there is a new runtime parameter in some MOM_parameter_doc files.
Added the ability to passively run ePBL_column twice in a diagnostic mode and then provide diagnostics of the differences in the diffusivities and boundary layer depths that are generated with the two options. This is controlled by the new runtime parameter EPBL_OPTIONS_DIFF, which is an integer that specifies which options to change or 0 (the default) to disable this capability. Associated with this are the new diagnostics ePBL_opt_diff_Kd_ePBL, ePBL_opt_maxdiff_Kd_ePBL and ePBL_opt_diff_h_ML, which only are registered when the differencing is enabled. For now, only changes associated with the settings of EPBL_ORIGINAL_PE_CALC and EPBL_ANSWER_DATE can be evaluated, but this list will grow as new options are added. As a part of these changes, there were some other reforms to the way that MOM_energetic_PBL handles diagnostics, with the 2-d and 3-d arrays changed from allocatable arrays with enduring memory commitments in the energetic_PBL_CS type into simple arrays in energetic_PBL, relying on the fact that compilers will be smart enough to avoid actually allocating this memory when it is unused to avoid expanding the overall memory requirements of MOM6. A number of allocate and deallocate calls were eliminated by these changes. In addition, explicit 'units=' descriptors were added to numerous register_diag_field calls to help identify inconsistent conversion factors. Also, the diagnostic of the number of boundary layer depth iterations was added to the ePBL_column_diags, which enabled the intent of the CS and G arguments to ePBL_column to be changed to intent(in). The unnecessary extra logic associated with the use of the OBL_converged variable in ePBL_column was eliminated, but the results are uneffected. Because the MOM_parameter_doc files were changing anyway, and because this commit is only a diagnostic change, about 6 spelling errors were corrected in ePBL parameter descriptions as a part of this commit. All answers are bitwise identical, but there is a new runtime parameter and there may be new diagnostics depending on the choice of a non-default value for that new parameter.
Added the new internal subroutine find_Kd_from_PE_chg inside of the MOM_energetic_PBL module to directly calculate an increment in the diapycnal diffusivity from an energy input. This can be used when ePBL does not convert released mean kinetic energy into turbulent kinetic energy (i.e., if MKE_TO_TKE_EFFIC = 0.) and is more efficient than the more general iterative approach. To preserve old answers, this new option is only enabled for the surface boundary layer when the new runtime parameter DIRECT_EPBL_MIXING_CALC is set to true. This new option can be tested passively by setting EPBL_OPTIONS_DIFF to 3 in a run that uses ePBL. By default all answers are bitwise identical, but there is a new runtime parameter in some of the MOM_parameter_doc files.
Add the option to do energetically consistent bottom boundary layer mixing with the new routine ePBL_BBL_column. ePBL_BBL_column is closely based on the surface-focused ePBL mixing in ePBL_column, but without adding convective instability driven mixing or mean-TKE driven mixing to avoid possible double-counting. This new option is enabled by setting the new runtime parameter EPBL_BBL_EFFIC to be positive. If both EPBL_BBL_EFFIC and BBL_EFFIC are set to positive values, there is a risk of double-counting, but this case is not being trapped for now. The changes include the addition of a new mandatory vertvisc_type argument to the publicly visible routine energetic_PBL. When this new ePBL bottom boundary layer mixing option is enabled, there are several new diagnostics available that are related to bottom boundary layer mixing. Several new checksum calls were also added with this new option when DEBUG = True. The MOM_parameter_doc files are altered by the addition of two new runtime parameters, and by the correction of several spelling errors in the descriptions of other ePBL parameters. By default, all answers are bitwise identical.
Added the ability to modify the bottom boundary layer TKE budget to account for an exponential decay of TKE away from the boundary and the fact that when the diffusivity is increased at an interface, it causes an increased buoyancy flux that varies linearly throughout a well mixed bottom boundary layer and through the layer above. This new capability is enabled by the new runtime parameter DECAY_ADJUSTED_BBL_TKE and is implemented via the new internal function exp_decay_TKE_adjust. In addition, this commit adds 9 bottom-boundary layer specific run-time parameters that are analogous to parameters that set the properties of the surface-driven mixing, and take their defaults from them, but can now be set independently for the bottom boundary layer. The inefficient option to use bisection to estimate the bottom boundary layer depth was eliminated, as it certain that it is not being used yet in any cases, and it should be deprecated for the estimation of the surface boundary layer. By default, all answers are bitwise identical, but there are up to 10 new runtime parameters that will appear in some MOM_parameter_doc files.
Added the new optional unscale argument to reproducing_sum() and reproducing_sum_EFP(). When this is used, the resulting real sum is restored to the same scaling as the input arrays, but when there is an EFP_type returned it is the same as it would have been had the input array been rescaled before it was passed in. All answers are bitwise identical, but thre is a new optional argument to a two publicly visible interfaces.
Added the new element RZL2_to_kg to the unit_scale_type for convenience when rescaling masses and other globally integrated variables. All answers are bitwise identical, but there is a new element in a transparent type.
Revised write_energy() and accumulate_net_input() to work more extensively in dimensionally rescaled variables by using the new unscale arguments to the reproducing_sum functions. As a result of these changes, 15 rescaling factors were eliminated or moved toward the end of write_energy(). All answers are bitwise identical.
Use reproducing_sum with unscale to calculate the global mass diagnostic, globally integrated ocean surface area, offline tracer transport residuals and in calculating the OBC inflow area in tidal_bay_set_OBC_data(). This change allows for the elimination or replacement of 6 rescaling factors and one added instance with a consistent conversion factor and diagnostic units occur on the same line. All answers are bitwise identical.
Co-authored-by: Robert Hallberg <[email protected]>
Correct a bug that G%gridLonT/G%gridLatT were not using global indexing in outputing extreme surface information.
Previously extreme surface message is triggered when `sea_lev` is smaller than or **equal** to ocean topography: sfc_state%sea_lev(i,j) <= -G%bathyT(i,j) - G%Z_ref The equality is innocuous for a non-zero minimum thickness (Angstrom/=0), but it can be problematic for true zero thickness. Besides, there is the fourth criterion in this check: sfc_state%sea_lev(i,j) + G%bathyT(i,j) + G%Z_ref < CS%bad_val_col_thick This is supposed to allow a user-specified tolerance (default=0.0), which should be a more stringent check when the tolerance is larger than zero. The equality from the first check contradicts this logic.
Fix a bug that sea surface height averaged over every dynamic time step (SSH_inst) is not outputted correctly.
* Update MOM_wave_interface.F90 The index at the interface has been changed from I-1 ->i+1 and J-1 -> j+1, which helps fixed model error in 3D simulations while keeping halo_size to 1. * Update MOM_wave_interface.F90 Corrected the index for thickness which fixed the issue of 3D wave coupled simulations while keeping the halo_size in thickness as 1
- The present code only tests BBL_EFFIC when deciding whether to set the bottom TKE and ustar, but this means they are all zero when EPBL_BBL_EFFIC is non-zero and BBL_EFFIC is zero. - Adds logic to also check EPBL_BBL_EFFIC, thereby allowing non-zero ustar and TKE for EPBL_BBL_EFFIC>0.0 - Fixed BBL_TKE diagnostic in EPBL that was not populated. - Will change answers when EPBL_BBL_EFFIC>0.0, but won't change answers in any of our existing configurations.
* Move MOM_generic_tracer with no changes Renames src/tracer/MOM_generic_tracer.F90 to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90 * Remove MOM_generic_tracer Git rm MOM_generic_tracer.F90 --------- Co-authored-by: Theresa Morrison <[email protected]>
Added missing factors to the conversion arguments in the register_diag_field calls for the diagnostics of the KPP non-local transport tendency and the net surface tracer fluxes, and corrected the dimensional scaling that is being applied to the KPP_Vt2 diagnostic as calculated in KPP_compute_BLD. All solutions are bitwise identical, but now output files with these 3 sets of KPP diagnostics are invariant to dimensional rescaling. This can be verified with the visc.nc file generated by the single_column/KPP test case in MOM6-examples. Whereas previously the diagnostics KPP_Vt2, KPP_QminusSW, KPP_netSalt, KPP_NLT_dTdt and KPP_NLT_dSdt in that file would change when dimensional rescaling was applied, now they do not. No output is changed unless dimensional rescaling is used.
The `default=` optional argument to get_param() only provides a uniform value to initialize an array of integers. This commit adds an optional `defaults=` argument to get_param_int_array(), doc_param_int_array() and log_param_int_array() to allow for the specification of an array of default values. These additions are analogous to what had previously been added for real arrays in github.com//pull/760. This commit also adds the new internal function int_array_string(), analogous to real_array_string(), in MOM_document. This differs slightly from its real array counterpart in that it only uses the syntax like `3*75` for lists of integers that are longer than we would use to specify dates and times or pairs of layout parameters, because "(0, 0)" seems more readily interpretable than "(2*0)". The new defaults argument is now used in the get_param calls for LAYOUT and IO_LAYOUT, and in setting the tidal reference dates. Several spelling errors in comments were also corrected in the files that were being edited. All answers are bitwise identical, but there are minor changes in many MOM_parameter_doc.layout files and some MOM_parameter_doc.all files.
|
this PR passed UFS regression test on all platforms. @sanAkel is checking all modified codes at this moment and I will click approval when he give me greenlight (plus no code modification is required from NCAR side) |
@jiandewang I have no problems with anything! Please approve it. Thanks! |
|
Another source of answer changes is NOAA-GFDL#784. We are happy with those changes but there seems to be more causes for answer changes. I am looking further... |
alperaltuntas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn’t pinpoint the exact cause of the remaining answer changes, but there’s strong evidence they’re related to the reorganization of certain routines, which likely affected compiler optimizations. I believe @Hallberg-NOAA mentioned something along those lines earlier. All the failing tests (which are Intel-only) pass when compiled with GNU, which supports this idea. Reverting merge commit bf35f31 also seems to restore answers for at least some tests, though I wasn’t able to identify exactly which part of that merge is responsible. Given all that, I think we're ready to approve this PR.
|
Any ETA on when this PR will be merged or what's (yet) to do? |
|
We tried to do a little forensics into the answer changes from NCAR, but couldn't come up with anything. Given that everyone has reviewed and accepted the changes, I think it's safe to merge. |
In mom-ocean#1664, src/tracer/MOM_generic_tracers.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracers.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase
In mom-ocean#1664, src/tracer/MOM_generic_tracers.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracers.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase. This reverts commits 2208dd7 and fcf5fff.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase. This reverts commits 2208dd7 and fcf5fff.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase. This reverts commits 2208dd7 and fcf5fff.
In mom-ocean#1664, src/tracer/MOM_generic_tracer.F90 was moved to config_src/external/GFDL_ocean_BGC/MOM_generic_tracer.F90. This was done to simplify COBALT v3 development. However, having MOM_generic_tracer.F90 in the generic_tracers codebase doesnt work when compiling generic_tracers as a library, so we maintain MOM_generic_tracer.F90 in the MOM6 codebase. This reverts commits 2208dd7 and fcf5fff.
An Earth Day PR from GFDL 🌎, containing local contributions from the last few months.
This PR is expected to report a regression. Diagnostic checksum testing now includes a model timestamp, which is used to sort the output. This will prevent false negatives from reordering of
post_datacalls. But it will report a regression withmain, which does not have the timestamps.Features/Diagnostics
dtbtNOAA-GFDL/MOM6#823: (@herrwang0)RESET_INTXPA_INTEGRAL_FLATTESToption to pressure gradient NOAA-GFDL/MOM6#843: (@claireyung)Bugfix/Refactor
write_energywhenDT<2 NOAA-GFDL/MOM6#749: (@herrwang0)Rho0andrho_refin Boussinesq PGF NOAA-GFDL/MOM6#837: (@herrwang0)Infrastructure
get_param_array_int()NOAA-GFDL/MOM6#791: (@Hallberg-NOAA)TIDE_SAL_SCALAR_VALUENOAA-GFDL/MOM6#819: (@herrwang0)Contributors: