From 771e32e4feb5906e1f23a96075962f1488b85e8d Mon Sep 17 00:00:00 2001 From: George McCabe <23407799+georgemccabe@users.noreply.github.com> Date: Wed, 31 May 2023 11:43:52 -0600 Subject: [PATCH 1/5] cleanup --- .../pytests/util/config_metplus/test_config_metplus.py | 3 +-- metplus/util/config_metplus.py | 8 +++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/tests/pytests/util/config_metplus/test_config_metplus.py b/internal/tests/pytests/util/config_metplus/test_config_metplus.py index 5b374fe011..8dda66dea1 100644 --- a/internal/tests/pytests/util/config_metplus/test_config_metplus.py +++ b/internal/tests/pytests/util/config_metplus/test_config_metplus.py @@ -907,8 +907,7 @@ def test_getraw_instance_with_unset_var(metplus_config): ] ) @pytest.mark.util -def test_format_var_items_options_semicolon(config_value, - expected_result): +def test_format_var_items_options_semicolon(config_value, expected_result): time_info = {} field_configs = {'name': 'FNAME', diff --git a/metplus/util/config_metplus.py b/metplus/util/config_metplus.py index 6d0fa4295b..92fb1754ba 100644 --- a/metplus/util/config_metplus.py +++ b/metplus/util/config_metplus.py @@ -1128,16 +1128,14 @@ def _format_var_items(field_configs, time_info=None): # perform string substitution on name if time_info: - search_name = do_string_sub(search_name, - skip_missing_tags=True, - **time_info) + search_name = do_string_sub(search_name, **time_info, + skip_missing_tags=True) var_items['name'] = search_name # get levels, performing string substitution on each item of list for level in getlist(field_configs.get('levels')): if time_info: - level = do_string_sub(level, - **time_info) + level = do_string_sub(level, **time_info) var_items['levels'].append(level) # if no levels are found, add an empty string From b314e0b75a9bc853b1bdd754e56b24f9837e2040 Mon Sep 17 00:00:00 2001 From: George McCabe <23407799+georgemccabe@users.noreply.github.com> Date: Wed, 31 May 2023 11:44:52 -0600 Subject: [PATCH 2/5] pass (optional) logger to threshold check functions to properly log an error instead of printing to console --- metplus/util/config_metplus.py | 8 +++++--- metplus/util/string_manip.py | 30 ++++++++++++++++-------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/metplus/util/config_metplus.py b/metplus/util/config_metplus.py index 92fb1754ba..61d0cad416 100644 --- a/metplus/util/config_metplus.py +++ b/metplus/util/config_metplus.py @@ -963,7 +963,8 @@ def parse_var_list(config, time_info=None, data_type=None, met_tool=None, index, search_prefixes) - field_info = _format_var_items(field_configs, time_info) + field_info = _format_var_items(field_configs, time_info, + config.logger) if not isinstance(field_info, dict): config.logger.error(f'Could not process {current_type}_' f'VAR{index} variables: {field_info}') @@ -1102,11 +1103,12 @@ def _find_var_name_indices(config, data_types, met_tool=None): return [int(index) for index in indices] -def _format_var_items(field_configs, time_info=None): +def _format_var_items(field_configs, time_info=None, logger=None): """! Substitute time information into field information and format values. @param field_configs dictionary with config variable names to read @param time_info dictionary containing time info for current run + @param logger (optional) logging object @returns dictionary containing name, levels, and output_names, as well as thresholds and extra options if found. If not enough information was set in the METplusConfig object, an empty @@ -1147,7 +1149,7 @@ def _format_var_items(field_configs, time_info=None): search_thresh = field_configs.get('thresh') if search_thresh: thresh = getlist(search_thresh) - if not validate_thresholds(thresh): + if not validate_thresholds(thresh, logger): return 'Invalid threshold supplied' var_items['thresh'] = thresh diff --git a/metplus/util/string_manip.py b/metplus/util/string_manip.py index da72ca9ca0..987e7cfff4 100644 --- a/metplus/util/string_manip.py +++ b/metplus/util/string_manip.py @@ -311,13 +311,11 @@ def camel_to_underscore(camel): def get_threshold_via_regex(thresh_string): """!Ensure thresh values start with >,>=,==,!=,<,<=,gt,ge,eq,ne,lt,le and then a number Optionally can have multiple comparison/number pairs separated with && or ||. - Args: - @param thresh_string: String to examine, i.e. <=3.4 - Returns: - None if string does not match any valid comparison operators or does - not contain a number afterwards - regex match object with comparison operator in group 1 and - number in group 2 if valid + + @param thresh_string: String to examine, i.e. <=3.4 + @returns None if string does not match any valid comparison operators + or does not contain a number afterwards. Regex match object with + comparison operator in group 1 and number in group 2 if valid """ comparison_number_list = [] @@ -359,14 +357,14 @@ def get_threshold_via_regex(thresh_string): return comparison_number_list -def validate_thresholds(thresh_list): +def validate_thresholds(thresh_list, logger=None): """ Checks list of thresholds to ensure all of them have the correct format Should be a comparison operator with number pair combined with || or && i.e. gt4 or >3&&<5 or gt3||lt1 - Args: - @param thresh_list list of strings to check - Returns: - True if all items in the list are valid format, False if not + + @param thresh_list list of strings to check + @param logger (optional) logging object to output error + @returns True if all items in the list are valid format, False if not """ valid = True for thresh in thresh_list: @@ -375,8 +373,12 @@ def validate_thresholds(thresh_list): valid = False if valid is False: - print("ERROR: Threshold values must use >,>=,==,!=,<,<=,gt,ge,eq,ne,lt, or le with a number, " - "optionally combined with && or ||") + err_str = ("Threshold values must use >,>=,==,!=,<,<=,gt,ge,eq,ne,lt, " + "or le with a number, optionally combined with && or ||") + if logger: + logger.error(err_str) + else: + print(f'ERROR: {err_str}') return False return True From 0698befb58937230bd32e9e93b31a9c6a22af7e4 Mon Sep 17 00:00:00 2001 From: George McCabe <23407799+georgemccabe@users.noreply.github.com> Date: Wed, 31 May 2023 11:46:33 -0600 Subject: [PATCH 3/5] per #2189, added unit test to recreate bug where spaces around && or || cause the threshold to be rejected as invalid --- .../tests/pytests/util/string_manip/test_util_string_manip.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/tests/pytests/util/string_manip/test_util_string_manip.py b/internal/tests/pytests/util/string_manip/test_util_string_manip.py index 22453a53fe..d53f19b469 100644 --- a/internal/tests/pytests/util/string_manip/test_util_string_manip.py +++ b/internal/tests/pytests/util/string_manip/test_util_string_manip.py @@ -128,6 +128,8 @@ def test_threshold(key, value): ("goSFP90", None), ("NA", [('NA', '')]), (" Date: Wed, 31 May 2023 11:47:19 -0600 Subject: [PATCH 4/5] per #2189, remove whitespace around threshold values to fix bug that rejects complex thresholds with whitespace around && or || --- metplus/util/string_manip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metplus/util/string_manip.py b/metplus/util/string_manip.py index 987e7cfff4..fd523c6ce2 100644 --- a/metplus/util/string_manip.py +++ b/metplus/util/string_manip.py @@ -322,7 +322,7 @@ def get_threshold_via_regex(thresh_string): # split thresh string by || or && thresh_split = re.split(r'\|\||&&', thresh_string) # check each threshold for validity - for thresh in thresh_split: + for thresh in [item.strip() for item in thresh_split]: found_match = False for comp in list(VALID_COMPARISONS)+list(VALID_COMPARISONS.values()): # if valid, add to list of tuples From 33485bf07e49e07a9775340709a9d056c1097b67 Mon Sep 17 00:00:00 2001 From: George McCabe <23407799+georgemccabe@users.noreply.github.com> Date: Wed, 31 May 2023 12:08:53 -0600 Subject: [PATCH 5/5] per #2189, added thresholds that demonstrate bug to the GridStat unit tests to ensure it is formatted properly --- .../pytests/wrappers/grid_stat/test_grid_stat_wrapper.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/tests/pytests/wrappers/grid_stat/test_grid_stat_wrapper.py b/internal/tests/pytests/wrappers/grid_stat/test_grid_stat_wrapper.py index 0b1416a2ea..71d65503a8 100644 --- a/internal/tests/pytests/wrappers/grid_stat/test_grid_stat_wrapper.py +++ b/internal/tests/pytests/wrappers/grid_stat/test_grid_stat_wrapper.py @@ -14,9 +14,10 @@ obs_name = 'APCP_03' obs_level_no_quotes = '(*,*)' obs_level = f'"{obs_level_no_quotes}"' -fcst_fmt = f'field = [{{ name="{fcst_name}"; level="{fcst_level}"; }}];' +both_thresh = ' lt-0.5,gt-0.5 && lt0.5,gt0.5 ' +fcst_fmt = f'field = [{{ name="{fcst_name}"; level="{fcst_level}"; cat_thresh=[{both_thresh}]; }}];' obs_fmt = (f'field = [{{ name="{obs_name}"; ' - f'level="{obs_level_no_quotes}"; }}];') + f'level="{obs_level_no_quotes}"; cat_thresh=[{both_thresh}]; }}];') time_fmt = '%Y%m%d%H' run_times = ['2005080700', '2005080712'] @@ -52,6 +53,7 @@ def set_minimum_config_settings(config): config.set('config', 'FCST_VAR1_LEVELS', fcst_level) config.set('config', 'OBS_VAR1_NAME', obs_name) config.set('config', 'OBS_VAR1_LEVELS', obs_level) + config.set('config', 'BOTH_VAR1_THRESH', both_thresh) @pytest.mark.parametrize(