Feature #2433 Ugrid config variables in GridStat/PointStat

georgemccabe commented 4 months ago

Also moved the seeps variable up so that it matches the order of the default config files in the MET repo

Pull Request Testing

[X] Describe testing already performed for these changes:

Added unit tests

[X] Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

Test that new settings work as expected in an example use case.

[X] Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
[X] Do these changes include sufficient testing updates? [Yes]
[X] Will this PR result in changes to the test suite? [No]
[X] Please complete this pull request review by 3/25/2024.

Pull Request Checklist

See the METplus Workflow for details.

[X] Add any new Python packages to the METplus Components Python Requirements table.
[X] Review the source issue metadata (required labels, projects, and milestone).
[X] Complete the PR definition above.
[X] Ensure the PR title matches the feature or bugfix branch name.
[X] Define the PR metadata, as permissions allow. Select: Reviewer(s) Select: Organization level software support Project or Repository level development cycle Project Select: Milestone as the version that will include these changes
[x] After submitting the PR, select the :gear: icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
[ ] After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
[ ] Close the linked issue and delete your feature or bugfix branch from GitHub.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 8743687808

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

15 of 15 (100.0%) changed or added relevant lines in 3 files are covered.
10 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.04%) to 90.947%

Files with Coverage Reduction	New Missed Lines	%
metplus/wrappers/command_builder.py	1	94.31%
metplus/util/config_metplus.py	4	87.83%
metplus/wrappers/ascii2nc_wrapper.py	5	89.06%
<!--	Total:	10	-->

Totals
Change from base Build 8269428481:	-0.04%
Covered Lines:	8599
Relevant Lines:	9455

💛 - Coveralls

DanielAdriaansen commented 4 months ago

Testing summary so far:

Test 1

POINT_STAT_UGRID_DATASET=lfric2
POINT_STAT_MAX_DISTANCE_KM=30
POINT_STAT_UGRID_COORDINATES_FILE=/home/dadriaan/projects/ugrid/data/fcst/lfric/20240112_ftp/lfric_2021-07-28_00.00.00.nc
POINT_STAT_UGRID_CONFIG_FILE=/home/dadriaan/projects/ugrid/data/met_base/config/UGridConfig_lfric2

ERROR  : 
ERROR  : UGridFile::set_dataset() The UGrid dataset "lfric2" is not supported. Please add "/d1/personal/hsoh/git/features/feature_2746_ugrid_nearest_n_points/MET/share/met/config/UGridConfig_lfric2".
ERROR  :

Test 2

POINT_STAT_UGRID_DATASET=
POINT_STAT_MAX_DISTANCE_KM=30
POINT_STAT_UGRID_COORDINATES_FILE=/home/dadriaan/projects/ugrid/data/fcst/lfric/20240112_ftp/lfric_2021-07-28_00.00.00.nc
POINT_STAT_UGRID_CONFIG_FILE=/home/dadriaan/projects/ugrid/data/met_base/config/UGridConfig_lfric2

ERROR  : 
ERROR  : process_command_line() -> ugrid_dataset is not defined at the configuration file.
ERROR  :

JohnHalleyGotway commented 3 months ago

Reassigning this PR to the beta5 development cycle and convert it to draft until work can resume on it.

DanielAdriaansen commented 3 months ago

@DanielAdriaansen update contributors guide to say the word squash in this section: https://metplus.readthedocs.io/en/develop/Contributors_Guide/add_use_case.html#merge-the-pull-request-and-ensure-that-all-tests-pass.

DanielAdriaansen commented 3 months ago

@georgemccabe Could we change: POINT_STAT_MAX_DISTANCE_KM to POINT_STAT_UGRID_MAX_DIST_KM GRID_STAT_MAX_DISTANCE_KM to GRID_STAT_UGRID_MAX_DIST_KM

If you feel we should spell out DISTANCE, I don't have a problem with that just trying to keep them short. But I do think we should include UGRID in the conf item name.

georgemccabe commented 3 months ago

@DanielAdriaansen, I name the METplus config variables after the name of the corresponding MET config variable and tool so users can infer the names of the METplus config variables. It is easy to support both POINT_STAT_MAX_DISTANCE_KM and POINT_STAT_MAX_DIST_KM, but the variable name that matches the MET variable would take precedence if both are set.

DanielAdriaansen commented 3 months ago

@DanielAdriaansen, I name the METplus config variables after the name of the corresponding MET config variable and tool so users can infer the names of the METplus config variables. It is easy to support both POINT_STAT_MAX_DISTANCE_KM and POINT_STAT_MAX_DIST_KM, but the variable name that matches the MET variable would take precedence if both are set.

OK, and big 🤦 on my part, you already have UGRID in the conf items, despite me requesting incorrectly above. Thanks for getting it right even though I had it wrong! I think just using DISTANCE is good, no need to support DIST. Thanks!

dtcenter / METplus