(+) Refactor of MOM_file_parser

[marshallward]

This patch includes several minor changes to the MOM_file_parser and
supporting modules in order to accommodate stronger unit testing.

It includes the following API changes:

• Removal of static_value from get_param

• Redefined link_parameter and parameter_block as private

• New functions: all_across_PEs(), any_across_PEs()

static_value was not used in any known experiments (outside of
internal GFDL testing), and the two derived types describe internal
operations within MOM_file_parser, so we do not expect any disruptions
from these changes.

A detailed summary of the changes are listed below.

• assert() is now used to detect same files with different IO units.

  Detection of reopenend files of the same name but different IO unit
  has been changed from MOM_error(FATAL, ...) to assert(), to
  reflect that this should be a logically impossible result.

• Bugfix: Reopened files are now reported to all PEs.

  If an open file is re-opened, then only the root PE will detect this
  and will return immediately. However, the others will proceed into
  populate_param_data and will get stuck in a broadcast waiting for
  root.

  We fix this by communicating the reopened state to all PEs and allow
  all ranks to return before re-processing the data.

  Note that this could also be resolved by allowing all ranks to track
  IO unit numbers, but for now we do not attempt to change this
  behavior.

• newunit= used to generate parameter file IO unit

  The parameter IO unit is now generated by newunit= rather than an
  explicit search for an unused IO unit. Note that this is a Fortran
  2008 feature.

  Testing around available IO units has also been removed.

• Removal of generic IO error handling

  Generic "IO error" tests, and corresponding err= arguments, have
  been removed in most cases. We now rely on the Fortran runtime to
  provide diagnostics on these errors, which should typically exceed any
  information that MOM6 could provide.

• Removal of purported namelist support

  There were several blocks of code provided to support namelist syntax,
  but did not appear to be working, nor was there any known instance of
  it being used by anyone, so it has been removed.

• #define/undef/= syntax testing across ranks

  Previously, only the root PE would test for consistency of the
  #define-like syntax, even though all ranks have this information.
  This required a second, awkwardly placed syntax test later in the
  subroutine.

  This test is redefined to run over all ranks, and the subsequent test
  has been removed.

• define/override test reordering

  The found_override test when coupled to a #define-like declaration
  was unreachable due to the presence of an even stronger test related
  to valid syntax.

  This test has been moved to provide more detailed information about
  the nature of the error.

• link_parameter, parameter_block defined as private

  Internal derived types of MOM_file_parser are redefined as private.
  This preserves the integrity of instances of these types, and also
  prevents creation of implicit object code required to access them
  externally.

• Removal of static_value from get_param interface

  The static_value argument of get_param has been removed, since it
  is functionally equivalent to default. While this is an API change,
  there is no known case of anyone using this argument.

• The param_type%doc fields are now properly deallocated after closed.

• Quotes have been added around some filename error warnings, to help
  detect issues related to whitespace.

• any_across_PEs and all_across_PEs

  New functions for calling any() and all() across PE ranks have
  been added. Behavior is in line with other functions, such as
  min_across_PEs.

[codecov]

Codecov Report

Merging #74 (6f70955) into dev/gfdl (a468bee) will increase coverage by 0.00%.
The diff coverage is 41.17%.

❗ Current head 6f70955 differs from pull request most recent head 9caa701. Consider uploading reports for the commit 9caa701 to get more accurate results

@@            Coverage Diff            @@
##           dev/gfdl      #74   +/-   ##
=========================================
  Coverage     29.03%   29.04%           
=========================================
  Files           244      244           
  Lines         71865    71849   -16     
=========================================
  Hits          20869    20869           
+ Misses        50996    50980   -16     

Impacted Files	Coverage Δ
src/core/MOM_verticalGrid.F90	70.21% <ø> (ø)
src/framework/MOM_coms.F90	55.83% <ø> (ø)
config_src/infra/FMS1/MOM_coms_infra.F90	41.73% <36.36%> (+1.17%)	⬆️
src/framework/MOM_file_parser.F90	62.82% <42.10%> (+1.74%)	⬆️
src/framework/MOM_domains.F90	50.84% <50.00%> (-0.88%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a468bee...9caa701. Read the comment docs.

[Hallberg-NOAA]

As written, the logic on lines 468 and 469 seems to be implementing the parallel version of .not.any(), and not the parallel version of all(). To correctly implement all_across_PEs() as the analog of all(), I think that line should be all_across_PEs = (field_flag > 0).

Exactly the same issue pertains to config_src/infra/FMS2/MOM_coms_infral.F90.

The fact that there are two identical routines in the two infra/FMS[12] directories, neither of which calls an any FMS infrastructure interfaces directly, suggests that these new routines should perhaps be implemented in src/framework/MOM_coms.F90.

[marshallward]

Looks like you are right. As you might have guessed, all_across_PEs was not used anywhere. Perhaps adding it was a mistake.

I believe this is better placed in the infra layer, because the conversions to integers and use of {min,max}_across_PEs reflects the absence of logical MPI collectives in FMS and is thus a uniquely FMS operation. A different framework would not have this restriction.

[marshallward]

all_across_PEs test has been updated

[Hallberg-NOAA]

The changes to MOM_file_parser.F90 seem well conceived.

However, as noted in detail in a separate comment attached to one of the relevant lines, the logic in the new all_across_PEs() routine looks to me like it is incorrect. Although any_across_PEs() is being used, I don't see where all_across_PEs() is being used, which could be how faulty logic could have slipped through. Moreover, I don't understand why the new any_across_PEs() and all_across_PEs() routines should be in the multiple config_src/infra/*/MOM_coms_infra.F90 files, rather than being implemented just once directly in src/framework/MOM_coms.F90.

[Hallberg-NOAA]

This PR is not compiling in static memory mode. The static_value= argument at https://github.com/NOAA-GFDL/MOM6/blob/dev/gfdl/src/core/MOM_verticalGrid.F90#:~:text=call%20get_param(param_file%2C%20mdl%2C%20%22NK,static_value%3DNK_) (i.e. at about line 145 of core/MOM_verticalGrid.F90) needs to be changed to `default='.

[marshallward]

Thank you for detecting this error, I've updated the PR.

[Hallberg-NOAA]

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14861 .