Enhancement: Point-Stat and Grid-Stat wrappers UGRID updates

JohnHalleyGotway commented 9 months ago

Describe the Enhancement

Issue dtcenter/MET#2231 and pull request dtcenter/MET#2723 adds support for reading unstructured grid information from NetCDF input files. Three new configuration options are added to both the Point-Stat and Grid-Stat default configuration files. This issue is to enhance those wrappers to add support for them.

ugrid_max_distance_km = 30;
ugrid_user_map_config: to override metadata settings (dimension and variable names)
ugrid_coordinates_file: to specify the static metadata file (for MPAS)

I note that these new config options were NOT documented well in the MET User's Guide.

Issue dtcenter/MET#2748 requests that @hsoh-u add documentation about these options and support @georgemccabe in updating METplus wrappers to set them.

Time Estimate

4 hours?

Sub-Issues

Consider breaking the enhancement down into sub-issues. None needed.

Relevant Deadlines

Ideally do this during the METplus-6.0.0-beta3 development cycle.

Funding Source

2770043

Define the Metadata

Assignee

[x] Select engineer(s) or no engineer required
[ ] Select scientist(s) or no scientist required Recommend @georgemccabe doing the work and asking @jvigh to review the resulting PR.

Labels

[x] Select component(s)
[x] Select priority
[x] Select requestor(s)

Projects and Milestone

[x] Select Repository and/or Organization level Project(s) or add alert: NEED CYCLE ASSIGNMENT label
[x] Select Milestone as the next official version or Future Versions

Define Related Issue(s)

Consider the impact to the other METplus components.

[x] METplus, MET, METdataio, METviewer, METexpress, METcalcpy, METplotpy None.

Enhancement Checklist

See the METplus Workflow for details.

[ ] Complete the issue definition above, including the Time Estimate and Funding Source.
[ ] Fork this repository or create a branch of develop. Branch name: feature_<Issue Number>_<Description>
[ ] Complete the development and test your changes.
[ ] Add/update log messages for easier debugging.
[ ] Add/update unit tests.
[ ] Add/update documentation.
[ ] Add any new Python packages to the METplus Components Python Requirements table.
[ ] Push local changes to GitHub.
[ ] Submit a pull request to merge into develop. Pull request: feature <Issue Number> <Description>
[ ] Define the pull request metadata, as permissions allow. Select: Reviewer(s) and Development issues Select: Repository level development cycle Project for the next official release Select: Milestone as the next official version
[ ] Iterate until the reviewer(s) accept and merge your changes.
[ ] Delete your fork or branch.
[ ] Close this issue.

georgemccabe commented 9 months ago

According to PR dtcenter/MET#2723, the new variable names in GridStat and PointStat are:

ugrid_dataset = "";
ugrid_max_distance_km = 30;
ugrid_coordinates_file = "";

This differs from the names in the original post on this issue (ugrid_user_map_config is included and ugrid_dataset is not). I assume the correct variable names are what is found in the code changes for the PR, but I wanted to make sure. @JohnHalleyGotway or @hsoh-u, please let me know.

georgemccabe commented 9 months ago

I also noticed that ugrid_user_map_config is not in the default but it is commented out in the MET test config files.

georgemccabe commented 6 months ago

@hsoh-u, @JohnHalleyGotway , and @DanielAdriaansen : please see the above discussion and advise on the actual names of the config variables that should be supported via METplus wrappers.

DanielAdriaansen commented 6 months ago

Argh thanks @georgemccabe. There are four items to discuss:

ugrid_metadata_map
ugrid_dataset
ugrid_max_distance_km
ugrid_coordinates_file

I am not sure how this works, but ugrid_metadata_map lives in separate MET config files depending on the UGRID name: https://github.com/dtcenter/MET/blob/v12.0.0-beta3/data/config/UGridConfig_lfric https://github.com/dtcenter/MET/blob/v12.0.0-beta3/data/config/UGridConfig_mpas

Maybe @hsoh-u can comment, but I do not think this needs to be exposed via METplus wrappers. I think a user with a new UGRID (not "mpas" or "lfric") would need to create one of these files UGridConfig_newugrid and place it inside the MET config file directory, and then set ugrid_dataset=newugrid via the PointStat, GridStat, config files or METplus Wrappers.

Thus, I think we need three items supported:

ugrid_dataset
ugrid_max_distance_km
ugrid_coordinates_file

These currently exist for both PointStat and GridStat, thus I propose:

POINT_STAT_UGRID_DATASET
POINT_STAT_UGRID_MAX_DISTANCE_KM
POINT_STAT_UGRID_COORDINATES_FILE
GRID_STAT_UGRID_DATASET
GRID_STAT_UGRID_MAX_DISTANCE_KM
GRID_STAT_UGRID_COORDINATES_FILE

DanielAdriaansen commented 6 months ago

Both grid_stat and point_stat also have a new command line switch:

"-config config_file" specifies additional PointStatConfig file containing the configuration settings for unstructured grid (optional).

I propose: POINT_STAT_UGRID_CONFIG_FILE GRID_STAT_UGRID_CONFIG_FILE

DanielAdriaansen commented 5 months ago

@georgemccabe I spoke with @willmayfield and @hsoh-u today and we agree that there does not need to be support for ugrid_metadata_map, and that providing support for the -config command line switch for grid_stat and point_stat is sufficient.

In summary, we'd like support for:

POINT_STAT_UGRID_DATASET --> MET conf
POINT_STAT_UGRID_MAX_DISTANCE_KM --> MET conf
POINT_STAT_UGRID_COORDINATES_FILE --> MET conf
GRID_STAT_UGRID_DATASET --> MET conf
GRID_STAT_UGRID_MAX_DISTANCE_KM --> MET conf
GRID_STAT_UGRID_COORDINATES_FILE --> MET conf
POINT_STAT_UGRID_CONFIG_FILE --> CL arg
GRID_STAT_UGRID_CONFIG_FILE --> CL arg

Happy to work with you on testing when you're able to get to this. I am not certain that the charge key is still relevant. I will ping Tara and ask.

DanielAdriaansen commented 5 months ago

@TaraJensen can you confirm the charge key for this work? Should it be what's currently listed (2770043) or something else? Thanks.

TaraJensen commented 5 months ago

See email for response

dtcenter / METplus