Dm diag_yaml

[uramirez8707]

Description

• Adds two public subroutines to fms_diag_yaml_mod, diag_yaml_object_init which fills in the diag_yaml_obj and diag_yaml_object_end, which destroys the diag_yaml_obj. They are called in diag_manager_init and diag_manager_end respectively.
• Removes any c structs that were meant to hold the diag_table.yaml file and made the corresponding changes to the Makefile.am
• All the code in fms_diag_yaml_mod is hidden inside the -Duse_yaml macro because yaml_parser_mod won't compile without it

Fixes # (issue)

Still to do:

• Error checking: check that diag_table entries are valid?
• Test diag_yaml_object_init

How Has This Been Tested?
make check still passes with intel/gnu
make check passes with --with-yaml on skylake with intel

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules
• New check tests, if applicable, are included
• make distcheck passes

[bensonr]

There was a discussion on renaming of region to sub-region. I see much of the code still uses the region nomenclature with a smattering of sub-region. For consistency sake we should refer to it all the same.

[bensonr]

For the sub-regions, I want to ensure I understand the intent. This storage object will accommodate the use of four pairs of lat-lon values or i-j indexes. What I don't see is some way of constraining or limiting the vertical extent nor specifying the particular tile for the index option.

[uramirez8707]

What I don't see is some way of constraining or limiting the vertical extent

I am not sure I understand what you mean. The *_region arrays store the bounds for all four of the possible dimensions: [dim1_min, dim1_max, dim2_min, dim2_max, dim3_min, dim3_max, dim4_min, dim4_max].

specifying the particular tile for the index option.

Yes, I forgot to add the "tile". It will be a required key if using the "index" approach. This will be added in the next commit.

[bensonr]

This is where confusion comes in on how a sub-region is being defined. I know in the past we've always defined the sub-region to have sides of constant lat or lon. I saw the eight entries here and thought maybe we were expanding the capability to create somewhat irregular sub-regiosn denoted by either (i,j) or (lat,lon) for each fo the corners of the sub-region se, sw, ne, nw.

[thomas-robinson]

Can you extend the sub_region_type instead of using a flag?

type sub_region_type
end type
type, extends (sub_region_type) :: lat_lon_sub_region_type
     real :: lat_lon_region (NUM_REGION_ARRAY) !< Bounds of the regional section to capture if in "latlon" mode
end type lat_lon_sub_region_type
type, extends (sub_region_type) :: index_sub_region_type
     integer :: index_region (NUM_REGION_ARRAY) !< Bounds of the regional section to capture if in "index" mode
end type index_sub_region_type

Then your variables can be class (sub_region_type). I think this is better/faster/more extendable than reading a string.

[uramirez8707]

I don't understand. How will the code know which sub_region_type is inside in each diag_yaml%diag_files(:) type?

[thomas-robinson]

This looks like work that could be shared between 2 threads because one loop doesn't depend on the other.

[thomas-robinson]

Rename the yaml file that is used for diagnostic input without "table" in the name

[thomas-robinson]

The type names need to follow the style guide https://github.com/NOAA-GFDL/FMS/blob/main/CODE_STYLE.md#derived-types

[thomas-robinson]

3 things, and they're all in diag_manager/fms_diag_yaml_object.F90

1. in type subRegion_type the arrays should be allocatable because this set up is wasting memory. You may be able to remove grid_type if you are only allocating one of them. Alternatively, you could also use two types that extend type subRegion_type, They would be named something like type subRegionIndicies_type and type subRegionLatLon_type, and each would have the correct variable defined on the stack like how you have it now.
2. Line 81 should have a comment about why the character length is set and not allocatable. In the future, someone will look at this and change it to a : without any context for why it's set up like this.
3. Same as 2 but line 117.

[thomas-robinson]

Yes, you need both of them. But they should be allocatable, and only one should be allocated. Then you shouldn't need the grid_type variable because only one will be allocated (unless the string is used downstream for some reason).
With this set up where they are both on the stack, you are just wasting memory.

[thomas-robinson]

I think you should add a comment about why len=MAX_STR_LEN and not : for someone else who will have to deal with this in the future.