Feature #2586 GenVxMask improvements (#2833)

* resolve some SonarQube complaints

* per #2586, added function with tests to properly parse list of command line arguments that can now contain comma-separated lists that should not be split up into separate items

* add support for {app}_{data_type}_FILE_WINDOW_BEGIN/END, e.g. GEN_VX_MASK_OBS_FILE_WINDOW_BEGIN. This just adds support for an additional variation of the config variable names

* add support for an empty label for input templates

* update wrapper to be consistent with other wrappers wrt finding input files, progress towards #2492. Allow file window range to be specified separately for mask and input files. Other cleanup to move towards consistent wrappers with fewer wrapper-specific overrides of functions like get_command

* update unit tests to align with changes for #2492

* add documentation for config variables that are newly supported to allow file window range to be specified separately for mask and input files

* renamed GEN_VX_MASK_OBS variables to be GEN_VX_MASK_INPUT as suggested by @JohnHalleyGotway in PR review

* fix logic to properly read input files by handling inputs that support multiple inputs with labels (used by GridDiag and UserScript wrappers) and typical inputs (all other wrappers). Prior to this change only input templates that have the FCST or OBS identifier were read properly via get_input_templates

docs/Users_Guide/glossary.rst

@@ -4362,12 +4362,58 @@ METplus Configuration Glossary
      | *Used by:* GenVxMask
 
    GEN_VX_MASK_FILE_WINDOW_BEGIN
-     Used to control the lower bound of the window around the valid time to determine if a GenVxMask input file should be used for processing. Overrides :term:`FILE_WINDOW_BEGIN`. See 'Use Windows to Find Valid Files' section for more information.
+     Used to control the lower bound of the window around the valid time to determine if a GenVxMask input file should
+     be used for processing. Applies to both the input file and the mask file(s).
+     Set :term:`GEN_VX_MASK_INPUT_FILE_WINDOW_BEGIN` or
+     :term:`GEN_VX_MASK_MASK_FILE_WINDOW_BEGIN` to set them separately.
+     Overrides :term:`FILE_WINDOW_BEGIN`.
+     See 'Use Windows to Find Valid Files' section for more information.
 
      | *Used by:* GenVxMask
 
    GEN_VX_MASK_FILE_WINDOW_END
-     Used to control the upper bound of the window around the valid time to determine if an GenVxMask input file should be used for processing. Overrides :term:`FILE_WINDOW_BEGIN`. See 'Use Windows to Find Valid Files' section for more information.
+     Used to control the upper bound of the window around the valid time to determine if an GenVxMask input file should
+     be used for processing. Applies to both the input file and the mask file(s).
+     Set :term:`GEN_VX_MASK_INPUT_FILE_WINDOW_END` or
+     :term:`GEN_VX_MASK_MASK_FILE_WINDOW_END` to set them separately.
+     Overrides :term:`FILE_WINDOW_END`.
+     See 'Use Windows to Find Valid Files' section for more information.
+
+     | *Used by:* GenVxMask
+
+   GEN_VX_MASK_INPUT_FILE_WINDOW_BEGIN
+     Used to control the lower bound of the window around the valid time to determine if a GenVxMask input file should
+     be used for processing. Applies to the input file only (not the mask file).
+     Set :term:`GEN_VX_MASK_FILE_WINDOW_BEGIN` to control both input and mask.
+     Overrides :term:`FILE_WINDOW_BEGIN`.
+     See 'Use Windows to Find Valid Files' section for more information.
+
+     | *Used by:* GenVxMask
+
+   GEN_VX_MASK_INPUT_FILE_WINDOW_END
+     Used to control the upper bound of the window around the valid time to determine if a GenVxMask input file should
+     be used for processing. Applies to the input file only (not the mask file).
+     Set :term:`GEN_VX_MASK_FILE_WINDOW_END` to control both input and mask.
+     Overrides :term:`FILE_WINDOW_END`.
+     See 'Use Windows to Find Valid Files' section for more information.
+
+     | *Used by:* GenVxMask
+
+   GEN_VX_MASK_MASK_FILE_WINDOW_BEGIN
+     Used to control the lower bound of the window around the valid time to determine if a GenVxMask mask file should
+     be used for processing. Applies to the mask file only (not the input file).
+     Set :term:`GEN_VX_MASK_FILE_WINDOW_BEGIN` to control both input and mask.
+     Overrides :term:`FILE_WINDOW_BEGIN`.
+     See 'Use Windows to Find Valid Files' section for more information.
+
+     | *Used by:* GenVxMask
+
+   GEN_VX_MASK_MASK_FILE_WINDOW_END
+     Used to control the upper bound of the window around the valid time to determine if a GenVxMask mask file should
+     be used for processing. Applies to the mask file only (not the input file).
+     Set :term:`GEN_VX_MASK_FILE_WINDOW_END` to control both input and mask.
+     Overrides :term:`FILE_WINDOW_END`.
+     See 'Use Windows to Find Valid Files' section for more information.
 
      | *Used by:* GenVxMask
 

docs/Users_Guide/wrappers.rst

@@ -1749,6 +1749,10 @@ Configuration
 | :term:`GEN_VX_MASK_CUSTOM_LOOP_LIST`
 | :term:`GEN_VX_MASK_FILE_WINDOW_BEGIN`
 | :term:`GEN_VX_MASK_FILE_WINDOW_END`
+| :term:`GEN_VX_MASK_INPUT_FILE_WINDOW_BEGIN`
+| :term:`GEN_VX_MASK_INPUT_FILE_WINDOW_END`
+| :term:`GEN_VX_MASK_MASK_FILE_WINDOW_BEGIN`
+| :term:`GEN_VX_MASK_MASK_FILE_WINDOW_END`
 | :term:`GEN_VX_MASK_SKIP_VALID_TIMES`
 | :term:`GEN_VX_MASK_INC_VALID_TIMES`
 | :term:`GEN_VX_MASK_SKIP_INIT_TIMES`

internal/tests/pytests/util/config_metplus/test_config_metplus.py

@@ -68,64 +68,6 @@ def test_get_default_config_list():
     assert actual_both == expected_both
 
 
-@pytest.mark.parametrize(
-    'regex,index,id,expected_result', [
-        # 0: No ID
-        (r'^FCST_VAR(\d+)_NAME$', 1, None,
-         {'1': [None],
-          '2': [None],
-          '4': [None]}),
-        # 1: ID and index 2
-        (r'(\w+)_VAR(\d+)_NAME', 2, 1,
-         {'1': ['FCST'],
-          '2': ['FCST'],
-          '4': ['FCST']}),
-        # 2: index 1, ID 2, multiple identifiers
-        (r'^FCST_VAR(\d+)_(\w+)$', 1, 2,
-         {'1': ['NAME', 'LEVELS'],
-          '2': ['NAME'],
-          '4': ['NAME']}),
-        # 3: command that StatAnalysis wrapper uses
-        (r'MODEL(\d+)$', 1, None,
-         {'1': [None],
-          '2': [None],}),
-        # 4: TCPairs conensus logic
-        (r'^TC_PAIRS_CONSENSUS(\d+)_(\w+)$', 1, 2,
-         {'1': ['NAME', 'MEMBERS', 'REQUIRED', 'MIN_REQ'],
-          '2': ['NAME', 'MEMBERS', 'REQUIRED', 'MIN_REQ']}),
-    ]
-)
-@pytest.mark.util
-def test_find_indices_in_config_section(metplus_config, regex, index,
-                                        id, expected_result):
-    config = metplus_config
-    config.set('config', 'FCST_VAR1_NAME', 'name1')
-    config.set('config', 'FCST_VAR1_LEVELS', 'level1')
-    config.set('config', 'FCST_VAR2_NAME', 'name2')
-    config.set('config', 'FCST_VAR4_NAME', 'name4')
-    config.set('config', 'MODEL1', 'model1')
-    config.set('config', 'MODEL2', 'model2')
-
-    config.set('config', 'TC_PAIRS_CONSENSUS1_NAME', 'name1')
-    config.set('config', 'TC_PAIRS_CONSENSUS1_MEMBERS', 'member1')
-    config.set('config', 'TC_PAIRS_CONSENSUS1_REQUIRED', 'True')
-    config.set('config', 'TC_PAIRS_CONSENSUS1_MIN_REQ', '1')
-    config.set('config', 'TC_PAIRS_CONSENSUS2_NAME', 'name2')
-    config.set('config', 'TC_PAIRS_CONSENSUS2_MEMBERS', 'member2')
-    config.set('config', 'TC_PAIRS_CONSENSUS2_REQUIRED', 'True')
-    config.set('config', 'TC_PAIRS_CONSENSUS2_MIN_REQ', '2')
-
-    indices = config_metplus.find_indices_in_config_section(regex, config,
-                                                            index_index=index,
-                                                            id_index=id)
-
-    pp = pprint.PrettyPrinter()
-    print(f'Indices:')
-    pp.pprint(indices)
-
-    assert indices == expected_result
-
-
 @pytest.mark.parametrize(
     'config_var_name, expected_indices, set_met_tool', [
         ('FCST_GRID_STAT_VAR1_NAME', [1], True),
@@ -409,14 +351,14 @@ def test_parse_var_list_both(metplus_config, data_type, list_created):
     var_list = config_metplus.parse_var_list(conf, time_info=None, data_type=data_type)
     print(f'var_list:{var_list}')
     for list_to_check in list_created.split(':'):
-        if (not var_list[0][f'{list_to_check}_name'] == "NAME1" or
-                not var_list[1][f'{list_to_check}_name'] == "NAME1" or
-                not var_list[2][f'{list_to_check}_name'] == "NAME2" or
-                not var_list[3][f'{list_to_check}_name'] == "NAME2" or
-                not var_list[0][f'{list_to_check}_level'] == "LEVELS11" or
-                not var_list[1][f'{list_to_check}_level'] == "LEVELS12" or
-                not var_list[2][f'{list_to_check}_level'] == "LEVELS21" or
-                not var_list[3][f'{list_to_check}_level'] == "LEVELS22"):
+        if (var_list[0][f'{list_to_check}_name'] != "NAME1" or
+                var_list[1][f'{list_to_check}_name'] != "NAME1" or
+                var_list[2][f'{list_to_check}_name'] != "NAME2" or
+                var_list[3][f'{list_to_check}_name'] != "NAME2" or
+                var_list[0][f'{list_to_check}_level'] != "LEVELS11" or
+                var_list[1][f'{list_to_check}_level'] != "LEVELS12" or
+                var_list[2][f'{list_to_check}_level'] != "LEVELS21" or
+                var_list[3][f'{list_to_check}_level'] != "LEVELS22"):
             assert False
 
 
@@ -517,8 +459,6 @@ def test_parse_var_list_fcst_and_obs_and_both(metplus_config, data_type, list_le
             if expect[f'{dt_lower}_level'] != reality[f'{dt_lower}_level']:
                 assert False
 
-        assert True
-
 
 # option defined in obs only
 @pytest.mark.parametrize(
@@ -646,9 +586,9 @@ def test_parse_var_list_ensemble(metplus_config):
                                    met_tool='ensemble_stat')
 
     pp = pprint.PrettyPrinter()
-    print(f'ENSEMBLE_VAR_LIST:')
+    print('ENSEMBLE_VAR_LIST:')
     pp.pprint(ensemble_var_list)
-    print(f'VAR_LIST:')
+    print('VAR_LIST:')
     pp.pprint(var_list)
 
     assert(len(ensemble_var_list) == len(expected_ens_list))
@@ -715,9 +655,9 @@ def test_parse_var_list_series_by(metplus_config):
                                          met_tool='series_analysis')
 
     pp = pprint.PrettyPrinter()
-    print(f'ExtractTiles var list:')
+    print('ExtractTiles var list:')
     pp.pprint(actual_et_list)
-    print(f'SeriesAnalysis var list:')
+    print('SeriesAnalysis var list:')
     pp.pprint(actual_sa_list)
 
     assert len(actual_et_list) == len(expected_et_list)
@@ -898,7 +838,6 @@ def test_getraw_instance_with_unset_var(metplus_config):
     """! Replicates bug where CURRENT_FCST_NAME is substituted with
      an empty string when copied from an instance section
      """
-    pytest.skip()
     instance = 'my_section'
     config = metplus_config
     config.set('config', 'MODEL', 'FCST')

internal/tests/pytests/util/string_manip/test_util_string_manip.py

@@ -5,7 +5,8 @@
 import pprint
 from datetime import datetime
 
-from metplus.util.string_manip import *
+from metplus.util.string_manip import get_log_path, get_logfile_info, template_to_regex, subset_list, expand_int_string_to_list, round_0p5, validate_thresholds, get_threshold_via_regex, camel_to_underscore, remove_quotes, getlist, getlistint, list_to_str, comparison_to_letter_format, format_thresh, format_level, find_indices_in_config_section
+
 
 
 @pytest.mark.parametrize(
@@ -373,7 +374,7 @@ def test_getlist_begin_end_incr(list_string, output_list):
 
 
 @pytest.mark.parametrize(
-    'input, add_quotes, expected_output', [
+    'input_list, add_quotes, expected_output', [
         (['a', 'b', 'c'], None, '"a", "b", "c"'),
         (['0', '1', '2'], None, '"0", "1", "2"'),
         (['a', 'b', 'c'], True, '"a", "b", "c"'),
@@ -385,11 +386,11 @@ def test_getlist_begin_end_incr(list_string, output_list):
     ]
 )
 @pytest.mark.util
-def test_list_to_str(input, add_quotes, expected_output):
+def test_list_to_str(input_list, add_quotes, expected_output):
     if add_quotes is None:
-        assert list_to_str(input) == expected_output
+        assert list_to_str(input_list) == expected_output
     else:
-        assert list_to_str(input, add_quotes=add_quotes) == expected_output
+        assert list_to_str(input_list, add_quotes=add_quotes) == expected_output
 
 
 @pytest.mark.parametrize(
@@ -440,7 +441,7 @@ def test_format_level(level, expected_result):
 
 
 @pytest.mark.parametrize(
-    'regex,index,id,expected_result', [
+    'regex,index,id_index,expected_result', [
         # 0: No ID
         (r'^FCST_VAR(\d+)_NAME$', 1, None,
          {'1': [None],
@@ -468,7 +469,7 @@ def test_format_level(level, expected_result):
 )
 @pytest.mark.util
 def test_find_indices_in_config_section(metplus_config, regex, index,
-                                        id, expected_result):
+                                        id_index, expected_result):
     config = metplus_config
     config.set('config', 'FCST_VAR1_NAME', 'name1')
     config.set('config', 'FCST_VAR1_LEVELS', 'level1')
@@ -487,10 +488,10 @@ def test_find_indices_in_config_section(metplus_config, regex, index,
     config.set('config', 'TC_PAIRS_CONSENSUS2_MIN_REQ', '2')
 
     indices = find_indices_in_config_section(regex, config, index_index=index,
-                                             id_index=id)
+                                             id_index=id_index)
 
     pp = pprint.PrettyPrinter()
-    print(f'Indices:')
+    print('Indices:')
     pp.pprint(indices)
 
     assert indices == expected_result

internal/tests/pytests/wrappers/gen_vx_mask/test_gen_vx_mask.py

@@ -20,6 +20,34 @@ def gen_vx_mask_wrapper(metplus_config):
     config.set('config', 'DO_NOT_RUN_EXE', True)
     return GenVxMaskWrapper(config)
 
+@pytest.mark.parametrize(
+    'input_val, expected',
+    [
+        # no input returns a list with an empty string
+        ('', [''] ),
+        # two sets of options as demonstrated in GenVxMask_multiple.conf
+        ("-type lat -thresh 'ge30&&le50', -type lon -thresh 'le-70&&ge-130' -intersection -name lat_lon_mask",
+         ["-type lat -thresh 'ge30&&le50'",
+          "-type lon -thresh 'le-70&&ge-130' -intersection -name lat_lon_mask"] ),
+        # first item in list must be a flag starting with -, e.g. -type
+        ("type lat -thresh 'ge30&&le50'",
+         ["error"] ),
+        # complex single set of options that includes multiple values for -type/-thresh/etc as recently added in dtcenter/MET#3008
+        ('-type data,data,lat,lon -mask_field \'name="LAND"; level="L0";\' -mask_field \'name="TMP"; level="L0";\' -thresh eq1,lt273,gt0,lt0 -intersection -v 5',
+         ['-type data,data,lat,lon -mask_field \'name="LAND"; level="L0";\' -mask_field \'name="TMP"; level="L0";\' -thresh eq1,lt273,gt0,lt0 -intersection -v 5'] ),
+        # two sets of options with one of them containing multiple values for -type/-thresh/etc
+        ('-type data -mask_field \'name="LAND"; level="L0";\' -thresh eq1, -type data,lat,lon -mask_field \'name="TMP"; level="L0";\' -thresh lt273,gt0,lt0 -intersection -v 5',
+         ['-type data -mask_field \'name="LAND"; level="L0";\' -thresh eq1',
+          '-type data,lat,lon -mask_field \'name="TMP"; level="L0";\' -thresh lt273,gt0,lt0 -intersection -v 5']),
+    ]
+)
+@pytest.mark.wrapper
+def test_handle_command_options(metplus_config, input_val, expected):
+    config = metplus_config
+    wrapper = GenVxMaskWrapper(config)
+    actual = wrapper.parse_command_options_list(input_val)
+    assert actual == expected
+
 
 @pytest.mark.parametrize(
     'missing, run, thresh, errors, allow_missing', [
@@ -78,9 +106,8 @@ def test_run_gen_vx_mask_once(metplus_config):
                                              'GenVxMask_test')
     wrap.c_dict['OUTPUT_TEMPLATE'] = '{valid?fmt=%Y%m%d%H}_ZENITH_LAT_MASK.nc'
     wrap.c_dict['COMMAND_OPTIONS'] = ["-type lat -thresh 'ge30&&le50'"]
-#    wrap.c_dict['MASK_INPUT_TEMPLATES'] = ['LAT', 'LON']
-#    wrap.c_dict['COMMAND_OPTIONS'] = ["-type lat -thresh 'ge30&&le50'", "-type lon -thresh 'le-70&&ge-130' -intersection"]
 
+    wrap.c_dict['ALL_FILES'] = wrap.get_all_files_for_each(time_info)
     wrap.run_at_time_once(time_info)
 
     expected_cmd = f"{wrap.app_path} \"2018020100_ZENITH\" LAT {wrap.config.getdir('OUTPUT_BASE')}/GenVxMask_test/2018020100_ZENITH_LAT_MASK.nc -type lat -thresh 'ge30&&le50' -v 2"
@@ -96,16 +123,22 @@ def test_run_gen_vx_mask_twice(metplus_config):
     input_dict = {'valid': datetime.datetime.strptime("201802010000",'%Y%m%d%H%M'),
                   'lead': 0}
     time_info = time_util.ti_calculate(input_dict)
+    cmd_args = [
+        "-type lat -thresh 'ge30&&le50'",
+        "-type lon -thresh 'le-70&&ge-130' -intersection -name lat_lon_mask"
+    ]
 
-    wrap = gen_vx_mask_wrapper(metplus_config)
-    wrap.c_dict['INPUT_TEMPLATE'] = '{valid?fmt=%Y%m%d%H}_ZENITH'
-    wrap.c_dict['MASK_INPUT_TEMPLATES'] = ['LAT', 'LON']
-    wrap.c_dict['OUTPUT_DIR'] = os.path.join(wrap.config.getdir('OUTPUT_BASE'),
-                                             'GenVxMask_test')
-    wrap.c_dict['OUTPUT_TEMPLATE'] = '{valid?fmt=%Y%m%d%H}_ZENITH_LAT_LON_MASK.nc'
-    cmd_args = ["-type lat -thresh 'ge30&&le50'", "-type lon -thresh 'le-70&&ge-130' -intersection -name lat_lon_mask"]
-    wrap.c_dict['COMMAND_OPTIONS'] = cmd_args
+    config = metplus_config
+
+    output_dir = os.path.join(config.getdir('OUTPUT_BASE'), 'GenVxMask_test')
+    config.set('config', 'GEN_VX_MASK_INPUT_TEMPLATE', '{valid?fmt=%Y%m%d%H}_ZENITH')
+    config.set('config', 'GEN_VX_MASK_INPUT_MASK_TEMPLATE', 'LAT, LON')
+    config.set('config', 'GEN_VX_MASK_OUTPUT_DIR', output_dir)
+    config.set('config', 'GEN_VX_MASK_OUTPUT_TEMPLATE', '{valid?fmt=%Y%m%d%H}_ZENITH_LAT_LON_MASK.nc')
+    config.set('config', 'GEN_VX_MASK_OPTIONS', ','.join(cmd_args))
+    wrap = gen_vx_mask_wrapper(config)
 
+    wrap.c_dict['ALL_FILES'] = wrap.get_all_files_for_each(time_info)
     wrap.run_at_time_once(time_info)
 
     expected_cmds = [f"{wrap.app_path} \"2018020100_ZENITH\" LAT {wrap.config.getdir('OUTPUT_BASE')}/stage/gen_vx_mask/temp_0.nc {cmd_args[0]} -v 2",