Add a deta_dt diagnostic to the split BT/BC mode #99
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
|
I'm very sad that this repo thinks this is the first time I've contributed to MOM6 :( |
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #99 +/- ##
=========================================
Coverage 28.76% 28.76%
=========================================
Files 248 248
Lines 72967 72976 +9
=========================================
+ Hits 20990 20995 +5
- Misses 51977 51981 +4
Continue to review full report at Codecov.
|
How odd - did you change your GH account? |
klindsay28
reviewed
Apr 1, 2022
klindsay28
reviewed
Apr 1, 2022
|
@adcroft apparently it have every right to be suspicious of me...trailing whitespace, incorrect unit conversions- I'll show myself out. |
d03be27 to
d806df8
Compare
Hallberg-NOAA
requested changes
Apr 10, 2022
There was a problem hiding this comment.
This PR is very close to being ready to go. However, there is one actual bug and 3 other very minor things that I would like to see changed before accepting it.
- The non-Boussinesq unit conversion for the new diagnostic on line 1353 is wrong, in that it is inconsistent with the long-description and text describing the units, and I believe (based on the conversation surrounding Remove conversion to meters for diagnostics in non-Boussinesq mode mom-ocean/MOM6#1565) is the exact opposite of the author's intentions. Please either use
conversion=GV%H_to_mks*US%s_to_T(the preferred solution) orconversion=GV%H_to_kg_m2*US%s_to_Ton line 1353. - The newly introduced variable dHdT_rescale (line 1123) is unused and should be removed.
- post_data is a call that triggers communication and synchronization between processors. Although the diagnostic deta_dt is calculated exactly where it needs to be calculated, it might be more efficient for the call to post_data for deeta_dt to be moved later in the routine to where the other post_data calls occur.
- Please follow the pattern for the other diagnostic id and initialize id_deta_dt to -1 on line 180.
The SSH tendency can now be computed for the split baroclinic/ bartropic (integrated with RK2) way of calculating the dynamics. This extra diagnostic is needed for computing vertical mass transport (wmo) in z* coordinates offline from the convergence of the horizontal mass transports. The meaning, units, and conversion factors associated with the deta_dt diagnostic change between Boussinesq and non-Boussinesq modes. This commit accounts for these differences and reports the units consistent with the mass transports.
This commit adds the ability to use Hybgen regridding, derived from Hycom code. There are two new files, MOM_hybgen_regrid.F90 and MOM_hybgen_unmix.F90, which in turn add 15 new runtime parameters (with names like HYBGEN_...) to control the hybgen code. This new regridding option is specified via REGRIDDING_COORDINATE_MODE="HYBGEN", and this new option is listed in the MOM_parameter_doc files for cases that have USE_REGRIDDING=True. There is also a new publicly visible parameter, REGRIDDING_HYBGEN, in regrid_consts.F90. In addition, the new routine regridding_preadjust_reqs is provided in MOM_regridding to specify whether convective adjustment or hybgen_unmixing should be done before regridding. This is used along with the optional argument conv_adjust to regridding_main to move these pre-adjustment steps out of regridding_main. The unused routine ALE_build_grid was eliminated, and comments were added describing a few undocumented internal real variables. All answers are bitwise identical, but there are several new public interfaces and there will be new lines in the comments in some MOM_parameter_doc files.
Eliminated the option to do convective adjustment within regridding_main, including making the optional argument conv_adjust mandatory, with an error message if it is set to .true., so that there will not be the possibility of this code change silently changing the code behavior or solutions. After a decent interval, this argument can be safely eliminated. As a result of these changes, the intent of two key arguments to regridding_main() could be changed from intent(inout) to intent(in), which should help clarify the purpose of this routine. All answers are bitwise identical, but there are interface changes.
Remove the pointer attribute from tides_CS in its parent CS in unsplit mode. This recovers tidal forcing functionality in the unsplit mode.
- Ever since a renaming of driver directories, the tests under "test-top-api", that are meant to check we are compatible with each "cap", have been failing silently. - This fixes the wrong arguments but does not fix the ability to fail silently which is somewhere deep inside mkmf.
Modified the syntax of the logical comparisons with the various mask2d variables to always check whether they are larger than 0.0, not 0.5 and use similar syntax throughout the MOM6 code. Because these arrays are always either 0.0 or 1.0, the answers are bitwise identical.
Address undocumented interfaces by adding explicit interfaces to replaces one
level of the pass-throughs or eliminated them. Specifically
- Eliminated global_field_sum() from MOM_domains, as it is no longer used
anywhere in the MOM6 code or drivers. At the config_src/infra level, the
pass-through calls to mpp_global_sum are commented out in case there are
other versions of the MOM6 code that still use global_field_sum.
- Added explicit interfaces to horizontal_interp_init() and
time_interp_extern_init() in the config_src/infra rather that leaving them
as undocumented pass-throughs to the FMS code.
- Added the explicit subroutine stdout_if_root() to MOM_io.F90 to replace an undocumented
pass-through to the FMS stdout function.
- Added comments describing the publicly visible encoding parameters ind_flux,
ind_alpha, and ind_csurf that are a part of the coupler_types module.
- Removed the unnecessary or commented-out public declarations for
post_data_1d_k, vert_fill_TS, and legacy_diabatic.
All answers are bitwise identical, but there are some minor changes to the
names of subroutines offered at the config_src/infra level to permit the
documentation of these interfaces without running afoul of some bugs with
certain versions of the gnu compiler.
This patch removes any non-standard compiler extensions from the source and replaces them with equivalent standard statements. Specifically, this allows the code to be compiled with GCC using the -std=f2018 flag. None of these changes impact GFDL testing, since all extensions were supported by GNU, Intel, and PGI, but at least offer some greater assurance that the code will compile and behave more predicably on as-yet untested platforms. The specific changes are outlined below: * `loc() == 0` tests have been replaced with `associated()` where appropriate. (If field is not a pointer then it was removed, since using loc() was already a very bad idea.) * the `dfloat()` type conversion was replaced with `real()`. For consistency with existing MOM code, the `kind` remains unspecified. Technically this could represent an answer change, but was only applied to nz, which is always a very small integer. * `abort()` is an extension, and is replaced with the compliant (and admittedly platform-dependent) `error stop` statement.
Removed the unused and unnecessary function append_substring. All answers are bitwise identical, but an unused public interface is being eliminated.
* Fix non-standard white space Made widespread corrections to indentation and other white space issues to conform to the MOM6 standards, as documented in the white space section of the MOM6 style guide, at https://github.com/mom-ocean/MOM6/wiki/Code-style-guide. - MOM6 code uses a 2-point indent scheme. - There should be a space around all assignment (" = ") operators. - The names of optional arguments do not use whitespace, to differentiate them from assignments. - There is a space around the semicolons for stacked enddo statements. After this change, greping for '^ [a-zA-Z]', '^ [a-zA-Z]' or '^ [a-zA-Z]' does not find any lines in the MOM6 src or config_src/infra code. All answers and output are bitwise identical.
6441a02 to
3b3fda0
Compare
|
@Hallberg-NOAA as always, thank you for your constructive feedback and careful review. Please let me know if you have any other comments. |
Hallberg-NOAA
approved these changes
Apr 19, 2022
|
This PR has conditionally passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/15277, but because the available_diags files include new entries, MOM6-examples will also need to be updated. Because of the large number of commits in this relatively simple PR, I am using a squash merge on this PR. |
Hallberg-NOAA
added a commit
to Hallberg-NOAA/MOM6
that referenced
this pull request
Apr 20, 2022
Added a call to set thickness_units, which are used in the units description for the diagnostic deta_dt, which was recently added by NOAA-GFDL#99, but with an uninitialized variable being used for the units description. This changes contents of one entry in the MOM_available_diags, which will once again reproduce across runs and compilers. All solutions are bitwise identical.
marshallward
pushed a commit
that referenced
this pull request
Apr 21, 2022
Added a call to set thickness_units, which are used in the units description for the diagnostic deta_dt, which was recently added by #99, but with an uninitialized variable being used for the units description. This changes contents of one entry in the MOM_available_diags, which will once again reproduce across runs and compilers. All solutions are bitwise identical.
marshallward
pushed a commit
that referenced
this pull request
Sep 12, 2022
(1) update to MOM6 main branch 20220729 commit (GFDL-candidate-20220721) (2) cap update: supporting traditional and ESMF-managed threading
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.
The SSH tendency can now be computed for the split baroclinic/
bartropic (integrated with RK2) way of calculating the dynamics.
This extra diagnostic is needed for computing vertical mass
transport (wmo) in z* coordinates offline from the convergence
of the horizontal mass transports.
The unsplit cases still need an equivalent diagnostic added.
@adcroft