Feature 2706 mvmode obs fcst input size

[davidalbo]

Expected Differences

• Do these changes introduce new tools, command line arguments, or configuration file options? [Yes]
  
  If yes, please describe: Config option multivar_intensity_flag was replaced with multivar_intensity_compare_fcst and multivar_intensity_compare_obs int 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.

• Review the source issue metadata (required labels, projects, and milestone).
• Complete the PR definition above.
• Ensure the PR title matches the feature or bugfix branch name.
• Define the PR metadata, as permissions allow.
  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
• After submitting the PR, select the ⚙️ 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.

[hertneky]

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.

[davidalbo]

@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.

[hertneky]

@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.

[JohnHalleyGotway]

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!

[JohnHalleyGotway]

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.