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

NCAR to main 2024-03-19 #1621

Merged
merged 11 commits into from
Apr 1, 2024
Merged

Conversation

alperaltuntas
Copy link
Collaborator

This PR contains enhancements and bug fixes from NCAR fork as listed below. In summary, these include (1) automated runtime land block elimination based on automatically generating a mask_table during domain initialization at runtime, (2) rotational symmetry fixes and efficiency improvements in Leith+E, and misc bug fixes.

Since this PR includes changes in the NUOPC cap, I'd recommend EMC to first run their tests and approve, before others run their full test suites.

alperaltuntas and others added 9 commits November 6, 2023 14:10
* Enhancements for adding land block elimination to NUOPC cap:
 - Add sum_across_PEs_int4_2d to the sum_across_PEs interface
 - Allow mask_table file to be placed in run directory (now,
the first dir that is looked at).

* Enhance NUOPC cap to support MOM_mask_table.

- Determine masked blocks.
- Evenly distribute eliminated cells.
- Fill ESMF gindex array accordingly.
- During Export phase, set fields of eliminated cells to zero.

* set %label in register_netcdf_field and register_netcdf_axis

* first working version of an automated mask table generator

* While determining masked blocks, take reentrancy and tripolar stitch into account

* apply tripolar stitch fix in auto mask_table generation

* add AUTO_IO_LAYOUT_FAC parameter to control IO_LOAYUT when AUTO_MASKTABLE is on

* Miscellaneous auto masking fixes to address reviews:

- Dimensionalize topographic depth variables used to determine cell masks in auto masktable routine.
- Raise error if the user provided PE layout is inconsistent with auto masktable generation.
- Save the masktable parameter description to a string variable to avoid repetition.
- Fix typos, whitespaces, use modern array syntax.

* Disable FPEs in MacOS testing

Due to poor handling of floating point in HDF5 1.14.3, it is currently
not possible to use floating point exceptions (FPEs) whenever this
version is present.

The GitHub Actions CI nodes would randomly select either 1.14.2 or
1.14.3, and would raise an FPE error if 1.14.3 was selected.
Additionally, the homebrew installation does not provide a clean method
for selecting a different version of HDF5.

Thus, for now we disable FPEs in the MacOS testing, and hope to catch
any legitimate FP errors in the Ubuntu version.  We will restore these
tests as soon as this has been fixed in an easily-accessible version of
HDF5.

As part of this PR, I have also moved the FCFLAGS configuration to the
platform specific Actions files, allowing for independent compiler
configuration for each platform.

---------

Co-authored-by: Marshall Ward <[email protected]>
* Fix biharmonic Leith

Biharmonic Leith uses Del omega at is-1 and js-1. This unavoidably requires
u at js-3 and v at is-3, which are unavailable. It also requires Del omega
at Ieq+1 and Jeq+1, which requires v at Ieq+3 and u at Jeq+3, which are
unavailable. This necessitates a halo update.

Fixes several bugs in Leith+E.
- Fixes indexing when computing smoothed vorticity and its gradient
- Crucially, computes `vert_vort_mag` when using Leith+E
- Fixes some logic in the smoothing code
- Other minor indexing fixes

* Leith+E Logic Update

Ah is required at h and q points. The original code computed Ah at
h points, then packed into Ah_h, then applied upper bounds to Ah.
If Ah_h is in the diag_table or if debug is true, then the value of
Ah with upper bounds get packed into Ah_h. Then, at q points the
code unpacks Ah_h. This update makes sure that the upper bound
gets applied to q points, not just h points.

* Leith+E halo updates

The main thing that this commit does is to perform smoothing of u and v
outside of the loop over layers. This swaps nz 2D blocking halo updates
for a single blocking 3D halo update.

* Leith+E smoothing

This commit adds a runtime flag, SMOOTH_AH. If True (default) then
`m_leithy` and `Ah` are both smoothed, which leads to many blocking
communications. If False then these fields are rougher, but there
is less communication.

* Leith+E eliminate pass-var

This commit removes one halo update in Leith+E. To achieve this
requires re-indexing two assignments. The value of Ah and Kh are
computed at h points, then re-used at q points. Without the halo
update it is necessary to offset the assignment at h and q
points, e.g. Kh(I,J) = Kh_h(i+1,j+1,k), to avoid accessing
values that have not been computed.

* Leith+E OBC

Adds code so that Leith+E works with OBC.

* Leith+E eliminate halo update

This commit eliminates one more halo update in Leith+E.

* *Correct rotational symmetry with USE_LEITHY

  This commit revises the smoothing code used when USE_LEITHY = True to give
answers that respect rotational symmetry and it also corrects some horizontal
indexing bugs and problems with the staggering in some halo update and smooth_x9
calls and reduces some loop ranges to their minimal required values.  The
specific changes include:

  1. Corrected a horizontal indexing bug when interpolating Kh_h and Ah_h to
     corner (q) points when USE_LEITHY = True.  These had previously been
     inappropriately copied from the thickness point to the southwest of the
     corner point.  This required symmetric-memory-mode calculations of the
     thickness point viscosities whenever USE_LEITHY is true, but to avoid adding
     complicated logic, the symmetric-memory loop bounds are used for the
     calculation of Kh.

  2. Revised smooth_x9 to give rotationally symmetric answers and split it into
     the two routines smooth_x9_h and smooth_x9_uv to reduce the memory used by
     this routine and reduce the use of optional arguments.

  3. Eliminated 4 unneeded halo update calls, and added error handling for the
     case where Leith options are used with insufficiently wide halos.

  4. Added new integers to indicate the loop ranges over which the viscosities
     and related variables should be calculated, depending on which options are
     active, and then adjusted 91 do-loop extents horizontal_viscosity code to
     reflect the loop ranges over which arrays are actually used.

  5. Added a new 2-d variable for the squared viscosity for smoothing that can
     be used for halo updates and to avoid having a variable with confusingly
     inconsistent dimensions at various points in the code.

  6. Corrected the position arguments on 2 smooth_x9 calls and 4 pass_var calls
     that are used when USE_LEITHY=.true. and SMOOTH_AH=.true.  As previously
     written, these smooth_x9 and pass_var calls would work when in non-symmetric
     memory mode but would give incorrect answers when in symmetric memory mode.

  These revisions change answers when USE_LEITHY is true, but answers are
bitwise identical in all other cases.

---------

Co-authored-by: Robert Hallberg <[email protected]>
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 22.32416% with 254 lines in your changes are missing coverage. Please review.

Project coverage is 37.40%. Comparing base (2ab885e) to head (e75d1da).

Files Patch % Lines
src/parameterizations/lateral/MOM_hor_visc.F90 30.64% 117 Missing and 12 partials ⚠️
src/framework/MOM_domains.F90 10.00% 110 Missing and 7 partials ⚠️
config_src/infra/FMS2/MOM_coms_infra.F90 0.00% 3 Missing ⚠️
config_src/infra/FMS2/MOM_domain_infra.F90 0.00% 3 Missing ⚠️
src/ice_shelf/MOM_ice_shelf.F90 0.00% 1 Missing ⚠️
src/ocean_data_assim/MOM_oda_driver.F90 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1621      +/-   ##
==========================================
- Coverage   37.45%   37.40%   -0.05%     
==========================================
  Files         270      270              
  Lines       79765    79914     +149     
  Branches    14833    14860      +27     
==========================================
+ Hits        29878    29894      +16     
- Misses      44350    44471     +121     
- Partials     5537     5549      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@kshedstrom kshedstrom left a comment

Choose a reason for hiding this comment

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

This doesn't change answers for me, so I approve. However, it changed the MOM_domains_init interface which requires PR #207 to SIS2. Also, a case of mine which is coupled to ice, but has no ice, now needs to have TOPO_CONFIG and/or TOPO_FILE set in SIS_input.

Hallberg-NOAA and others added 2 commits March 20, 2024 12:09
  This commit makes the unit_scale_type argument US to MOM_domains_init and
gen_auto_mask_table optional and moves it to the end of the argument list, so
that coupled or ice-ocean models using SIS2 will compile with the proposed
updates to the main branch of MOM6 from dev/ncar.  Because MOM6 and SIS2 use
some common framework code but are managed in separate github repositories, we
need to use optional argument to allow a single version of SIS2 to work across
changes to MOM6 interfaces.  Because the TOPO_CONFIG parameter as used in SIS2
has a default value, there is an alternative call to get_param for TOPO_CONFIG
with a default when MOM_domains_init is called with a domain_name argument.
Also added missing scale arguments to get_param calls for MINIMUM_DEPTH and
MASKING_DEPTH.  This commit also adds or corrects units in the comments
describing 4 recently added or modified variables.  All answers are bitwise
identical in any cases that worked before (noting that some cases using SIS2
would not even compile).
Make the US argument to MOM_domains_init optional
@sanAkel
Copy link
Collaborator

sanAkel commented Mar 22, 2024

@alperaltuntas what's the highest (finest) resolution that has been tested for the automatic mask table generation? Any performance issues? (just asking!)

@jiandewang
Copy link
Collaborator

preliminary test shows no problem for UFS. However our main testing machine HERA is undergoing migration from CentOS to Rocky 8 which preveting me from doing clean test at this moment. Please bear me a bit delay for final testing.

@alperaltuntas
Copy link
Collaborator Author

@alperaltuntas what's the highest (finest) resolution that has been tested for the automatic mask table generation? Any performance issues? (just asking!)

The highest I tried was 0.25 deg. If you are asking about the performance of the iteration, it's quite fast for both 0.66 and 0.25 degrees (on the order of 0.1sec). If we discover in the future that it is slow for finer resolutions (e.g., 1/12 deg), I have plans to speed up the iteration, but for now, I opted for a simpler implementation for ease of maintenance. Other than that, I have not encountered any performance issues.

Copy link
Collaborator

@abozec abozec left a comment

Choose a reason for hiding this comment

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

COAPS approves.

@sanAkel
Copy link
Collaborator

sanAkel commented Mar 25, 2024

@alperaltuntas what's the highest (finest) resolution that has been tested for the automatic mask table generation? Any performance issues? (just asking!)

The highest I tried was 0.25 deg. If you are asking about the performance of the iteration, it's quite fast for both 0.66 and 0.25 degrees (on the order of 0.1sec). If we discover in the future that it is slow for finer resolutions (e.g., 1/12 deg), I have plans to speed up the iteration, but for now, I opted for a simpler implementation for ease of maintenance. Other than that, I have not encountered any performance issues.

That ( on the order of 0.1sec) sounds great! Thank you!

@jiandewang
Copy link
Collaborator

@alperaltuntas what is automatic mask table generation ?

@jiandewang
Copy link
Collaborator

@alperaltuntas if we use land mask, then the lat/lon information on masked domain will be written as undefined value, is that true ? See an example at
https://www.emc.ncep.noaa.gov/gc_wmb/wd20xw/JD/ocn_2013_04_01_06-geolon.txt

@jiandewang
Copy link
Collaborator

@alperaltuntas one more minor point: in restart file variable "h" are all 1e-10 on land points but using land mask approach pure land domian have some points (but not all of them) with value of zero. All ocean points have identical values which is what we expected.
see samples files at https://www.emc.ncep.noaa.gov/gc_wmb/wd20xw/JD/NCAR/

Copy link
Collaborator

@jiandewang jiandewang left a comment

Choose a reason for hiding this comment

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

finally machine is back to normal. Sorry for the delay

Copy link
Collaborator

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

This is good on the GFDL side now, and this version passes our regression testing.

@alperaltuntas
Copy link
Collaborator Author

Thank you all for the reviews and confirmations. I am merging the PR.

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.

9 participants