Feature 2706 mvmode obs fcst input size #2742
Conversation
Add documentation for new MvMODE config options/changes.
try to fix link
Fix link
There was a problem hiding this comment.
Reviewed config changes and Documentation. Thanks for working on this.
I also performed tests as follows:
- cntrl: Run as in the MvMODE usecase
- multivar_logic outside/inside dictionaries - if outside and field arrays not equal, produces a meaningful error msg
-multivar_logic within field dictionaries, for the obs dictionary I had an array of 3 variables but included only 2 in the logic (multivar_logic = "1 && 2";) - produced expected error of expecting 3 args, got 2
-multivar_intensity_compare_fcst/obs empty - produces expected results of no intensity info and use of multivar_name/_level
-merge_flag = ENGINE/BOTH produced expected error message - not implemented for mvmode
-included multiple convolution thresh/radii and received the expected error - not implemented for mvmode
-regridding to both FCST/OBS regrids to the expected 1st field of the respective array - also ran a test regridding to grid from a file path which was good. - Percentile thresholding ran. Compared a run with and w/o requesting intensities. Object counts and size are the same. I don't see any red flags here.
|
@hertneky another pathological test I happened to do that gave a good message... setting a conv_thresh to a percentile, but setting the merge_thresh to a simple comparison. The values I happened to use produced simple objects that were not contained in merge objects, and it gave a warning. I'd expect the same warning if one were to set conv/merge thresholds 'backwards' so that merge objects are smaller than simple objects. |
|
@davidalbo Good point. I did not test a smaller merge thresh than the conv_thresh. I just did now and see the meaningful error message. |
There was a problem hiding this comment.
I scrolled through the code changes with an eye mostly toward formatting. I made a bunch of exceedingly minor proposed changes. All but 2 are just whitespace changes and deleting commented-out code with the potential to cause confusion in the future.
2 suggestions are to use "na_str" instead of "Not set". We don't ever want to include embedded whitespace in strings that could be written to the ascii output files. Embedded whitespace will break downstream parsing logic. I did not actually try compiling with the na_str change, but we could let the GitHub tests tell us if it's a problem.
I'll click the Request changes option since I want to make sure that it compiles with the change to na_str. But other than that exceedingly minor point, things are looking good.
Thanks for all your work on this!
…using the na_str variable instead.
There was a problem hiding this comment.
I approve of these changes.
I did check to confirm that MET does compile without error in this GHA run. So replacing "Not set" with na_str works fine.
I checked out the branch, replaced the other two instance of "Not set" and pushed those changes.
Expected Differences
Do these changes introduce new tools, command line arguments, or configuration file options? [Yes]
If yes, please describe: Config option
multivar_intensity_flagwas replaced withmultivar_intensity_compare_fcstandmultivar_intensity_compare_obsint arrays. Also, now the fcst and obs field arrays can be of different lengths.Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]
If yes, please describe:
Pull Request Testing
Describe testing already performed for these changes: Tested extensively on a single case, changing number of obs compared to number of forecasts. Some perc_thresh settings were tried out as well. Compared the previous version to this one (with equal number of forecast and obs inputs) and got identical results.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions: If accessible, you can try seneca:/d1/personal/dave/mvmode_test and look at run-multi.bsh for some examples.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Do these changes include sufficient testing updates? [No] We want to add more unit tests, but those will come in a new issue.
Will this PR result in changes to the test suite? [No]
If yes, describe the new output and/or changes to the existing output:
Please complete this pull request review by [November 17].
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: Coordinated METplus-X.Y Support project for bugfix releases or MET-X.Y.Z Development project for official releases