From 0c7307a221e94aecfcb4507a168dc3b85ccc2a3c Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Tue, 28 Nov 2023 17:07:26 -0800 Subject: [PATCH 01/31] Remove default diags running with only Python API --- e3sm_diags/e3sm_diags_driver.py | 3 +- e3sm_diags/parameter/core_parameter.py | 24 +------- e3sm_diags/parser/core_parser.py | 8 +-- e3sm_diags/run.py | 80 ++++++++++++-------------- e3sm_diags/viewer/main.py | 4 +- 5 files changed, 45 insertions(+), 74 deletions(-) diff --git a/e3sm_diags/e3sm_diags_driver.py b/e3sm_diags/e3sm_diags_driver.py index 111d20f0d..f99d6422b 100644 --- a/e3sm_diags/e3sm_diags_driver.py +++ b/e3sm_diags/e3sm_diags_driver.py @@ -354,8 +354,9 @@ def main(parameters=[]): # If no parameters are passed, use the parser args as defaults. Otherwise, # create the dictionary of expected parameters. - if not parameters: + if len(parameters) == 0: parameters = get_parameters(parser) + expected_parameters = create_parameter_dict(parameters) if not os.path.exists(parameters[0].results_dir): diff --git a/e3sm_diags/parameter/core_parameter.py b/e3sm_diags/parameter/core_parameter.py index 4b96c13ab..6b677a38f 100644 --- a/e3sm_diags/parameter/core_parameter.py +++ b/e3sm_diags/parameter/core_parameter.py @@ -58,28 +58,8 @@ def __init__(self): # 'model_vs_obs' (by default), 'model_vs_model', or 'obs_vs_obs'. self.run_type: str = "model_vs_obs" - # A list of the sets to be run. Default is all sets - self.sets: List[str] = [ - "zonal_mean_xy", - "zonal_mean_2d", - "zonal_mean_2d_stratosphere", - "meridional_mean_2d", - "lat_lon", - "polar", - "area_mean_time_series", - "cosp_histogram", - "enso_diags", - "qbo", - "streamflow", - "diurnal_cycle", - "arm_diags", - "tc_analysis", - "annual_cycle_zonal_mean", - "lat_lon_land", - "lat_lon_river", - "aerosol_aeronet", - "aerosol_budget", - ] + # A list of the sets to be run. + self.sets: List[str] = [] # The current set that is being ran when looping over sets in # `e3sm_diags_driver.run_diag()`. diff --git a/e3sm_diags/parser/core_parser.py b/e3sm_diags/parser/core_parser.py index d823de253..72f168f4a 100644 --- a/e3sm_diags/parser/core_parser.py +++ b/e3sm_diags/parser/core_parser.py @@ -784,8 +784,7 @@ def get_cfg_parameters( RuntimeError If the parameters input file is not `.json` or `.cfg` format. """ - - parameters = [] + params = [] self._parse_arguments() @@ -801,10 +800,7 @@ def get_cfg_parameters( else: raise RuntimeError("The parameters input file must be a .cfg file") - for p in params: - parameters.append(p) - - return parameters + return params def _get_cfg_parameters( self, cfg_file, check_values=False, argparse_vals_only=True diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index d7a6ba870..86e018e05 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -1,7 +1,9 @@ import copy +from itertools import chain +from typing import List, Union import e3sm_diags # noqa: F401 -from e3sm_diags.e3sm_diags_driver import get_default_diags_path, main +from e3sm_diags.e3sm_diags_driver import main from e3sm_diags.logger import custom_logger, move_log_to_prov_dir from e3sm_diags.parameter import SET_TO_PARAMETERS from e3sm_diags.parameter.core_parameter import CoreParameter @@ -18,7 +20,6 @@ class Run: """ def __init__(self): - self.sets_to_run = CoreParameter().sets self.parser = CoreParser() def run_diags(self, parameters): @@ -48,46 +49,47 @@ def get_final_parameters(self, parameters): # For each of the passed in parameters, we can only have one of # each type. - types = set([p.__class__ for p in parameters]) - if len(types) != len(parameters): - msg = "You passed in two or more parameters of the same type." + param_types_list = [ + p.__class__ for p in parameters if p.__class__ != CoreParameter + ] + param_types_set = set(param_types_list) + + if len(param_types_set) != len(param_types_list): + msg = ( + "You passed in two or more non-CoreParameter objects of the same type." + ) raise RuntimeError(msg) self._add_parent_attrs_to_children(parameters) - final_params = [] + # Get the sets to run using the parameter objects via the Python API + sets_to_run = [param.sets for param in parameters] + self.sets_to_run = list(chain.from_iterable(sets_to_run)) + api_params = [] for set_name in self.sets_to_run: - other_params = self._get_other_diags(parameters[0].run_type) - # For each of the set_names, get the corresponding parameter. - param = self._get_instance_of_param_class( + api_param = self._get_instance_of_param_class( SET_TO_PARAMETERS[set_name], parameters ) # Since each parameter will have lots of default values, we want to remove them. # Otherwise when calling get_parameters(), these default values # will take precedence over values defined in other_params. - self._remove_attrs_with_default_values(param) - param.sets = [set_name] - - params = self.parser.get_parameters( - orig_parameters=param, - other_parameters=other_params, - cmd_default_vars=False, - argparse_vals_only=False, - ) + self._remove_attrs_with_default_values(api_param) + api_param.sets = [set_name] + + # Makes sure that any parameters that are selectors will be in param. + self._add_attrs_with_default_values(api_param) - # Makes sure that any parameters that are selectors - # will be in param. - self._add_attrs_with_default_values(param) - # The select() call in get_parameters() was made for the original - # command-line way of using CDP. - # We just call it manually with the parameter object param. - params = self.parser.select(param, params) + api_params.append(api_param) - final_params.extend(params) + # Get the diagnostic parameter objects from the .cfg file passed via + # the -d/--diags CLI argument (if set). Otherwise, it will be an empty + # list. + cfg_params = self._get_diags_from_cfg_file() + final_params = api_params + cfg_params self.parser.check_values_of_params(final_params) return final_params @@ -233,29 +235,21 @@ def _get_instance_of_param_class(self, cls, parameters): msg = "There's weren't any class of types {} in your parameters." raise RuntimeError(msg.format(class_types)) - def _get_other_diags(self, run_type): + def _get_diags_from_cfg_file(self) -> Union[List, List[CoreParameter]]: """ - If the user has ran the script with a -d, get the diags for that. - If not, load the default diags based on sets_to_run and run_type. + Get parameters defined by the cfg file passed to -d/--diags (if set). + + If ``-d`` is not passed, return an empty list. """ args = self.parser.view_args() + params = [] - # If the user has passed in args with -d. if args.other_parameters: params = self.parser.get_cfg_parameters(argparse_vals_only=False) - else: - default_diags_paths = [ - get_default_diags_path(set_name, run_type, False) - for set_name in self.sets_to_run - ] - params = self.parser.get_cfg_parameters( - files_to_open=default_diags_paths, argparse_vals_only=False - ) - - # For each of the params, add in the default values - # using the parameter classes in SET_TO_PARAMETERS. - for i in range(len(params)): - params[i] = SET_TO_PARAMETERS[params[i].sets[0]]() + params[i] + # For each of the params, add in the default values + # using the parameter classes in SET_TO_PARAMETERS. + for i in range(len(params)): + params[i] = SET_TO_PARAMETERS[params[i].sets[0]]() + params[i] return params diff --git a/e3sm_diags/viewer/main.py b/e3sm_diags/viewer/main.py index 4227b3e1e..23b6c786b 100644 --- a/e3sm_diags/viewer/main.py +++ b/e3sm_diags/viewer/main.py @@ -113,8 +113,8 @@ def insert_data_in_row(row_obj, name, url): def create_viewer(root_dir, parameters): """ - Based of the parameters, find the files with the - certain extension and create the viewer in root_dir. + Based of the parameters, find the files with the certain extension and + create the viewer in root_dir. """ # Group each parameter object based on the `sets` parameter. set_to_parameters = collections.defaultdict(list) From 00865fea88db1cc1a41314d3e262879edd2776b0 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 13:25:54 -0800 Subject: [PATCH 02/31] Clean up `test_all_sets.py` and `test_diags.py` - Make both files run each diagnostic parameter object one at a time - Move test utilities to `utils.py` - Fix paths in `all_sets_modified.cfg` - Update `Run.run_diags()` and `main.main()` to return parameter objects with their results after running diagnostics --- e3sm_diags/e3sm_diags_driver.py | 4 +- e3sm_diags/run.py | 13 +- tests/complete_run.py | 2 +- tests/integration/all_sets.py | 26 +- tests/integration/all_sets_modified.cfg | 26 +- tests/integration/test_all_sets.py | 117 --------- ..._diags.py => test_all_sets_image_diffs.py} | 226 +++++++++--------- .../test_all_sets_modified_image_counts.py | 42 ++++ tests/integration/utils.py | 80 +++++++ 9 files changed, 267 insertions(+), 269 deletions(-) delete mode 100644 tests/integration/test_all_sets.py rename tests/integration/{test_diags.py => test_all_sets_image_diffs.py} (95%) create mode 100644 tests/integration/test_all_sets_modified_image_counts.py diff --git a/e3sm_diags/e3sm_diags_driver.py b/e3sm_diags/e3sm_diags_driver.py index f99d6422b..2c5336102 100644 --- a/e3sm_diags/e3sm_diags_driver.py +++ b/e3sm_diags/e3sm_diags_driver.py @@ -347,7 +347,7 @@ def _collapse_results(parameters: List[List[CoreParameter]]) -> List[CoreParamet return output_parameters -def main(parameters=[]): +def main(parameters=[]) -> List[CoreParameter]: # Get the diagnostic run parameters # --------------------------------- parser = CoreParser() @@ -409,6 +409,8 @@ def main(parameters=[]): ) raise Exception(message) + return parameters_results + if __name__ == "__main__": main() diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 86e018e05..35ab29808 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -33,10 +33,13 @@ def run_diags(self, parameters): raise RuntimeError(msg) try: - main(final_params) + parameter_results = main(final_params) except Exception: logger.exception("Error traceback:", exc_info=True) - move_log_to_prov_dir(final_params[0].results_dir) + + move_log_to_prov_dir(parameter_results[0].results_dir) + + return parameter_results def get_final_parameters(self, parameters): """ @@ -60,6 +63,12 @@ def get_final_parameters(self, parameters): ) raise RuntimeError(msg) + # FIXME: This line produces some unintended side-effects. For example, + # let's say we have two objects: 1. CoreParameter, 2. ZonalMean2DParameter. + # If object 1 has `plevs=[200]`, this value will get copied to object 2. + # Object 2 has a check to make sure plevs has more than 1 value. This + # breaks the diagnostic run as a result. The workaround is to loop + # over `run_diags()` function and run one parameter at a time. self._add_parent_attrs_to_children(parameters) # Get the sets to run using the parameter objects via the Python API diff --git a/tests/complete_run.py b/tests/complete_run.py index 00e490615..1d64f7339 100644 --- a/tests/complete_run.py +++ b/tests/complete_run.py @@ -12,7 +12,7 @@ # This test should be run with the latest E3SM Diags tutorial code. from examples.run_v2_6_0_all_sets_E3SM_machines import run_lcrc -from tests.integration.test_diags import _compare_images +from tests.integration.test_all_sets_image_diffs import _compare_images class TestCompleteRun: diff --git a/tests/integration/all_sets.py b/tests/integration/all_sets.py index 943de7f89..f3f5b5f6a 100644 --- a/tests/integration/all_sets.py +++ b/tests/integration/all_sets.py @@ -1,30 +1,10 @@ # Running the software with the API: # python all_sets.py -d all_sets.py -from e3sm_diags.parameter.area_mean_time_series_parameter import ( - AreaMeanTimeSeriesParameter, -) -from e3sm_diags.parameter.core_parameter import CoreParameter -from e3sm_diags.parameter.enso_diags_parameter import EnsoDiagsParameter -from e3sm_diags.parameter.meridional_mean_2d_parameter import MeridionalMean2dParameter -from e3sm_diags.parameter.zonal_mean_2d_parameter import ZonalMean2dParameter from e3sm_diags.run import Run +from tests.integration.utils import _get_test_params run_object = Run() -param = CoreParameter() -ts_param = AreaMeanTimeSeriesParameter() -m2d_param = MeridionalMean2dParameter() -m2d_param.plevs = [ - 200.0, - 500.0, -] -z2d_param = ZonalMean2dParameter() -z2d_param.plevs = [ - 200.0, - 300.0, -] +params = _get_test_params() -enso_param = EnsoDiagsParameter() -enso_param.test_name = "e3sm_v1" - -run_object.run_diags([param, ts_param, m2d_param, z2d_param, enso_param]) +run_object.run_diags(params) diff --git a/tests/integration/all_sets_modified.cfg b/tests/integration/all_sets_modified.cfg index b4fc5e472..ed20df010 100644 --- a/tests/integration/all_sets_modified.cfg +++ b/tests/integration/all_sets_modified.cfg @@ -9,9 +9,9 @@ test_name = "system tests" short_test_name = "short_system tests" ref_name = "ERA-Interim" reference_name = "ERA-Interim Reanalysis 1979-2015" -reference_data_path = "tests/integration" +reference_data_path = "tests/integration/integration_test_data" ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration" +test_data_path = "tests/integration/integration_test_data" test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" backend = "mpl" @@ -31,9 +31,9 @@ test_name = "system tests" short_test_name = "short_system tests" ref_name = "ERA-Interim" reference_name = "ERA-Interim Reanalysis 1989-2005" -reference_data_path = "tests/integration" +reference_data_path = "tests/integration/integration_test_data" ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration" +test_data_path = "tests/integration/integration_test_data" test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" backend = "mpl" @@ -55,9 +55,9 @@ test_name = "system tests" short_test_name = "short_system tests" ref_name = "ERA-Interim" reference_name = "ERA-Interim Reanalysis 1989-2005" -reference_data_path = "tests/integration" +reference_data_path = "tests/integration/integration_test_data" ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration" +test_data_path = "tests/integration/integration_test_data" test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" backend = "mpl" @@ -80,9 +80,9 @@ test_name = "system tests" short_test_name = "short_system tests" ref_name = "ERA-Interim" reference_name = "ERA-Interim Reanalysis 1979-2015" -reference_data_path = "tests/integration" +reference_data_path = "tests/integration/integration_test_data" ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration" +test_data_path = "tests/integration/integration_test_data" test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" backend = "mpl" @@ -104,9 +104,9 @@ test_name = "system tests" short_test_name = "short_system tests" ref_name = "ERA-Interim" reference_name = "ERA-Interim Reanalysis 1979-2015" -reference_data_path = "tests/integration" +reference_data_path = "tests/integration/integration_test_data" ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration" +test_data_path = "tests/integration/integration_test_data" test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" backend = "mpl" @@ -126,9 +126,9 @@ test_name = "system tests" short_test_name = "short_system tests" ref_name = "MISRCOSP" reference_name = "MISR COSP (2000-2009)" -reference_data_path = "tests/integration" +reference_data_path = "tests/integration/integration_test_data" ref_file = "CLDMISR_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration" +test_data_path = "tests/integration/integration_test_data" test_file = "CLD_MISR_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" backend = "mpl" @@ -142,7 +142,7 @@ variables = ["TREFHT"] test_name = "system tests" short_test_name = "short_system tests" -test_data_path = "tests/integration" +test_data_path = "tests/integration/integration_test_data" test_file = "TREFHT_201201_201312.nc" start_yr = '2012' end_yr = '2013' diff --git a/tests/integration/test_all_sets.py b/tests/integration/test_all_sets.py deleted file mode 100644 index f23d736a9..000000000 --- a/tests/integration/test_all_sets.py +++ /dev/null @@ -1,117 +0,0 @@ -import ast -import configparser -import os -import re -import shutil -from typing import List - -import pytest - -from e3sm_diags.parameter import SET_TO_PARAMETERS -from e3sm_diags.parameter.core_parameter import CoreParameter -from e3sm_diags.run import runner -from tests.integration.config import TEST_DATA_DIR -from tests.integration.utils import run_cmd_and_pipe_stderr - -# The path to the integration test data, which needs to be downloaded -# prior to running this test file. -MODULE_PATH = os.path.dirname(__file__) -TEST_DATA_PATH = os.path.join(MODULE_PATH, TEST_DATA_DIR) - -# The path to the integration test diagnostics .cfg file. -CFG_PATH = os.path.join(MODULE_PATH, "all_sets_modified.cfg") -CFG_PATH = os.path.abspath(CFG_PATH) - - -class TestAllSets: - def test_all_sets(self): - expected_num_diags = 12 - - # *_data_path needs to be added b/c the tests runs the diags from a different location - cmd = ( - f"e3sm_diags_driver.py -d {CFG_PATH} " - f"--reference_data_path {TEST_DATA_PATH} " - f"--test_data_path {TEST_DATA_PATH}" - ) - - stderr = run_cmd_and_pipe_stderr(cmd) - - # count the number of pngs in viewer_dir - results_dir = self._get_results_dir(stderr) - count = self._count_images(results_dir) - - # -1 is needed because of the E3SM logo in the viewer html - assert count - 1 == expected_num_diags - - shutil.rmtree(results_dir) # remove all generated results from the diags - - def _get_results_dir(self, output: List[str]): - """Given output from e3sm_diags_driver, extract the path to results_dir.""" - for line in output: - match = re.search("Viewer HTML generated at (.*)viewer.*.html", line) - if match: - results_dir = match.group(1) - return results_dir - - raise RuntimeError("No viewer directory listed in output: {}".format(output)) - - def _count_images(self, directory: str): - """Count the number of images of type file_type in directory""" - count = 0 - - for _, __, files in os.walk(directory): - for f in files: - if f.endswith("png"): - count += 1 - - return count - - @pytest.mark.xfail - def test_all_sets_directly(self): - # TODO: This test is meant to replace `test_all_sets`. It should create - # CoreParameter objects per diagnostic set defined in - # `all_sets_modified.cfg`. These CoreParameter objects should then be - # passed to `runner.run_diags()`. The benefit with this approach is - # that we don't need to run the command using subprocess, which means - # immediate unit testing feedback rather than waiting to pipe the - # complete stderr. We can also step through the code for debugging using - # an interactive console such as VS Code's Python debugger. - params = self._convert_cfg_to_param_objs() - - for param in params: - runner.run_diags([param]) - - def _convert_cfg_to_param_objs(self) -> List[CoreParameter]: - """Convert diagnostic cfg entries to parameter objects. - - NOTE: ast.literal_eval is not considered "safe" on untrusted data. - The reason why it is used is because `configparser.ConfigParser` - doesn't work well with parsing Python types from strings in - `.cfg` files, resulting in things such as nested strings or string - representation of lists. Since we are only calling literal_eval on - `.cfg` files hosted in this repo, there is minimal risk here. - - Returns - ------- - List[CoreParameter] - A list of CoreParameter objects, one for each diagnotic set. - """ - config = configparser.ConfigParser() - config.read(CFG_PATH) - params = [] - - for set_name in config.sections(): - param = SET_TO_PARAMETERS[set_name]() - - for option in config.options(set_name): - val = config.get(set_name, option) - val = ast.literal_eval(val) - - setattr(param, option, val) - - param.reference_data_path = TEST_DATA_PATH - param.test_data_path = TEST_DATA_PATH - - params.append(param) - - return params diff --git a/tests/integration/test_diags.py b/tests/integration/test_all_sets_image_diffs.py similarity index 95% rename from tests/integration/test_diags.py rename to tests/integration/test_all_sets_image_diffs.py index 2a33f1e7a..689e48a2e 100644 --- a/tests/integration/test_diags.py +++ b/tests/integration/test_all_sets_image_diffs.py @@ -2,16 +2,18 @@ import re import shutil import subprocess +import sys from typing import List import pytest from PIL import Image, ImageChops, ImageDraw from e3sm_diags.logger import custom_logger +from e3sm_diags.run import runner from tests.integration.config import TEST_IMAGES_PATH, TEST_ROOT_PATH -from tests.integration.utils import run_cmd_and_pipe_stderr +from tests.integration.utils import _get_test_params -# Run these tetsts on Cori by doing the following: +# Run these tests on Cori by doing the following: # cd tests/system # module load python/2.7-anaconda-4.4 # source activate e3sm_diags_env_dev @@ -19,20 +21,32 @@ # pip install /global/homes/f//e3sm_diags/ # python test_diags.py - # Set to True to place the results directory on Cori's web server # Set to False to place the results directory in tests/system CORI_WEB = False +CFG_PATH = f"{TEST_ROOT_PATH}/all_sets.cfg" logger = custom_logger(__name__) @pytest.fixture(scope="module") -def get_results_dir(): - command = f"python {TEST_ROOT_PATH}/all_sets.py -d {TEST_ROOT_PATH}/all_sets.cfg" - stderr = run_cmd_and_pipe_stderr(command) +def run_diags_and_get_results_dir() -> str: + """Run the diagnostics and get the results directory containing the images. + + The scope of this fixture is at the module level so that it only runs + once, then each individual test can reference the result directory. + + Returns + ------- + str + The path to the results directory. + """ + # Set -d flag to use CFG before running. + sys.argv.extend(["-d", CFG_PATH]) + params = _get_test_params() + results = runner.run_diags(params) + results_dir = results[0].results_dir - results_dir = _get_results_dir(stderr) logger.info("results_dir={}".format(results_dir)) if CORI_WEB: @@ -45,112 +59,10 @@ def get_results_dir(): return results_dir -def _get_results_dir(stderr): - """Given output from e3sm_diags_driver, extract the path to results_dir.""" - for line in stderr: - match = re.search("Viewer HTML generated at (.*)viewer.*.html", line) - if match: - results_dir = match.group(1) - return results_dir - - message = "No viewer directory listed in output: {}".format(stderr) - raise RuntimeError(message) - - -def _move_to_NERSC_webserver(machine_path_re_str, html_prefix_format_str, results_dir): - command = "git rev-parse --show-toplevel" - top_level = subprocess.check_output(command.split()).decode("utf-8").splitlines()[0] - match = re.search(machine_path_re_str, top_level) - if match: - username = match.group(1) - else: - message = "Username could not be extracted from top_level={}".format(top_level) - raise RuntimeError(message) - - html_prefix = html_prefix_format_str.format(username) - logger.info("html_prefix={}".format(html_prefix)) - new_results_dir = "{}/{}".format(html_prefix, results_dir) - logger.info("new_results_dir={}".format(new_results_dir)) - if os.path.exists(new_results_dir): - command = "rm -r {}".format(new_results_dir) - subprocess.check_output(command.split()) - command = "mv {} {}".format(results_dir, new_results_dir) - subprocess.check_output(command.split()) - command = "chmod -R 755 {}".format(new_results_dir) - subprocess.check_output(command.split()) - - return new_results_dir - - -def _compare_images( - mismatched_images: List[str], - image_name: str, - path_to_actual_png: str, - path_to_expected_png: str, -) -> List[str]: - # https://stackoverflow.com/questions/35176639/compare-images-python-pil - - actual_png = Image.open(path_to_actual_png).convert("RGB") - expected_png = Image.open(path_to_expected_png).convert("RGB") - diff = ImageChops.difference(actual_png, expected_png) - - diff_dir = f"{TEST_ROOT_PATH}image_check_failures" - if not os.path.isdir(diff_dir): - os.mkdir(diff_dir) - - bbox = diff.getbbox() - # If `diff.getbbox()` is None, then the images are in theory equal - if bbox is None: - pass - else: - # Sometimes, a few pixels will differ, but the two images appear identical. - # https://codereview.stackexchange.com/questions/55902/fastest-way-to-count-non-zero-pixels-using-python-and-pillow - nonzero_pixels = ( - diff.crop(bbox) - .point(lambda x: 255 if x else 0) - .convert("L") - .point(bool) - .getdata() - ) - num_nonzero_pixels = sum(nonzero_pixels) - logger.info("\npath_to_actual_png={}".format(path_to_actual_png)) - logger.info("path_to_expected_png={}".format(path_to_expected_png)) - logger.info("diff has {} nonzero pixels.".format(num_nonzero_pixels)) - width, height = expected_png.size - num_pixels = width * height - logger.info("total number of pixels={}".format(num_pixels)) - fraction = num_nonzero_pixels / num_pixels - logger.info("num_nonzero_pixels/num_pixels fraction={}".format(fraction)) - - # Fraction of mismatched pixels should be less than 0.02% - if fraction >= 0.0002: - mismatched_images.append(image_name) - - simple_image_name = image_name.split("/")[-1].split(".")[0] - shutil.copy( - path_to_actual_png, - os.path.join(diff_dir, "{}_actual.png".format(simple_image_name)), - ) - shutil.copy( - path_to_expected_png, - os.path.join(diff_dir, "{}_expected.png".format(simple_image_name)), - ) - # https://stackoverflow.com/questions/41405632/draw-a-rectangle-and-a-text-in-it-using-pil - draw = ImageDraw.Draw(diff) - (left, upper, right, lower) = diff.getbbox() - draw.rectangle(((left, upper), (right, lower)), outline="red") - diff.save( - os.path.join(diff_dir, "{}_diff.png".format(simple_image_name)), - "PNG", - ) - - return mismatched_images - - -class TestAllSets: +class TestAllSetsImageDiffs: @pytest.fixture(autouse=True) - def setup(self, get_results_dir): - self.results_dir = get_results_dir + def setup(self, run_diags_and_get_results_dir): + self.results_dir = run_diags_and_get_results_dir def test_results_directory_ends_with_specific_directory(self): assert self.results_dir.endswith("all_sets_results_test/") @@ -484,3 +396,93 @@ def _check_streamflow_plots(self): # Check the full HTML path is the same as the expected. full_html_path = "{}{}".format(self.results_dir, html_path) self._check_html_image(full_html_path, png_path, full_png_path) + + +def _move_to_NERSC_webserver(machine_path_re_str, html_prefix_format_str, results_dir): + command = "git rev-parse --show-toplevel" + top_level = subprocess.check_output(command.split()).decode("utf-8").splitlines()[0] + match = re.search(machine_path_re_str, top_level) + if match: + username = match.group(1) + else: + message = "Username could not be extracted from top_level={}".format(top_level) + raise RuntimeError(message) + + html_prefix = html_prefix_format_str.format(username) + logger.info("html_prefix={}".format(html_prefix)) + new_results_dir = "{}/{}".format(html_prefix, results_dir) + logger.info("new_results_dir={}".format(new_results_dir)) + if os.path.exists(new_results_dir): + command = "rm -r {}".format(new_results_dir) + subprocess.check_output(command.split()) + command = "mv {} {}".format(results_dir, new_results_dir) + subprocess.check_output(command.split()) + command = "chmod -R 755 {}".format(new_results_dir) + subprocess.check_output(command.split()) + + return new_results_dir + + +def _compare_images( + mismatched_images: List[str], + image_name: str, + path_to_actual_png: str, + path_to_expected_png: str, +) -> List[str]: + # https://stackoverflow.com/questions/35176639/compare-images-python-pil + + actual_png = Image.open(path_to_actual_png).convert("RGB") + expected_png = Image.open(path_to_expected_png).convert("RGB") + diff = ImageChops.difference(actual_png, expected_png) + + diff_dir = f"{TEST_ROOT_PATH}image_check_failures" + if not os.path.isdir(diff_dir): + os.mkdir(diff_dir) + + bbox = diff.getbbox() + # If `diff.getbbox()` is None, then the images are in theory equal + if bbox is None: + pass + else: + # Sometimes, a few pixels will differ, but the two images appear identical. + # https://codereview.stackexchange.com/questions/55902/fastest-way-to-count-non-zero-pixels-using-python-and-pillow + nonzero_pixels = ( + diff.crop(bbox) + .point(lambda x: 255 if x else 0) + .convert("L") + .point(bool) + .getdata() + ) + num_nonzero_pixels = sum(nonzero_pixels) + logger.info("\npath_to_actual_png={}".format(path_to_actual_png)) + logger.info("path_to_expected_png={}".format(path_to_expected_png)) + logger.info("diff has {} nonzero pixels.".format(num_nonzero_pixels)) + width, height = expected_png.size + num_pixels = width * height + logger.info("total number of pixels={}".format(num_pixels)) + fraction = num_nonzero_pixels / num_pixels + logger.info("num_nonzero_pixels/num_pixels fraction={}".format(fraction)) + + # Fraction of mismatched pixels should be less than 0.02% + if fraction >= 0.0002: + mismatched_images.append(image_name) + + simple_image_name = image_name.split("/")[-1].split(".")[0] + shutil.copy( + path_to_actual_png, + os.path.join(diff_dir, "{}_actual.png".format(simple_image_name)), + ) + shutil.copy( + path_to_expected_png, + os.path.join(diff_dir, "{}_expected.png".format(simple_image_name)), + ) + # https://stackoverflow.com/questions/41405632/draw-a-rectangle-and-a-text-in-it-using-pil + draw = ImageDraw.Draw(diff) + (left, upper, right, lower) = diff.getbbox() + draw.rectangle(((left, upper), (right, lower)), outline="red") + diff.save( + os.path.join(diff_dir, "{}_diff.png".format(simple_image_name)), + "PNG", + ) + + return mismatched_images diff --git a/tests/integration/test_all_sets_modified_image_counts.py b/tests/integration/test_all_sets_modified_image_counts.py new file mode 100644 index 000000000..bc9a2d300 --- /dev/null +++ b/tests/integration/test_all_sets_modified_image_counts.py @@ -0,0 +1,42 @@ +import os +import shutil +from typing import List + +from e3sm_diags.parameter.core_parameter import CoreParameter +from e3sm_diags.run import runner +from tests.integration.config import TEST_DATA_DIR +from tests.integration.utils import _convert_cfg_to_param_objs, _count_images + +# The path to the integration test data, which needs to be downloaded +# prior to running this test file. +MODULE_PATH = os.path.dirname(__file__) +TEST_DATA_PATH = os.path.join(MODULE_PATH, TEST_DATA_DIR) + +# The path to the integration test diagnostics .cfg file. +CFG_PATH = os.path.join(MODULE_PATH, "all_sets_modified.cfg") +CFG_PATH = os.path.abspath(CFG_PATH) + + +EXPECTED_NUM_IMAGES = 12 + + +def test_all_sets_modified_produces_the_expected_number_of_images(): + params = _convert_cfg_to_param_objs(CFG_PATH) + + results: List[CoreParameter] = [] + + for param in params: + result = runner.run_diags([param]) + results.extend(result) + + # The result directory should be the same for all diagnostic sets. + result_dir = results[0].results_dir + + # -1 is needed because of the E3SM logo that is used by the viewer HTML. + num_images = _count_images(result_dir) - 1 + + assert num_images == EXPECTED_NUM_IMAGES + + # TODO: Result dir should be set thet temporary pytest location that + # automatically gets cleaned up after every test. + shutil.rmtree(result_dir) diff --git a/tests/integration/utils.py b/tests/integration/utils.py index 014a7521d..a7b01585c 100644 --- a/tests/integration/utils.py +++ b/tests/integration/utils.py @@ -1,6 +1,18 @@ +import ast +import configparser +import os import subprocess from typing import List +from e3sm_diags.parameter import SET_TO_PARAMETERS +from e3sm_diags.parameter.area_mean_time_series_parameter import ( + AreaMeanTimeSeriesParameter, +) +from e3sm_diags.parameter.core_parameter import CoreParameter +from e3sm_diags.parameter.enso_diags_parameter import EnsoDiagsParameter +from e3sm_diags.parameter.meridional_mean_2d_parameter import MeridionalMean2dParameter +from e3sm_diags.parameter.zonal_mean_2d_parameter import ZonalMean2dParameter + def run_cmd_and_pipe_stderr(command: str) -> List[str]: """Runs the test command and pipes the stderr for further processing. @@ -44,3 +56,71 @@ def run_cmd_and_pipe_stderr(command: str) -> List[str]: print(*stderr, sep="\n") return stderr + + +def _get_test_params() -> List[CoreParameter]: + param = CoreParameter() + ts_param = AreaMeanTimeSeriesParameter() + + m2d_param = MeridionalMean2dParameter() + m2d_param.plevs = [ + 200.0, + 500.0, + ] + z2d_param = ZonalMean2dParameter() + z2d_param.plevs = [ + 200.0, + 300.0, + ] + + enso_param = EnsoDiagsParameter() + enso_param.test_name = "e3sm_v1" + + params = [param, ts_param, m2d_param, z2d_param, enso_param] + + return params + + +def _convert_cfg_to_param_objs(cfg_path: str) -> List[CoreParameter]: + """Convert diagnostic cfg entries to parameter objects. + + NOTE: ast.literal_eval is not considered "safe" on untrusted data. + The reason why it is used is because `configparser.ConfigParser` + doesn't work well with parsing Python types from strings in + `.cfg` files, resulting in things such as nested strings or string + representation of lists. Since we are only calling literal_eval on + `.cfg` files hosted in this repo, there is minimal risk here. + + Returns + ------- + List[CoreParameter] + A list of CoreParameter objects, one for each diagnotic set. + """ + config = configparser.ConfigParser() + config.read(cfg_path) + params = [] + + for set_name in config.sections(): + param = SET_TO_PARAMETERS[set_name]() + + for option in config.options(set_name): + val = config.get(set_name, option) + val = ast.literal_eval(val) + + setattr(param, option, val) + + params.append(param) + + return params + + +def _count_images(directory: str): + """Count the number of images of type file_type in directory""" + count = 0 + + for _, __, files in os.walk(directory): + for f in files: + if f.endswith("png"): + count += 1 + + return count From 083ae3c2be5bf379059eaa8558e3d08af7d7775a Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 13:33:49 -0800 Subject: [PATCH 03/31] Fix paths for enso_diags in `all_sets_modified.cfg` --- tests/integration/all_sets_modified.cfg | 4 ++-- .../test_all_sets_modified_image_counts.py | 24 +++++++------------ 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/tests/integration/all_sets_modified.cfg b/tests/integration/all_sets_modified.cfg index ed20df010..2ec85df7f 100644 --- a/tests/integration/all_sets_modified.cfg +++ b/tests/integration/all_sets_modified.cfg @@ -169,9 +169,9 @@ results_dir = "tests/integration/all_sets_results" seasons = ["ANN"] start_yr = '2012' end_yr = '2013' -test_data_path = '.' +test_data_path = "tests/integration/integration_test_data" test_file = 'TREFHT_201201_201312.nc' -reference_data_path = '.' +reference_data_path = "tests/integration/integration_test_data" ref_file = 'TREFHT_201201_201312.nc' test_name = "system tests" variables = ["TREFHT"] diff --git a/tests/integration/test_all_sets_modified_image_counts.py b/tests/integration/test_all_sets_modified_image_counts.py index bc9a2d300..ebf6434d5 100644 --- a/tests/integration/test_all_sets_modified_image_counts.py +++ b/tests/integration/test_all_sets_modified_image_counts.py @@ -4,36 +4,28 @@ from e3sm_diags.parameter.core_parameter import CoreParameter from e3sm_diags.run import runner -from tests.integration.config import TEST_DATA_DIR +from tests.integration.config import TEST_ROOT_PATH from tests.integration.utils import _convert_cfg_to_param_objs, _count_images -# The path to the integration test data, which needs to be downloaded -# prior to running this test file. -MODULE_PATH = os.path.dirname(__file__) -TEST_DATA_PATH = os.path.join(MODULE_PATH, TEST_DATA_DIR) - # The path to the integration test diagnostics .cfg file. -CFG_PATH = os.path.join(MODULE_PATH, "all_sets_modified.cfg") +CFG_PATH = os.path.join(TEST_ROOT_PATH, "all_sets_modified.cfg") CFG_PATH = os.path.abspath(CFG_PATH) - -EXPECTED_NUM_IMAGES = 12 +# +1 is needed because of the E3SM logo that is used by the viewer HTML. +EXPECTED_NUM_IMAGES = 12 + 1 def test_all_sets_modified_produces_the_expected_number_of_images(): params = _convert_cfg_to_param_objs(CFG_PATH) - - results: List[CoreParameter] = [] + params_results: List[CoreParameter] = [] for param in params: result = runner.run_diags([param]) - results.extend(result) + params_results.extend(result) # The result directory should be the same for all diagnostic sets. - result_dir = results[0].results_dir - - # -1 is needed because of the E3SM logo that is used by the viewer HTML. - num_images = _count_images(result_dir) - 1 + result_dir = params_results[0].results_dir + num_images = _count_images(result_dir) assert num_images == EXPECTED_NUM_IMAGES From daf1940d90af9322c0e7a3e9e4124493c5f440e2 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 14:15:57 -0800 Subject: [PATCH 04/31] Fix referenced paths in `test_all_sets_image_diffs.py` --- pyproject.toml | 2 +- .../integration/test_all_sets_image_diffs.py | 64 +++++++------------ 2 files changed, 24 insertions(+), 42 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 50df363ba..a60a540a1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,7 @@ skip = "e3sm_diags/e3sm_diags_driver.py" junit_family = "xunit2" addopts = "--cov=e3sm_diags --cov-report term --cov-report html:tests_coverage_reports/htmlcov --cov-report xml:tests_coverage_reports/coverage.xml -s" python_files = ["tests.py", "test_*.py"] -testpaths = "tests/e3sm_diags" +# testpaths = "tests/e3sm_diags" [tool.mypy] # Docs: https://mypy.readthedocs.io/en/stable/config_file.html diff --git a/tests/integration/test_all_sets_image_diffs.py b/tests/integration/test_all_sets_image_diffs.py index 689e48a2e..fdcd0f677 100644 --- a/tests/integration/test_all_sets_image_diffs.py +++ b/tests/integration/test_all_sets_image_diffs.py @@ -13,17 +13,6 @@ from tests.integration.config import TEST_IMAGES_PATH, TEST_ROOT_PATH from tests.integration.utils import _get_test_params -# Run these tests on Cori by doing the following: -# cd tests/system -# module load python/2.7-anaconda-4.4 -# source activate e3sm_diags_env_dev -# If code in e3sm_diags has been changed: -# pip install /global/homes/f//e3sm_diags/ -# python test_diags.py - -# Set to True to place the results directory on Cori's web server -# Set to False to place the results directory in tests/system -CORI_WEB = False CFG_PATH = f"{TEST_ROOT_PATH}/all_sets.cfg" logger = custom_logger(__name__) @@ -41,20 +30,15 @@ def run_diags_and_get_results_dir() -> str: str The path to the results directory. """ - # Set -d flag to use CFG before running. + # Set -d flag to use the .cfg file for running additional diagnostic sets. sys.argv.extend(["-d", CFG_PATH]) + params = _get_test_params() results = runner.run_diags(params) - results_dir = results[0].results_dir - logger.info("results_dir={}".format(results_dir)) + results_dir = results[0].results_dir - if CORI_WEB: - results_dir = _move_to_NERSC_webserver( - "/global/u1/f/(.*)/e3sm_diags", - "/global/cfs/cdirs/acme/www/{}", - results_dir, - ) + logger.info(f"results_dir={results_dir}") return results_dir @@ -65,7 +49,7 @@ def setup(self, run_diags_and_get_results_dir): self.results_dir = run_diags_and_get_results_dir def test_results_directory_ends_with_specific_directory(self): - assert self.results_dir.endswith("all_sets_results_test/") + assert "all_sets_results_test" in self.results_dir def test_actual_images_produced_is_the_same_as_the_expected(self): actual_num_images, actual_images = self._count_images_in_dir( @@ -86,15 +70,16 @@ def test_area_mean_time_series_plot_diffs(self): # Check PNG path is the same as the expected. png_path = "{}/{}.png".format(set_name, variable) - full_png_path = "{}{}".format(self.results_dir, png_path) + full_png_path = os.path.join(self.results_dir, png_path) path_exists = os.path.exists(full_png_path) assert path_exists # Check full HTML path is the same as the expected. - html_path = "{}viewer/{}/variable/{}/plot.html".format( - self.results_dir, set_name, variable_lower + filename = "viewer/{}/variable/{}/plot.html".format( + set_name, variable_lower ) + html_path = os.path.join(self.results_dir, filename) self._check_html_image(html_path, png_path, full_png_path) def test_cosp_histogram_plot_diffs(self): @@ -149,16 +134,15 @@ def test_qbo_plot_diffs(self): # Check PNG path is the same as the expected. png_path = "{}/{}/qbo_diags.png".format(set_name, case_id) - full_png_path = "{}{}".format(self.results_dir, png_path) + full_png_path = os.path.join(self.results_dir, png_path) path_exists = os.path.exists(full_png_path) assert path_exists # Check full HTML path is the same as the expected. # viewer/qbo/variable/era-interim/plot.html - html_path = "{}viewer/{}/variable/{}/plot.html".format( - self.results_dir, set_name, case_id_lower - ) + filename = "viewer/{}/variable/{}/plot.html".format(set_name, case_id_lower) + html_path = os.path.join(self.results_dir, filename) self._check_html_image(html_path, png_path, full_png_path) def test_streamflow_plot_diffs(self): @@ -265,14 +249,13 @@ def _check_plots_generic( region, ) - full_png_path = "{}{}".format(self.results_dir, png_path) + full_png_path = os.path.join(self.results_dir, png_path) path_exists = os.path.exists(full_png_path) assert path_exists # Check full HTML path is the same as the expected. - html_path = "{}viewer/{}/{}/{}-{}{}-{}/{}.html".format( - self.results_dir, + filename = "viewer/{}/{}/{}-{}{}-{}/{}.html".format( set_name, case_id_lower, variable_lower, @@ -281,6 +264,7 @@ def _check_plots_generic( ref_name_lower, season_lower, ) + html_path = os.path.join(self.results_dir, filename) self._check_html_image(html_path, png_path, full_png_path) def _check_plots_2d(self, set_name): @@ -316,15 +300,14 @@ def _check_enso_map_plots(self, case_id): png_path = "{}/{}/regression-coefficient-{}-over-{}.png".format( set_name, case_id, variable_lower, nino_region_lower ) - full_png_path = "{}{}".format(self.results_dir, png_path) + full_png_path = os.path.join(self.results_dir, png_path) path_exists = os.path.exists(full_png_path) assert path_exists # Check full HTML path is the same as the expected. - html_path = "{}viewer/{}/map/{}/plot.html".format( - self.results_dir, set_name, case_id_lower - ) + filename = "viewer/{}/map/{}/plot.html".format(set_name, case_id_lower) + html_path = os.path.join(self.results_dir, filename) self._check_html_image(html_path, png_path, full_png_path) def _check_enso_scatter_plots(self, case_id): @@ -339,15 +322,14 @@ def _check_enso_scatter_plots(self, case_id): png_path = "{}/{}/feedback-{}-{}-TS-NINO3.png".format( set_name, case_id, variable, region ) - full_png_path = "{}{}".format(self.results_dir, png_path) + full_png_path = os.path.join(self.results_dir, png_path) path_exists = os.path.exists(full_png_path) assert path_exists # Check full HTML path is the same as the expected. - html_path = "{}viewer/{}/scatter/{}/plot.html".format( - self.results_dir, set_name, case_id_lower - ) + filename = "viewer/{}/scatter/{}/plot.html".format(set_name, case_id_lower) + html_path = os.path.join(self.results_dir, filename) self._check_html_image(html_path, png_path, full_png_path) def _check_streamflow_plots(self): @@ -371,7 +353,7 @@ def _check_streamflow_plots(self): assert png_path == expected # Check path exists - full_png_path = "{}{}".format(self.results_dir, png_path) + full_png_path = os.path.join(self.results_dir, png_path) path_exists = os.path.exists(full_png_path) assert path_exists @@ -394,7 +376,7 @@ def _check_streamflow_plots(self): assert html_path == expected # Check the full HTML path is the same as the expected. - full_html_path = "{}{}".format(self.results_dir, html_path) + full_html_path = os.path.join(self.results_dir, html_path) self._check_html_image(full_html_path, png_path, full_png_path) From 9b55499421b70f096d7454feb9b160598270c3a2 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 14:18:49 -0800 Subject: [PATCH 05/31] Remove unused NERSC function --- .../integration/test_all_sets_image_diffs.py | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/tests/integration/test_all_sets_image_diffs.py b/tests/integration/test_all_sets_image_diffs.py index fdcd0f677..7ac3bd284 100644 --- a/tests/integration/test_all_sets_image_diffs.py +++ b/tests/integration/test_all_sets_image_diffs.py @@ -1,7 +1,6 @@ import os import re import shutil -import subprocess import sys from typing import List @@ -380,31 +379,6 @@ def _check_streamflow_plots(self): self._check_html_image(full_html_path, png_path, full_png_path) -def _move_to_NERSC_webserver(machine_path_re_str, html_prefix_format_str, results_dir): - command = "git rev-parse --show-toplevel" - top_level = subprocess.check_output(command.split()).decode("utf-8").splitlines()[0] - match = re.search(machine_path_re_str, top_level) - if match: - username = match.group(1) - else: - message = "Username could not be extracted from top_level={}".format(top_level) - raise RuntimeError(message) - - html_prefix = html_prefix_format_str.format(username) - logger.info("html_prefix={}".format(html_prefix)) - new_results_dir = "{}/{}".format(html_prefix, results_dir) - logger.info("new_results_dir={}".format(new_results_dir)) - if os.path.exists(new_results_dir): - command = "rm -r {}".format(new_results_dir) - subprocess.check_output(command.split()) - command = "mv {} {}".format(results_dir, new_results_dir) - subprocess.check_output(command.split()) - command = "chmod -R 755 {}".format(new_results_dir) - subprocess.check_output(command.split()) - - return new_results_dir - - def _compare_images( mismatched_images: List[str], image_name: str, From cb9f64e30f20d7dca220453337c29eeb9acf7a5c Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 16:24:51 -0800 Subject: [PATCH 06/31] Add debug mode --- Makefile | 6 ++ e3sm_diags/run.py | 188 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 154 insertions(+), 40 deletions(-) diff --git a/Makefile b/Makefile index eb6445974..4173c4ba2 100644 --- a/Makefile +++ b/Makefile @@ -55,6 +55,12 @@ clean-test: ## remove test and coverage artifacts rm -fr .pytest_cache rm -rf .mypy_cache +clean-test-int: + rm -rf tests/integration/all_sets_results_test + rm -rf tests/integration/image_check_failures + rm -rf tests/integration/integration_test_data + rm -rf tests/integration/integration_test_images + # Quality Assurance # ---------------------- pre-commit: # run pre-commit quality assurance checks diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 35ab29808..bc16df3f8 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -22,64 +22,178 @@ class Run: def __init__(self): self.parser = CoreParser() - def run_diags(self, parameters): + def run_diags( + self, parameters: List[CoreParameter], debug=False + ) -> List[CoreParameter]: + """Run a set of diagnostics with a list of parameters. + + Parameters + ---------- + parameters : List[CoreParameter] + A list of parameters. + debug : bool, optional + Run in debug mode or not, by default False. + + * If True, only the parameters passed via ``parameters`` will be + run. The sets to run are based on the sets defined by the + parameters. This makes it easy to debug a few sets. + * If False, run all sets using the list of parameters passed in this + function and parameters defined in a .cfg file (if defined). + This is the default option. + + Returns + ------- + List[CoreParameter] + _description_ + + Raises + ------ + RuntimeError + _description_ """ - Based on sets_to_run, run the diags with the list of parameters. - """ - final_params = self.get_final_parameters(parameters) - if not final_params: - msg = "No parameters we able to be extracted." - msg += " Please check the parameters you defined." - raise RuntimeError(msg) + + params = self.get_final_parameters(parameters, debug) + + if params is None or len(params) == 0: + raise RuntimeError( + "No parameters we able to be extracted. Please " + "check the parameters you defined." + ) try: - parameter_results = main(final_params) + params_results = main(params) except Exception: logger.exception("Error traceback:", exc_info=True) - move_log_to_prov_dir(parameter_results[0].results_dir) + move_log_to_prov_dir(params_results[0].results_dir) - return parameter_results + return params_results - def get_final_parameters(self, parameters): + def get_final_parameters(self, parameters: List[CoreParameter], debug: bool): """ Based on sets_to_run and the list of parameters, get the final list of paremeters to run the diags on. """ - if not parameters or not isinstance(parameters, list): - msg = "You must pass in a list of parameter objects." - raise RuntimeError(msg) + self._validate_parameters(parameters) + + # FIXME: This line produces some unintended side-effects. For example, + # let's say we have two objects: 1. CoreParameter, 2. ZonalMean2DParameter. + # If object 1 has `plevs=[200]`, this value will get copied to object 2. + # Object 2 has a check to make sure plevs has more than 1 value. This + # breaks the diagnostic run as a result. The workaround is to loop + # over `run_diags()` function and run one parameter at a time. + self._add_parent_attrs_to_children(parameters) + + if debug: + final_params = self._get_debug_parameters(parameters) + else: + final_params = self._get_run_parameters(parameters) + + self.parser.check_values_of_params(final_params) + + return final_params + + def _validate_parameters(self, parameters: List[CoreParameter]): + if parameters is None or not isinstance(parameters, list): + raise RuntimeError("You must pass in a list of parameter objects.") - # For each of the passed in parameters, we can only have one of - # each type. param_types_list = [ p.__class__ for p in parameters if p.__class__ != CoreParameter ] param_types_set = set(param_types_list) if len(param_types_set) != len(param_types_list): - msg = ( + raise RuntimeError( "You passed in two or more non-CoreParameter objects of the same type." ) - raise RuntimeError(msg) - # FIXME: This line produces some unintended side-effects. For example, - # let's say we have two objects: 1. CoreParameter, 2. ZonalMean2DParameter. - # If object 1 has `plevs=[200]`, this value will get copied to object 2. - # Object 2 has a check to make sure plevs has more than 1 value. This - # breaks the diagnostic run as a result. The workaround is to loop - # over `run_diags()` function and run one parameter at a time. - self._add_parent_attrs_to_children(parameters) + def _get_run_parameters( + self, parameters: List[CoreParameter] + ) -> List[CoreParameter]: + """Get the run parameters using all sets and a .cfg file if passed. + + Parameters + ---------- + parameters : List[CoreParameter] + A list of parameter objects. + + Returns + ------- + List[CoreParameter] + A list of parameter objects including ones defined in a .cfg file. + Any non-CoreParameter objects will be replaced by a sub-class + based on the set (``SETS_TO_PARAMETERS``). + """ + run_params = [] + + sets_to_run = SET_TO_PARAMETERS.keys() + for set_name in sets_to_run: + other_params = self._get_diags_from_cfg_file() + + # For each of the set_names, get the corresponding parameter. + param = self._get_instance_of_param_class( + SET_TO_PARAMETERS[set_name], parameters + ) + + # Since each parameter will have lots of default values, we want to remove them. + # Otherwise when calling get_parameters(), these default values + # will take precedence over values defined in other_params. + self._remove_attrs_with_default_values(param) + param.sets = [set_name] + + params = self.parser.get_parameters( + orig_parameters=param, + other_parameters=other_params, + cmd_default_vars=False, + argparse_vals_only=False, + ) + + # Makes sure that any parameters that are selectors + # will be in param. + self._add_attrs_with_default_values(param) + # The select() call in get_parameters() was made for the original + # command-line way of using CDP. + # We just call it manually with the parameter object param. + params = self.parser.select(param, params) + + run_params.extend(params) + + return run_params + + def _get_debug_parameters( + self, parameters: List[CoreParameter] + ) -> List[CoreParameter]: + """Get the parameters explicitly defined in the Python script only. + + This method replaces CoreParameter objects with the related sub-class + for the specified set. + + Parameters + ---------- + parameters : List[CoreParameter] + A list of parameter objects. + + Returns + ------- + List[CoreParameter] + A list of parameter objects. Any non-CoreParameter objects will be + replaced by a sub-class based on the set (``SETS_TO_PARAMETERS``). + + Notes + ----- + This method is implemented to avoid any breaking changes or inadvertent + side-effects of modifying `_get_run_parameters()`. In the future, it + is advisable to refactor both methods as a single method. + """ + debug_params = [] - # Get the sets to run using the parameter objects via the Python API sets_to_run = [param.sets for param in parameters] - self.sets_to_run = list(chain.from_iterable(sets_to_run)) + sets_to_run = list(chain.from_iterable(sets_to_run)) # type: ignore - api_params = [] - for set_name in self.sets_to_run: + for set_name in sets_to_run: # For each of the set_names, get the corresponding parameter. api_param = self._get_instance_of_param_class( - SET_TO_PARAMETERS[set_name], parameters + SET_TO_PARAMETERS[set_name], parameters # type: ignore ) # Since each parameter will have lots of default values, we want to remove them. @@ -91,17 +205,11 @@ def get_final_parameters(self, parameters): # Makes sure that any parameters that are selectors will be in param. self._add_attrs_with_default_values(api_param) - api_params.append(api_param) + debug_params.append(api_param) - # Get the diagnostic parameter objects from the .cfg file passed via - # the -d/--diags CLI argument (if set). Otherwise, it will be an empty - # list. - cfg_params = self._get_diags_from_cfg_file() + self.parser.check_values_of_params(debug_params) - final_params = api_params + cfg_params - self.parser.check_values_of_params(final_params) - - return final_params + return debug_params def _add_parent_attrs_to_children(self, parameters): """ From 715c025d0f7ff94c279434b4d8bbb2015f6beb8d Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 16:35:54 -0800 Subject: [PATCH 07/31] Fix missing default value to debug in get_final_parameters --- e3sm_diags/run.py | 9 +++++++-- tests/integration/test_all_sets_modified_image_counts.py | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index bc16df3f8..a22769121 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -22,8 +22,11 @@ class Run: def __init__(self): self.parser = CoreParser() + # The list of sets to run using parameter objects. + self.sets_to_run = [] + def run_diags( - self, parameters: List[CoreParameter], debug=False + self, parameters: List[CoreParameter], debug: bool = False ) -> List[CoreParameter]: """Run a set of diagnostics with a list of parameters. @@ -69,7 +72,9 @@ def run_diags( return params_results - def get_final_parameters(self, parameters: List[CoreParameter], debug: bool): + def get_final_parameters( + self, parameters: List[CoreParameter], debug: bool = False + ): """ Based on sets_to_run and the list of parameters, get the final list of paremeters to run the diags on. diff --git a/tests/integration/test_all_sets_modified_image_counts.py b/tests/integration/test_all_sets_modified_image_counts.py index ebf6434d5..ab2bf9a73 100644 --- a/tests/integration/test_all_sets_modified_image_counts.py +++ b/tests/integration/test_all_sets_modified_image_counts.py @@ -20,7 +20,7 @@ def test_all_sets_modified_produces_the_expected_number_of_images(): params_results: List[CoreParameter] = [] for param in params: - result = runner.run_diags([param]) + result = runner.run_diags([param], debug=True) params_results.extend(result) # The result directory should be the same for all diagnostic sets. From 0cdca64d3523a77ac71e05abb25415bc836f6393 Mon Sep 17 00:00:00 2001 From: Tom Vo Date: Wed, 29 Nov 2023 16:36:37 -0800 Subject: [PATCH 08/31] Update pyproject.toml --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index a60a540a1..50df363ba 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,7 @@ skip = "e3sm_diags/e3sm_diags_driver.py" junit_family = "xunit2" addopts = "--cov=e3sm_diags --cov-report term --cov-report html:tests_coverage_reports/htmlcov --cov-report xml:tests_coverage_reports/coverage.xml -s" python_files = ["tests.py", "test_*.py"] -# testpaths = "tests/e3sm_diags" +testpaths = "tests/e3sm_diags" [tool.mypy] # Docs: https://mypy.readthedocs.io/en/stable/config_file.html From 2f96ca410e4cbffcdb09d85a7e3f00c53da095fb Mon Sep 17 00:00:00 2001 From: Tom Vo Date: Wed, 29 Nov 2023 16:36:42 -0800 Subject: [PATCH 09/31] Update e3sm_diags/run.py --- e3sm_diags/run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index a22769121..4819ea5bd 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -47,7 +47,7 @@ def run_diags( Returns ------- List[CoreParameter] - _description_ + A list of parameter objects with their results. Raises ------ From 590fc37659f553abd7477a533d490ce848c4d041 Mon Sep 17 00:00:00 2001 From: Tom Vo Date: Wed, 29 Nov 2023 16:36:48 -0800 Subject: [PATCH 10/31] Update e3sm_diags/run.py --- e3sm_diags/run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 4819ea5bd..720698174 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -52,7 +52,7 @@ def run_diags( Raises ------ RuntimeError - _description_ + If a diagnostic run using a parameter fails for any reason. """ params = self.get_final_parameters(parameters, debug) From e7b049de4a59b2e155894dc20e8007d6da413ed0 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 16:52:05 -0800 Subject: [PATCH 11/31] Fix sets to run --- e3sm_diags/run.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 720698174..9d9fdecc6 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -131,8 +131,10 @@ def _get_run_parameters( """ run_params = [] - sets_to_run = SET_TO_PARAMETERS.keys() - for set_name in sets_to_run: + if len(self.sets_to_run) == 0: + self.sets_to_run = list(SET_TO_PARAMETERS.keys()) + + for set_name in self.sets_to_run: other_params = self._get_diags_from_cfg_file() # For each of the set_names, get the corresponding parameter. @@ -192,13 +194,14 @@ def _get_debug_parameters( """ debug_params = [] - sets_to_run = [param.sets for param in parameters] - sets_to_run = list(chain.from_iterable(sets_to_run)) # type: ignore + if len(self.sets_to_run) == 0: + sets_to_run = [param.sets for param in parameters] + self.sets_to_run = list(chain.from_iterable(sets_to_run)) - for set_name in sets_to_run: + for set_name in self.sets_to_run: # For each of the set_names, get the corresponding parameter. api_param = self._get_instance_of_param_class( - SET_TO_PARAMETERS[set_name], parameters # type: ignore + SET_TO_PARAMETERS[set_name], parameters ) # Since each parameter will have lots of default values, we want to remove them. From 9cb67778f4cca917dee386c039fc17b7cdfb5ad1 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 17:06:53 -0800 Subject: [PATCH 12/31] Fix default sets --- e3sm_diags/parameter/__init__.py | 7 +++++++ e3sm_diags/run.py | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/e3sm_diags/parameter/__init__.py b/e3sm_diags/parameter/__init__.py index a09a9f393..e846fbeeb 100644 --- a/e3sm_diags/parameter/__init__.py +++ b/e3sm_diags/parameter/__init__.py @@ -36,3 +36,10 @@ "aerosol_budget": CoreParameter, "mp_partition": MPpartitionParameter, } + + +# FIXME: mp_partition was not originally included as a default set +# in `CoreParameter.sets`. Including it will break an integration +# test, so it needs to be removed. +DEFAULT_SETS = list(SET_TO_PARAMETERS.keys()) +DEFAULT_SETS = [key for key in DEFAULT_SETS if key != "mp_partition"] diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 9d9fdecc6..62792fa38 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -5,7 +5,7 @@ import e3sm_diags # noqa: F401 from e3sm_diags.e3sm_diags_driver import main from e3sm_diags.logger import custom_logger, move_log_to_prov_dir -from e3sm_diags.parameter import SET_TO_PARAMETERS +from e3sm_diags.parameter import DEFAULT_SETS, SET_TO_PARAMETERS from e3sm_diags.parameter.core_parameter import CoreParameter from e3sm_diags.parser.core_parser import CoreParser @@ -132,7 +132,7 @@ def _get_run_parameters( run_params = [] if len(self.sets_to_run) == 0: - self.sets_to_run = list(SET_TO_PARAMETERS.keys()) + self.sets_to_run = DEFAULT_SETS for set_name in self.sets_to_run: other_params = self._get_diags_from_cfg_file() From 0b99fff2b84dd1a904696654af5872a4d936ac7e Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 29 Nov 2023 17:25:07 -0800 Subject: [PATCH 13/31] Fix `test_all_sets_and_all_season` --- pyproject.toml | 2 +- tests/integration/test_run.py | 13 +------------ 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 50df363ba..a60a540a1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,7 @@ skip = "e3sm_diags/e3sm_diags_driver.py" junit_family = "xunit2" addopts = "--cov=e3sm_diags --cov-report term --cov-report html:tests_coverage_reports/htmlcov --cov-report xml:tests_coverage_reports/coverage.xml -s" python_files = ["tests.py", "test_*.py"] -testpaths = "tests/e3sm_diags" +# testpaths = "tests/e3sm_diags" [tool.mypy] # Docs: https://mypy.readthedocs.io/en/stable/config_file.html diff --git a/tests/integration/test_run.py b/tests/integration/test_run.py index f1ea689de..cdbf77576 100644 --- a/tests/integration/test_run.py +++ b/tests/integration/test_run.py @@ -65,7 +65,7 @@ def test_all_sets_and_all_seasons(self): ] parameters = self.runner.get_final_parameters( - [self.core_param, ts_param, enso_param, streamflow_param] + [self.core_param, ts_param, enso_param, streamflow_param], debug=True ) # Counts the number of each set and each seasons to run the diags on. set_counter, season_counter = ( @@ -84,17 +84,6 @@ def test_all_sets_and_all_seasons(self): msg = "Count for {} is invalid: {}" self.fail(msg.format(set_name, count)) - # enso_diags and streamflow only run ANN, no seasons - # So, reduce the ANN count by the number of times these appear - season_counter["ANN"] -= set_counter["enso_diags"] - season_counter["ANN"] -= set_counter["streamflow"] - if not all(season_counter["ANN"] == count for count in season_counter.values()): - self.fail( - "In .cfg files, at least one season does not match the count for ANN: {}".format( - season_counter - ) - ) - def test_zonal_mean_2d(self): # Running zonal_mean_2d with the core param only. self.runner.sets_to_run = ["zonal_mean_2d"] From d19d4dab5908cbe6b9a8b109416f8f64e8370c3a Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 11:22:36 -0800 Subject: [PATCH 14/31] Refactor how cfg parameters are set --- e3sm_diags/parser/core_parser.py | 9 +- e3sm_diags/run.py | 116 +++++++++++------- .../test_all_sets_modified_image_counts.py | 3 + tests/integration/test_run.py | 23 +++- 4 files changed, 101 insertions(+), 50 deletions(-) diff --git a/e3sm_diags/parser/core_parser.py b/e3sm_diags/parser/core_parser.py index 72f168f4a..feae9ed60 100644 --- a/e3sm_diags/parser/core_parser.py +++ b/e3sm_diags/parser/core_parser.py @@ -672,6 +672,8 @@ def get_parameters( ): """ Get the parameters based on the command line arguments and return a list of them. + + # TODO: This code is really confusing and should be refactored -- Tom """ if not cmdline_parameters: cmdline_parameters = self._get_cmdline_parameters(*args, **kwargs) @@ -700,7 +702,6 @@ def get_parameters( final_parameters = [orig_parameters] elif cmdline_parameters: final_parameters = [cmdline_parameters] - # User didn't give any command line options, so create a parameter from the # defaults of the command line argument or the Parameter class. elif cmd_default_vars: @@ -724,6 +725,7 @@ def get_parameters( # Sometimes, one of these can be None, so get the one that's None. parameter = parameter if parameter else cmdline_parameter + # FIXME: This returns an empty list because final_parameters = self.select(parameter, final_parameters) self._add_aliases(final_parameters) @@ -784,7 +786,7 @@ def get_cfg_parameters( RuntimeError If the parameters input file is not `.json` or `.cfg` format. """ - params = [] + all_params = [] self._parse_arguments() @@ -797,10 +799,11 @@ def get_cfg_parameters( params = self._get_cfg_parameters( diags_file, check_values, argparse_vals_only ) + all_params.extend(params) else: raise RuntimeError("The parameters input file must be a .cfg file") - return params + return all_params def _get_cfg_parameters( self, cfg_file, check_values=False, argparse_vals_only=True diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 62792fa38..c8c9fe11d 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -3,7 +3,7 @@ from typing import List, Union import e3sm_diags # noqa: F401 -from e3sm_diags.e3sm_diags_driver import main +from e3sm_diags.e3sm_diags_driver import get_default_diags_path, main from e3sm_diags.logger import custom_logger, move_log_to_prov_dir from e3sm_diags.parameter import DEFAULT_SETS, SET_TO_PARAMETERS from e3sm_diags.parameter.core_parameter import CoreParameter @@ -21,6 +21,9 @@ class Run: def __init__(self): self.parser = CoreParser() + args = self.parser.view_args() + + self.has_cfg_params = len(args.other_parameters) > 0 # The list of sets to run using parameter objects. self.sets_to_run = [] @@ -55,7 +58,7 @@ def run_diags( If a diagnostic run using a parameter fails for any reason. """ - params = self.get_final_parameters(parameters, debug) + params = self.get_run_parameters(parameters, debug) if params is None or len(params) == 0: raise RuntimeError( @@ -72,12 +75,10 @@ def run_diags( return params_results - def get_final_parameters( - self, parameters: List[CoreParameter], debug: bool = False - ): + def get_run_parameters(self, parameters: List[CoreParameter], debug: bool = False): """ - Based on sets_to_run and the list of parameters, - get the final list of paremeters to run the diags on. + Based on sets_to_run and the list of parameters, get the final list of + paremeters to run the diags on. """ self._validate_parameters(parameters) @@ -89,14 +90,14 @@ def get_final_parameters( # over `run_diags()` function and run one parameter at a time. self._add_parent_attrs_to_children(parameters) - if debug: - final_params = self._get_debug_parameters(parameters) + if not debug: + run_params = self._get_cfg_parameters(parameters) else: - final_params = self._get_run_parameters(parameters) + run_params = self._get_debug_parameters(parameters) - self.parser.check_values_of_params(final_params) + self.parser.check_values_of_params(run_params) - return final_params + return run_params def _validate_parameters(self, parameters: List[CoreParameter]): if parameters is None or not isinstance(parameters, list): @@ -112,7 +113,7 @@ def _validate_parameters(self, parameters: List[CoreParameter]): "You passed in two or more non-CoreParameter objects of the same type." ) - def _get_run_parameters( + def _get_cfg_parameters( self, parameters: List[CoreParameter] ) -> List[CoreParameter]: """Get the run parameters using all sets and a .cfg file if passed. @@ -131,42 +132,93 @@ def _get_run_parameters( """ run_params = [] + # Get parameters from user-defined .cfg file or default diags .cfg + # file. + if self.has_cfg_params: + cfg_params = self._get_diags_from_cfg_file() + else: + run_type = parameters[0].run_type + cfg_params = self._get_default_diags_from_cfg_file(run_type) + + # Loop over the sets to run and get the related parameters. if len(self.sets_to_run) == 0: self.sets_to_run = DEFAULT_SETS for set_name in self.sets_to_run: - other_params = self._get_diags_from_cfg_file() - # For each of the set_names, get the corresponding parameter. param = self._get_instance_of_param_class( SET_TO_PARAMETERS[set_name], parameters ) - # Since each parameter will have lots of default values, we want to remove them. - # Otherwise when calling get_parameters(), these default values - # will take precedence over values defined in other_params. + # Since each parameter will have lots of default values, we want to + # remove them. Otherwise when calling get_parameters(), these + # default values will take precedence over values defined in + # other_params. self._remove_attrs_with_default_values(param) param.sets = [set_name] params = self.parser.get_parameters( orig_parameters=param, - other_parameters=other_params, + other_parameters=cfg_params, cmd_default_vars=False, argparse_vals_only=False, ) - # Makes sure that any parameters that are selectors - # will be in param. + # Makes sure that any parameters that are selectors will be in param. self._add_attrs_with_default_values(param) + # The select() call in get_parameters() was made for the original - # command-line way of using CDP. - # We just call it manually with the parameter object param. + # command-line way of using CDP. We just call it manually with the + # parameter object param. params = self.parser.select(param, params) run_params.extend(params) return run_params + def _get_diags_from_cfg_file(self) -> Union[List, List[CoreParameter]]: + """ + Get parameters defined by the cfg file passed to -d/--diags (if set). + """ + params = self.parser.get_cfg_parameters(argparse_vals_only=False) + + params_final = self._convert_params_to_subclass(params) + + return params_final + + def _get_default_diags_from_cfg_file(self, run_type): + # Get the paths to the default diags .cfg file(s). + paths = [] + for set_name in self.sets_to_run: + path = get_default_diags_path(set_name, run_type, False) + paths.append(path) + + # Convert the .cfg file(s) to parameter objects. + params = self.parser.get_cfg_parameters( + files_to_open=paths, argparse_vals_only=False + ) + + # Update parameter objects using subclass with default values. + params_final = self._convert_params_to_subclass(params) + + return params_final + + def _convert_params_to_subclass( + self, + parameters: List[CoreParameter], + ) -> List[CoreParameter]: + new_params: List[CoreParameter] = [] + + # For each of the params, add in the default values using the parameter + # classes in SET_TO_PARAMETERS. + for param in parameters: + set_key = param.sets[0] + new_param = SET_TO_PARAMETERS[set_key]() + param + + new_params.append(new_param) + + return new_params + def _get_debug_parameters( self, parameters: List[CoreParameter] ) -> List[CoreParameter]: @@ -360,23 +412,5 @@ def _get_instance_of_param_class(self, cls, parameters): msg = "There's weren't any class of types {} in your parameters." raise RuntimeError(msg.format(class_types)) - def _get_diags_from_cfg_file(self) -> Union[List, List[CoreParameter]]: - """ - Get parameters defined by the cfg file passed to -d/--diags (if set). - - If ``-d`` is not passed, return an empty list. - """ - args = self.parser.view_args() - params = [] - - if args.other_parameters: - params = self.parser.get_cfg_parameters(argparse_vals_only=False) - # For each of the params, add in the default values - # using the parameter classes in SET_TO_PARAMETERS. - for i in range(len(params)): - params[i] = SET_TO_PARAMETERS[params[i].sets[0]]() + params[i] - - return params - runner = Run() diff --git a/tests/integration/test_all_sets_modified_image_counts.py b/tests/integration/test_all_sets_modified_image_counts.py index ab2bf9a73..8e70a7e31 100644 --- a/tests/integration/test_all_sets_modified_image_counts.py +++ b/tests/integration/test_all_sets_modified_image_counts.py @@ -19,6 +19,9 @@ def test_all_sets_modified_produces_the_expected_number_of_images(): params = _convert_cfg_to_param_objs(CFG_PATH) params_results: List[CoreParameter] = [] + # FIXME: Test is fialing + # https://github.com/E3SM-Project/e3sm_diags/actions/runs/7040742260/job/19162184070#step:8:1232 + for param in params: result = runner.run_diags([param], debug=True) params_results.extend(result) diff --git a/tests/integration/test_run.py b/tests/integration/test_run.py index cdbf77576..96a08e06f 100644 --- a/tests/integration/test_run.py +++ b/tests/integration/test_run.py @@ -35,7 +35,7 @@ def setUp(self): def test_lat_lon_ann(self): self.core_param.seasons = ["ANN"] self.runner.sets_to_run = ["lat_lon"] - parameters = self.runner.get_final_parameters([self.core_param]) + parameters = self.runner.get_run_parameters([self.core_param]) for param in parameters: bad_seasons = ["DJF", "MAM", "JJA", "SON"] @@ -64,8 +64,8 @@ def test_all_sets_and_all_seasons(self): "streamflow", ] - parameters = self.runner.get_final_parameters( - [self.core_param, ts_param, enso_param, streamflow_param], debug=True + parameters = self.runner.get_run_parameters( + [self.core_param, ts_param, enso_param, streamflow_param] ) # Counts the number of each set and each seasons to run the diags on. set_counter, season_counter = ( @@ -84,16 +84,27 @@ def test_all_sets_and_all_seasons(self): msg = "Count for {} is invalid: {}" self.fail(msg.format(set_name, count)) + # enso_diags and streamflow only run ANN, no seasons + # So, reduce the ANN count by the number of times these appear + season_counter["ANN"] -= set_counter["enso_diags"] + season_counter["ANN"] -= set_counter["streamflow"] + if not all(season_counter["ANN"] == count for count in season_counter.values()): + self.fail( + "In .cfg files, at least one season does not match the count for ANN: {}".format( + season_counter + ) + ) + def test_zonal_mean_2d(self): # Running zonal_mean_2d with the core param only. self.runner.sets_to_run = ["zonal_mean_2d"] - core_only_results = self.runner.get_final_parameters([self.core_param]) + core_only_results = self.runner.get_run_parameters([self.core_param]) # Running zonal_mean_2d with a set-specific param. # We pass in both the core and this parameter. zonal_mean_2d_param = ZonalMean2dParameter() zonal_mean_2d_param.plevs = [10.0, 20.0, 30.0] - both_results = self.runner.get_final_parameters( + both_results = self.runner.get_run_parameters( [self.core_param, zonal_mean_2d_param] ) @@ -110,7 +121,7 @@ def test_zonal_mean_2d(self): another_zonal_mean_2d_param.reference_data_path = "/something" another_zonal_mean_2d_param.test_data_path = "/something/else" another_zonal_mean_2d_param.results_dir = "/something/else/too" - zm_2d_only_results = self.runner.get_final_parameters( + zm_2d_only_results = self.runner.get_run_parameters( [another_zonal_mean_2d_param] ) From 7c9c6071637e36da65025fdcf415d630a32bad40 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 12:00:25 -0800 Subject: [PATCH 15/31] Fix removal of CoreParameter default sets breaking integration test --- e3sm_diags/parameter/__init__.py | 7 ------- e3sm_diags/parameter/core_parameter.py | 29 ++++++++++++++++++++++++-- e3sm_diags/parser/core_parser.py | 11 ++++++---- e3sm_diags/run.py | 5 +++-- 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/e3sm_diags/parameter/__init__.py b/e3sm_diags/parameter/__init__.py index e846fbeeb..a09a9f393 100644 --- a/e3sm_diags/parameter/__init__.py +++ b/e3sm_diags/parameter/__init__.py @@ -36,10 +36,3 @@ "aerosol_budget": CoreParameter, "mp_partition": MPpartitionParameter, } - - -# FIXME: mp_partition was not originally included as a default set -# in `CoreParameter.sets`. Including it will break an integration -# test, so it needs to be removed. -DEFAULT_SETS = list(SET_TO_PARAMETERS.keys()) -DEFAULT_SETS = [key for key in DEFAULT_SETS if key != "mp_partition"] diff --git a/e3sm_diags/parameter/core_parameter.py b/e3sm_diags/parameter/core_parameter.py index 6b677a38f..03744f8d2 100644 --- a/e3sm_diags/parameter/core_parameter.py +++ b/e3sm_diags/parameter/core_parameter.py @@ -9,6 +9,31 @@ class CoreParameter: + # FIXME: mp_partition was not originally included as a default set + # in `CoreParameter.sets`. Including it will break an integration + # test, so it needs to be removed. + DEFAULT_SETS = [ + "zonal_mean_xy", + "zonal_mean_2d", + "zonal_mean_2d_stratosphere", + "meridional_mean_2d", + "lat_lon", + "polar", + "area_mean_time_series", + "cosp_histogram", + "enso_diags", + "qbo", + "streamflow", + "diurnal_cycle", + "arm_diags", + "tc_analysis", + "annual_cycle_zonal_mean", + "lat_lon_land", + "lat_lon_river", + "aerosol_aeronet", + "aerosol_budget", + ] + def __init__(self): # File I/O # ------------------------ @@ -58,8 +83,8 @@ def __init__(self): # 'model_vs_obs' (by default), 'model_vs_model', or 'obs_vs_obs'. self.run_type: str = "model_vs_obs" - # A list of the sets to be run. - self.sets: List[str] = [] + # A list of the sets to be run, by default all sets. + self.sets: List[str] = CoreParameter.DEFAULT_SETS # The current set that is being ran when looping over sets in # `e3sm_diags_driver.run_diag()`. diff --git a/e3sm_diags/parser/core_parser.py b/e3sm_diags/parser/core_parser.py index feae9ed60..05cffc20c 100644 --- a/e3sm_diags/parser/core_parser.py +++ b/e3sm_diags/parser/core_parser.py @@ -960,13 +960,16 @@ def is_subset(param1, param2): final_parameters = [] for param in parameters: - if all( - is_subset( + has_attrs = [] + + for select_parameter in selectors: + has_attr = is_subset( getattr(param, select_parameter), getattr(main_parameters, select_parameter), ) - for select_parameter in selectors - ): + has_attrs.append(has_attr) + + if all(has_attrs): final_parameters.append(param) return final_parameters diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index c8c9fe11d..9ccf112fb 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -5,7 +5,7 @@ import e3sm_diags # noqa: F401 from e3sm_diags.e3sm_diags_driver import get_default_diags_path, main from e3sm_diags.logger import custom_logger, move_log_to_prov_dir -from e3sm_diags.parameter import DEFAULT_SETS, SET_TO_PARAMETERS +from e3sm_diags.parameter import SET_TO_PARAMETERS from e3sm_diags.parameter.core_parameter import CoreParameter from e3sm_diags.parser.core_parser import CoreParser @@ -142,7 +142,7 @@ def _get_cfg_parameters( # Loop over the sets to run and get the related parameters. if len(self.sets_to_run) == 0: - self.sets_to_run = DEFAULT_SETS + self.sets_to_run = CoreParameter.DEFAULT_SETS for set_name in self.sets_to_run: # For each of the set_names, get the corresponding parameter. @@ -170,6 +170,7 @@ def _get_cfg_parameters( # The select() call in get_parameters() was made for the original # command-line way of using CDP. We just call it manually with the # parameter object param. + # FIXME: Something is going on here where no parameters are being returned. params = self.parser.select(param, params) run_params.extend(params) From 4c0a057c330e679685035a255ebd00a9e05c470b Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 12:26:16 -0800 Subject: [PATCH 16/31] Fix class property and default sets --- e3sm_diags/parameter/core_parameter.py | 53 ++++++++++--------- e3sm_diags/run.py | 16 +++--- .../integration/test_all_sets_image_diffs.py | 3 +- 3 files changed, 39 insertions(+), 33 deletions(-) diff --git a/e3sm_diags/parameter/core_parameter.py b/e3sm_diags/parameter/core_parameter.py index 03744f8d2..5382c74c1 100644 --- a/e3sm_diags/parameter/core_parameter.py +++ b/e3sm_diags/parameter/core_parameter.py @@ -7,33 +7,34 @@ logger = custom_logger(__name__) +# FIXME: There is probably a better way of defining default sets because most of +# this is repeated in SETS_TO_PARAMETERS and SETS_TO_PARSERS. +# Also integration tests will break if "mp_partition" is included because +# we did not take it into account yet. +DEFAULT_SETS = [ + "zonal_mean_xy", + "zonal_mean_2d", + "zonal_mean_2d_stratosphere", + "meridional_mean_2d", + "lat_lon", + "polar", + "area_mean_time_series", + "cosp_histogram", + "enso_diags", + "qbo", + "streamflow", + "diurnal_cycle", + "arm_diags", + "tc_analysis", + "annual_cycle_zonal_mean", + "lat_lon_land", + "lat_lon_river", + "aerosol_aeronet", + "aerosol_budget", +] -class CoreParameter: - # FIXME: mp_partition was not originally included as a default set - # in `CoreParameter.sets`. Including it will break an integration - # test, so it needs to be removed. - DEFAULT_SETS = [ - "zonal_mean_xy", - "zonal_mean_2d", - "zonal_mean_2d_stratosphere", - "meridional_mean_2d", - "lat_lon", - "polar", - "area_mean_time_series", - "cosp_histogram", - "enso_diags", - "qbo", - "streamflow", - "diurnal_cycle", - "arm_diags", - "tc_analysis", - "annual_cycle_zonal_mean", - "lat_lon_land", - "lat_lon_river", - "aerosol_aeronet", - "aerosol_budget", - ] +class CoreParameter: def __init__(self): # File I/O # ------------------------ @@ -84,7 +85,7 @@ def __init__(self): self.run_type: str = "model_vs_obs" # A list of the sets to be run, by default all sets. - self.sets: List[str] = CoreParameter.DEFAULT_SETS + self.sets: List[str] = DEFAULT_SETS # The current set that is being ran when looping over sets in # `e3sm_diags_driver.run_diag()`. diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 9ccf112fb..64f2db7bd 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -6,7 +6,7 @@ from e3sm_diags.e3sm_diags_driver import get_default_diags_path, main from e3sm_diags.logger import custom_logger, move_log_to_prov_dir from e3sm_diags.parameter import SET_TO_PARAMETERS -from e3sm_diags.parameter.core_parameter import CoreParameter +from e3sm_diags.parameter.core_parameter import DEFAULT_SETS, CoreParameter from e3sm_diags.parser.core_parser import CoreParser logger = custom_logger(__name__) @@ -21,9 +21,6 @@ class Run: def __init__(self): self.parser = CoreParser() - args = self.parser.view_args() - - self.has_cfg_params = len(args.other_parameters) > 0 # The list of sets to run using parameter objects. self.sets_to_run = [] @@ -134,7 +131,8 @@ def _get_cfg_parameters( # Get parameters from user-defined .cfg file or default diags .cfg # file. - if self.has_cfg_params: + + if self.has_cfg_file_arg: cfg_params = self._get_diags_from_cfg_file() else: run_type = parameters[0].run_type @@ -142,7 +140,7 @@ def _get_cfg_parameters( # Loop over the sets to run and get the related parameters. if len(self.sets_to_run) == 0: - self.sets_to_run = CoreParameter.DEFAULT_SETS + self.sets_to_run = DEFAULT_SETS for set_name in self.sets_to_run: # For each of the set_names, get the corresponding parameter. @@ -177,6 +175,12 @@ def _get_cfg_parameters( return run_params + @property + def has_cfg_file_arg(self): + args = self.parser.view_args() + + self.has_cfg_params = len(args.other_parameters) > 0 + def _get_diags_from_cfg_file(self) -> Union[List, List[CoreParameter]]: """ Get parameters defined by the cfg file passed to -d/--diags (if set). diff --git a/tests/integration/test_all_sets_image_diffs.py b/tests/integration/test_all_sets_image_diffs.py index 7ac3bd284..eba4e428a 100644 --- a/tests/integration/test_all_sets_image_diffs.py +++ b/tests/integration/test_all_sets_image_diffs.py @@ -12,7 +12,8 @@ from tests.integration.config import TEST_IMAGES_PATH, TEST_ROOT_PATH from tests.integration.utils import _get_test_params -CFG_PATH = f"{TEST_ROOT_PATH}/all_sets.cfg" +CFG_PATH = os.path.join(TEST_ROOT_PATH, "all_sets.cfg") + logger = custom_logger(__name__) From 63e29bf523d66f24cc4376195cb8842fed2d9406 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 12:27:33 -0800 Subject: [PATCH 17/31] Fix property --- e3sm_diags/run.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 64f2db7bd..c1869ca8d 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -25,6 +25,19 @@ def __init__(self): # The list of sets to run using parameter objects. self.sets_to_run = [] + @property + def has_cfg_file_arg(self): + """A property to check if `-d/--diags` was set to a `.cfg` filepath. + + Returns + ------- + bool + True if list contains more than one path, else False. + """ + args = self.parser.view_args() + + return len(args.other_parameters) > 0 + def run_diags( self, parameters: List[CoreParameter], debug: bool = False ) -> List[CoreParameter]: @@ -175,12 +188,6 @@ def _get_cfg_parameters( return run_params - @property - def has_cfg_file_arg(self): - args = self.parser.view_args() - - self.has_cfg_params = len(args.other_parameters) > 0 - def _get_diags_from_cfg_file(self) -> Union[List, List[CoreParameter]]: """ Get parameters defined by the cfg file passed to -d/--diags (if set). From 4e24fb22c554dd50d1c721312046e9bdcc3cc9df Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 12:28:33 -0800 Subject: [PATCH 18/31] Update docstring --- e3sm_diags/run.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index c1869ca8d..51110ea99 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -249,12 +249,6 @@ def _get_debug_parameters( List[CoreParameter] A list of parameter objects. Any non-CoreParameter objects will be replaced by a sub-class based on the set (``SETS_TO_PARAMETERS``). - - Notes - ----- - This method is implemented to avoid any breaking changes or inadvertent - side-effects of modifying `_get_run_parameters()`. In the future, it - is advisable to refactor both methods as a single method. """ debug_params = [] From 4d562ce4cf188336e92cacce80264f45fc07904e Mon Sep 17 00:00:00 2001 From: Tom Vo Date: Thu, 30 Nov 2023 12:29:01 -0800 Subject: [PATCH 19/31] Update e3sm_diags/run.py --- e3sm_diags/run.py | 1 - 1 file changed, 1 deletion(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 51110ea99..bf8b219de 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -181,7 +181,6 @@ def _get_cfg_parameters( # The select() call in get_parameters() was made for the original # command-line way of using CDP. We just call it manually with the # parameter object param. - # FIXME: Something is going on here where no parameters are being returned. params = self.parser.select(param, params) run_params.extend(params) From c2b5cf9b75e1363e3ca3706216483a2ae533cee0 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 12:40:41 -0800 Subject: [PATCH 20/31] Fix comments --- e3sm_diags/parser/core_parser.py | 1 - e3sm_diags/run.py | 43 ++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/e3sm_diags/parser/core_parser.py b/e3sm_diags/parser/core_parser.py index 05cffc20c..88521ccd1 100644 --- a/e3sm_diags/parser/core_parser.py +++ b/e3sm_diags/parser/core_parser.py @@ -725,7 +725,6 @@ def get_parameters( # Sometimes, one of these can be None, so get the one that's None. parameter = parameter if parameter else cmdline_parameter - # FIXME: This returns an empty list because final_parameters = self.select(parameter, final_parameters) self._add_aliases(final_parameters) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index bf8b219de..84cc20aae 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -1,6 +1,6 @@ import copy from itertools import chain -from typing import List, Union +from typing import List import e3sm_diags # noqa: F401 from e3sm_diags.e3sm_diags_driver import get_default_diags_path, main @@ -53,9 +53,10 @@ def run_diags( * If True, only the parameters passed via ``parameters`` will be run. The sets to run are based on the sets defined by the parameters. This makes it easy to debug a few sets. - * If False, run all sets using the list of parameters passed in this - function and parameters defined in a .cfg file (if defined). - This is the default option. + * If False, run all sets using the list of parameters passed in + this function and parameters defined in a .cfg file (if + defined), or use the .cfg file(s) for default diagnostics. This + is the default option. Returns ------- @@ -67,7 +68,6 @@ def run_diags( RuntimeError If a diagnostic run using a parameter fails for any reason. """ - params = self.get_run_parameters(parameters, debug) if params is None or len(params) == 0: @@ -142,21 +142,16 @@ def _get_cfg_parameters( """ run_params = [] - # Get parameters from user-defined .cfg file or default diags .cfg - # file. - if self.has_cfg_file_arg: - cfg_params = self._get_diags_from_cfg_file() + cfg_params = self._get_custom_params_from_cfg_file() else: run_type = parameters[0].run_type - cfg_params = self._get_default_diags_from_cfg_file(run_type) + cfg_params = self._get_default_params_from_cfg_file(run_type) - # Loop over the sets to run and get the related parameters. if len(self.sets_to_run) == 0: self.sets_to_run = DEFAULT_SETS for set_name in self.sets_to_run: - # For each of the set_names, get the corresponding parameter. param = self._get_instance_of_param_class( SET_TO_PARAMETERS[set_name], parameters ) @@ -187,9 +182,13 @@ def _get_cfg_parameters( return run_params - def _get_diags_from_cfg_file(self) -> Union[List, List[CoreParameter]]: - """ - Get parameters defined by the cfg file passed to -d/--diags (if set). + def _get_custom_params_from_cfg_file(self) -> List[CoreParameter]: + """Get parameters using the cfg file set by `-d`/`--diags`. + + Returns + ------- + List[CoreParameter] + A list of parameter objects. """ params = self.parser.get_cfg_parameters(argparse_vals_only=False) @@ -197,7 +196,19 @@ def _get_diags_from_cfg_file(self) -> Union[List, List[CoreParameter]]: return params_final - def _get_default_diags_from_cfg_file(self, run_type): + def _get_default_params_from_cfg_file(self, run_type: str) -> List[CoreParameter]: + """Get parameters using the default diagnostic .cfg file(s). + + Parameters + ---------- + run_type : str + The run type used to check for which .cfg file(s) to reference. + + Returns + ------- + List[CoreParameter] + A list of parameter objects. + """ # Get the paths to the default diags .cfg file(s). paths = [] for set_name in self.sets_to_run: From 2942f25f8851d3e3616f1c98d6eaef2acd99bf1a Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 14:18:52 -0800 Subject: [PATCH 21/31] Fix cfg_params being modified in memory --- e3sm_diags/run.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 84cc20aae..335efc3a8 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -163,9 +163,14 @@ def _get_cfg_parameters( self._remove_attrs_with_default_values(param) param.sets = [set_name] + # # FIXME: Make a deep copy of cfg_params because there is some + # buggy code in this method that changes parameter attributes in + # place, which affects downstream operations. The original + # cfg_params needs to be perserved for each iteration of this + # for loop. params = self.parser.get_parameters( orig_parameters=param, - other_parameters=cfg_params, + other_parameters=copy.deepcopy(cfg_params), cmd_default_vars=False, argparse_vals_only=False, ) From 8618ba97cbea33ae92d4ebc9edf571458abe3672 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 15:22:15 -0800 Subject: [PATCH 22/31] Update Makefile commands --- Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 4173c4ba2..a1f478a23 100644 --- a/Makefile +++ b/Makefile @@ -55,9 +55,11 @@ clean-test: ## remove test and coverage artifacts rm -fr .pytest_cache rm -rf .mypy_cache -clean-test-int: +clean-test-int-res: rm -rf tests/integration/all_sets_results_test rm -rf tests/integration/image_check_failures + +clean-test-int-data: rm -rf tests/integration/integration_test_data rm -rf tests/integration/integration_test_images From 4e7bf9cd2bed6b1acc3f0437f4e0d262eb01afee Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 15:52:42 -0800 Subject: [PATCH 23/31] Revert core_parser changes --- e3sm_diags/parser/core_parser.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/e3sm_diags/parser/core_parser.py b/e3sm_diags/parser/core_parser.py index 88521ccd1..77e854237 100644 --- a/e3sm_diags/parser/core_parser.py +++ b/e3sm_diags/parser/core_parser.py @@ -785,7 +785,8 @@ def get_cfg_parameters( RuntimeError If the parameters input file is not `.json` or `.cfg` format. """ - all_params = [] + + parameters = [] self._parse_arguments() @@ -798,11 +799,13 @@ def get_cfg_parameters( params = self._get_cfg_parameters( diags_file, check_values, argparse_vals_only ) - all_params.extend(params) else: raise RuntimeError("The parameters input file must be a .cfg file") - return all_params + for p in params: + parameters.append(p) + + return parameters def _get_cfg_parameters( self, cfg_file, check_values=False, argparse_vals_only=True @@ -959,16 +962,13 @@ def is_subset(param1, param2): final_parameters = [] for param in parameters: - has_attrs = [] - - for select_parameter in selectors: - has_attr = is_subset( + if all( + is_subset( getattr(param, select_parameter), getattr(main_parameters, select_parameter), ) - has_attrs.append(has_attr) - - if all(has_attrs): + for select_parameter in selectors + ): final_parameters.append(param) return final_parameters From 3122650e14315f0f5be67fed0903580884c6edcf Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 16:08:49 -0800 Subject: [PATCH 24/31] Revert formatting changes --- e3sm_diags/parser/core_parser.py | 2 -- e3sm_diags/viewer/main.py | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/e3sm_diags/parser/core_parser.py b/e3sm_diags/parser/core_parser.py index 77e854237..77a786374 100644 --- a/e3sm_diags/parser/core_parser.py +++ b/e3sm_diags/parser/core_parser.py @@ -672,8 +672,6 @@ def get_parameters( ): """ Get the parameters based on the command line arguments and return a list of them. - - # TODO: This code is really confusing and should be refactored -- Tom """ if not cmdline_parameters: cmdline_parameters = self._get_cmdline_parameters(*args, **kwargs) diff --git a/e3sm_diags/viewer/main.py b/e3sm_diags/viewer/main.py index 23b6c786b..4227b3e1e 100644 --- a/e3sm_diags/viewer/main.py +++ b/e3sm_diags/viewer/main.py @@ -113,8 +113,8 @@ def insert_data_in_row(row_obj, name, url): def create_viewer(root_dir, parameters): """ - Based of the parameters, find the files with the certain extension and - create the viewer in root_dir. + Based of the parameters, find the files with the + certain extension and create the viewer in root_dir. """ # Group each parameter object based on the `sets` parameter. set_to_parameters = collections.defaultdict(list) From 720dc10e60df7adfbc814154929611d1b71ce28f Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Thu, 30 Nov 2023 16:11:57 -0800 Subject: [PATCH 25/31] Revert formatting --- e3sm_diags/parser/core_parser.py | 1 + 1 file changed, 1 insertion(+) diff --git a/e3sm_diags/parser/core_parser.py b/e3sm_diags/parser/core_parser.py index 77a786374..d823de253 100644 --- a/e3sm_diags/parser/core_parser.py +++ b/e3sm_diags/parser/core_parser.py @@ -700,6 +700,7 @@ def get_parameters( final_parameters = [orig_parameters] elif cmdline_parameters: final_parameters = [cmdline_parameters] + # User didn't give any command line options, so create a parameter from the # defaults of the command line argument or the Parameter class. elif cmd_default_vars: From 3ae6d548d08adfe06dca6f562b232f04184f4c79 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Mon, 11 Dec 2023 18:16:05 -0600 Subject: [PATCH 26/31] Rename `debug` to `use_cfg` --- Makefile | 4 +- e3sm_diags/run.py | 65 +++++++++++-------- .../integration/test_all_sets_image_diffs.py | 5 +- .../test_all_sets_modified_image_counts.py | 5 +- 4 files changed, 43 insertions(+), 36 deletions(-) diff --git a/Makefile b/Makefile index a1f478a23..9736f7704 100644 --- a/Makefile +++ b/Makefile @@ -55,11 +55,11 @@ clean-test: ## remove test and coverage artifacts rm -fr .pytest_cache rm -rf .mypy_cache -clean-test-int-res: +clean-test-int-res: ## remove integration test results and image check failures rm -rf tests/integration/all_sets_results_test rm -rf tests/integration/image_check_failures -clean-test-int-data: +clean-test-int-data: # remove integration test data and images (expected) -- useful when they are updated rm -rf tests/integration/integration_test_data rm -rf tests/integration/integration_test_images diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 335efc3a8..28f161312 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -39,24 +39,25 @@ def has_cfg_file_arg(self): return len(args.other_parameters) > 0 def run_diags( - self, parameters: List[CoreParameter], debug: bool = False + self, parameters: List[CoreParameter], use_cfg: bool = True ) -> List[CoreParameter]: """Run a set of diagnostics with a list of parameters. Parameters ---------- parameters : List[CoreParameter] - A list of parameters. - debug : bool, optional - Run in debug mode or not, by default False. + A list of parameters defined through the Python API. + use_cfg : bool, optional + Run diagnostics a `.cfg` file, by default True. - * If True, only the parameters passed via ``parameters`` will be - run. The sets to run are based on the sets defined by the - parameters. This makes it easy to debug a few sets. - * If False, run all sets using the list of parameters passed in + * If True, run all sets using the list of parameters passed in this function and parameters defined in a .cfg file (if defined), or use the .cfg file(s) for default diagnostics. This is the default option. + * If False, only the parameters passed via ``parameters`` will be + run. The sets to run are based on the sets defined by the + parameters. This makes it easy to debug a few sets instead of + all of the debug sets too. Returns ------- @@ -68,7 +69,7 @@ def run_diags( RuntimeError If a diagnostic run using a parameter fails for any reason. """ - params = self.get_run_parameters(parameters, debug) + params = self.get_run_parameters(parameters, use_cfg) if params is None or len(params) == 0: raise RuntimeError( @@ -85,10 +86,22 @@ def run_diags( return params_results - def get_run_parameters(self, parameters: List[CoreParameter], debug: bool = False): - """ - Based on sets_to_run and the list of parameters, get the final list of - paremeters to run the diags on. + def get_run_parameters( + self, parameters: List[CoreParameter], use_cfg: bool = True + ) -> List[CoreParameter]: + """Get the run parameters. + + Parameters + ---------- + parameters : List[CoreParameter] + A list of parameters defined through the Python API. + use_cfg : bool, optional + Use parameters defined in a .cfg file too, by default True. + + Returns + ------- + List[CoreParameter] + A list of run parameters. """ self._validate_parameters(parameters) @@ -100,10 +113,10 @@ def get_run_parameters(self, parameters: List[CoreParameter], debug: bool = Fals # over `run_diags()` function and run one parameter at a time. self._add_parent_attrs_to_children(parameters) - if not debug: - run_params = self._get_cfg_parameters(parameters) + if use_cfg: + run_params = self._get_parameters_with_api_and_cfg(parameters) else: - run_params = self._get_debug_parameters(parameters) + run_params = self._get_parameters_with_api_only(parameters) self.parser.check_values_of_params(run_params) @@ -123,20 +136,20 @@ def _validate_parameters(self, parameters: List[CoreParameter]): "You passed in two or more non-CoreParameter objects of the same type." ) - def _get_cfg_parameters( + def _get_parameters_with_api_and_cfg( self, parameters: List[CoreParameter] ) -> List[CoreParameter]: - """Get the run parameters using all sets and a .cfg file if passed. + """Get the run parameters using the Python API and .cfg file. Parameters ---------- parameters : List[CoreParameter] - A list of parameter objects. + A list of parameter objects defined by the Python API. Returns ------- List[CoreParameter] - A list of parameter objects including ones defined in a .cfg file. + A list of run parameters, including ones defined in a .cfg file. Any non-CoreParameter objects will be replaced by a sub-class based on the set (``SETS_TO_PARAMETERS``). """ @@ -231,8 +244,7 @@ def _get_default_params_from_cfg_file(self, run_type: str) -> List[CoreParameter return params_final def _convert_params_to_subclass( - self, - parameters: List[CoreParameter], + self, parameters: List[CoreParameter] ) -> List[CoreParameter]: new_params: List[CoreParameter] = [] @@ -246,10 +258,10 @@ def _convert_params_to_subclass( return new_params - def _get_debug_parameters( + def _get_parameters_with_api_only( self, parameters: List[CoreParameter] ) -> List[CoreParameter]: - """Get the parameters explicitly defined in the Python script only. + """Get the parameters defined through the Python API only. This method replaces CoreParameter objects with the related sub-class for the specified set. @@ -262,8 +274,9 @@ def _get_debug_parameters( Returns ------- List[CoreParameter] - A list of parameter objects. Any non-CoreParameter objects will be - replaced by a sub-class based on the set (``SETS_TO_PARAMETERS``). + A list of parameter objects defined through the Python API only. + Any non-CoreParameter objects will be replaced by a sub-class based + on the set (``SETS_TO_PARAMETERS``). """ debug_params = [] diff --git a/tests/integration/test_all_sets_image_diffs.py b/tests/integration/test_all_sets_image_diffs.py index eba4e428a..b8a195d83 100644 --- a/tests/integration/test_all_sets_image_diffs.py +++ b/tests/integration/test_all_sets_image_diffs.py @@ -48,10 +48,7 @@ class TestAllSetsImageDiffs: def setup(self, run_diags_and_get_results_dir): self.results_dir = run_diags_and_get_results_dir - def test_results_directory_ends_with_specific_directory(self): - assert "all_sets_results_test" in self.results_dir - - def test_actual_images_produced_is_the_same_as_the_expected(self): + def test_num_images_is_the_same_as_the_expected(self): actual_num_images, actual_images = self._count_images_in_dir( f"{TEST_ROOT_PATH}/all_sets_results_test" ) diff --git a/tests/integration/test_all_sets_modified_image_counts.py b/tests/integration/test_all_sets_modified_image_counts.py index 8e70a7e31..f4dd0d791 100644 --- a/tests/integration/test_all_sets_modified_image_counts.py +++ b/tests/integration/test_all_sets_modified_image_counts.py @@ -19,11 +19,8 @@ def test_all_sets_modified_produces_the_expected_number_of_images(): params = _convert_cfg_to_param_objs(CFG_PATH) params_results: List[CoreParameter] = [] - # FIXME: Test is fialing - # https://github.com/E3SM-Project/e3sm_diags/actions/runs/7040742260/job/19162184070#step:8:1232 - for param in params: - result = runner.run_diags([param], debug=True) + result = runner.run_diags([param], use_cfg=True) params_results.extend(result) # The result directory should be the same for all diagnostic sets. From d3e0c9ad81eedc875e197fa872bbfc14d643dbcd Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Tue, 12 Dec 2023 11:27:15 -0800 Subject: [PATCH 27/31] Update use_cfg param in test --- e3sm_diags/plot/cartopy/arm_diags_plot.py | 1 + e3sm_diags/run.py | 6 +++--- tests/integration/test_all_sets_modified_image_counts.py | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/e3sm_diags/plot/cartopy/arm_diags_plot.py b/e3sm_diags/plot/cartopy/arm_diags_plot.py index 4837eab60..cf485dd06 100644 --- a/e3sm_diags/plot/cartopy/arm_diags_plot.py +++ b/e3sm_diags/plot/cartopy/arm_diags_plot.py @@ -174,6 +174,7 @@ def plot_convection_onset_statistics( var_time_absolute = cwv.getTime().asComponentTime() time_interval = int(var_time_absolute[1].hour - var_time_absolute[0].hour) + # FIXME: UnboundLocalError: local variable 'cwv_max' referenced before assignment number_of_bins = int(np.ceil((cwv_max - cwv_min) / bin_width)) bin_center = np.arange( (cwv_min + (bin_width / 2)), diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index 28f161312..aeb2b0ee0 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -26,7 +26,7 @@ def __init__(self): self.sets_to_run = [] @property - def has_cfg_file_arg(self): + def is_cfg_file_arg_set(self): """A property to check if `-d/--diags` was set to a `.cfg` filepath. Returns @@ -48,7 +48,7 @@ def run_diags( parameters : List[CoreParameter] A list of parameters defined through the Python API. use_cfg : bool, optional - Run diagnostics a `.cfg` file, by default True. + Also run diagnostics using a `.cfg` file, by default True. * If True, run all sets using the list of parameters passed in this function and parameters defined in a .cfg file (if @@ -155,7 +155,7 @@ def _get_parameters_with_api_and_cfg( """ run_params = [] - if self.has_cfg_file_arg: + if self.is_cfg_file_arg_set: cfg_params = self._get_custom_params_from_cfg_file() else: run_type = parameters[0].run_type diff --git a/tests/integration/test_all_sets_modified_image_counts.py b/tests/integration/test_all_sets_modified_image_counts.py index f4dd0d791..9960f363b 100644 --- a/tests/integration/test_all_sets_modified_image_counts.py +++ b/tests/integration/test_all_sets_modified_image_counts.py @@ -20,7 +20,7 @@ def test_all_sets_modified_produces_the_expected_number_of_images(): params_results: List[CoreParameter] = [] for param in params: - result = runner.run_diags([param], use_cfg=True) + result = runner.run_diags([param], use_cfg=False) params_results.extend(result) # The result directory should be the same for all diagnostic sets. From 5eaf7b318c4e2099027db45546399c3375b146ef Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Tue, 12 Dec 2023 12:28:19 -0800 Subject: [PATCH 28/31] Remove `test_all_sets_modified_image_counts.py` - Remove `all_sets_modified.cfg` too Update `build_workflow.yml` to separate testing steps - Move `complete_run.py` to `tests/integration` - Move `_compare_images()` to `tests/integration/utils.py` - Update `testing.rst` with correct path to `complete_run.py` and `pytest` --- .github/workflows/build_workflow.yml | 12 +- docs/source/dev_guide/testing.rst | 10 +- pyproject.toml | 3 +- tests/integration/all_sets.py | 10 - tests/integration/all_sets_modified.cfg | 187 ------------------ tests/{ => integration}/complete_run.py | 2 +- tests/integration/config.py | 4 +- .../integration/test_all_sets_image_diffs.py | 69 +------ .../test_all_sets_modified_image_counts.py | 34 ---- tests/integration/utils.py | 75 +++++++ 10 files changed, 96 insertions(+), 310 deletions(-) delete mode 100644 tests/integration/all_sets.py delete mode 100644 tests/integration/all_sets_modified.cfg rename tests/{ => integration}/complete_run.py (95%) delete mode 100644 tests/integration/test_all_sets_modified_image_counts.py diff --git a/.github/workflows/build_workflow.yml b/.github/workflows/build_workflow.yml index e69d82fff..cbe3de14e 100644 --- a/.github/workflows/build_workflow.yml +++ b/.github/workflows/build_workflow.yml @@ -99,10 +99,18 @@ jobs: mamba info - if: ${{ steps.skip_check.outputs.should_skip != 'true' }} - name: Run Tests + name: Run Unit Tests + run: pytest tests/e3sm_diags + + - if: ${{ steps.skip_check.outputs.should_skip != 'true' }} + name: Download Integration Test Data + run: python -m tests.integration.download_data + + - if: ${{ steps.skip_check.outputs.should_skip != 'true' }} + name: Run Integration Tests env: CHECK_IMAGES: False - run: bash tests/test.sh + run: pytest tests/integration publish-docs: if: ${{ github.event_name == 'push' }} diff --git a/docs/source/dev_guide/testing.rst b/docs/source/dev_guide/testing.rst index c61a41042..aa21ed3e4 100644 --- a/docs/source/dev_guide/testing.rst +++ b/docs/source/dev_guide/testing.rst @@ -56,7 +56,7 @@ The unit and integration tests are run automatically as part of this. Complete run test ----------------- -``tests/complete_run.py`` checks the images generated by all diagnostics to +``tests/integration/complete_run.py`` checks the images generated by all diagnostics to see if any differ from expected. This test is not run as part of the unit test suite, because it relies on a large quantity of data found on LCRC (Anvil/Chrysalis). @@ -78,7 +78,7 @@ Now that you have your changes on LCRC, enter your development environment. Then .. code:: pip install . # Install your changes - python -m unittest tests/complete_run.py + pytest tests/integration/complete_run.py If this test passes, you're done. If it fails however, that means your code has changed what the output looks like. @@ -108,11 +108,11 @@ then you need to update the expected images. find . -type f -name '*.png' > ../image_list_all_sets.txt cd .. -Run ``python -m unittest tests/complete_run.py`` again. Now, the test should pass. +Run ``pytest tests/integration/complete_run.py`` again. Now, the test should pass. After merging your pull request, edit ``README.md``. The version should be the version of E3SM Diags you ran -``python -m unittest tests/complete_run.py`` with, -the date should be the date you ran ``python -m unittest tests/complete_run.py`` on, +``pytest tests/integration/complete_run.py`` with, +the date should be the date you ran ``pytest tests/integration/complete_run.py`` on, and the hash should be for the top commit shown by ``git log`` or on https://github.com/E3SM-Project/e3sm_diags/commits/main. diff --git a/pyproject.toml b/pyproject.toml index a60a540a1..41a9a2930 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,8 @@ skip = "e3sm_diags/e3sm_diags_driver.py" junit_family = "xunit2" addopts = "--cov=e3sm_diags --cov-report term --cov-report html:tests_coverage_reports/htmlcov --cov-report xml:tests_coverage_reports/coverage.xml -s" python_files = ["tests.py", "test_*.py"] -# testpaths = "tests/e3sm_diags" +# Only run the unit tests because integration tests take a long time. +testpaths = "tests/e3sm_diags" [tool.mypy] # Docs: https://mypy.readthedocs.io/en/stable/config_file.html diff --git a/tests/integration/all_sets.py b/tests/integration/all_sets.py deleted file mode 100644 index f3f5b5f6a..000000000 --- a/tests/integration/all_sets.py +++ /dev/null @@ -1,10 +0,0 @@ -# Running the software with the API: -# python all_sets.py -d all_sets.py -from e3sm_diags.run import Run -from tests.integration.utils import _get_test_params - -run_object = Run() - -params = _get_test_params() - -run_object.run_diags(params) diff --git a/tests/integration/all_sets_modified.cfg b/tests/integration/all_sets_modified.cfg deleted file mode 100644 index 2ec85df7f..000000000 --- a/tests/integration/all_sets_modified.cfg +++ /dev/null @@ -1,187 +0,0 @@ -[zonal_mean_xy] -sets = ["zonal_mean_xy"] -case_id = "ERA-Interim" -variables = ["T"] -seasons = ["ANN"] -plevs = [200.0] - -test_name = "system tests" -short_test_name = "short_system tests" -ref_name = "ERA-Interim" -reference_name = "ERA-Interim Reanalysis 1979-2015" -reference_data_path = "tests/integration/integration_test_data" -ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration/integration_test_data" -test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" - -backend = "mpl" -results_dir = "tests/integration/all_sets_results" -debug = True - - -[zonal_mean_2d] -sets = ["zonal_mean_2d"] -case_id = "ERA-Interim" -variables = ["T"] -seasons = ["ANN"] -contour_levels = [180,185,190,200,210,220,230,240,250,260,270,280,290,295,300] -diff_levels = [-7,-6,-5,-4,-3,-2,-1,1,2,3,4,5,6,7] - -test_name = "system tests" -short_test_name = "short_system tests" -ref_name = "ERA-Interim" -reference_name = "ERA-Interim Reanalysis 1989-2005" -reference_data_path = "tests/integration/integration_test_data" -ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration/integration_test_data" -test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" - -backend = "mpl" -results_dir = "tests/integration/all_sets_results" -debug = True -plot_log_plevs = False -plot_plevs = False - - -[meridional_mean_2d] -sets = ["meridional_mean_2d"] -case_id = "ERA-Interim" -variables = ["T"] -seasons = ["ANN"] -contour_levels = [180,185,190,200,210,220,230,240,250,260,270,280,290,295,300] -diff_levels = [-7,-6,-5,-4,-3,-2,-1,1,2,3,4,5,6,7] - -test_name = "system tests" -short_test_name = "short_system tests" -ref_name = "ERA-Interim" -reference_name = "ERA-Interim Reanalysis 1989-2005" -reference_data_path = "tests/integration/integration_test_data" -ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration/integration_test_data" -test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" - -backend = "mpl" -results_dir = "tests/integration/all_sets_results" -debug = True -plot_log_plevs = False -plot_plevs = False - - -[lat_lon] -sets = ["lat_lon"] -case_id = "ERA-Interim" -variables = ["T"] -seasons = ["ANN"] -plevs = [850.0] -contour_levels = [240, 245, 250, 255, 260, 265, 270, 275, 280, 285, 290, 295] -diff_levels = [-10, -7.5, -5, -4, -3, -2, -1, -0.5, 0.5, 1, 2, 3, 4, 5, 7.5, 10] - -test_name = "system tests" -short_test_name = "short_system tests" -ref_name = "ERA-Interim" -reference_name = "ERA-Interim Reanalysis 1979-2015" -reference_data_path = "tests/integration/integration_test_data" -ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration/integration_test_data" -test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" - -backend = "mpl" -results_dir = "tests/integration/all_sets_results" -debug = True - - -[polar] -sets = ["polar"] -case_id = "ERA-Interim" -variables = ["T"] -seasons = ["ANN"] -regions = ["polar_S"] -plevs = [850.0] -contour_levels = [240, 244, 248, 252, 256, 260, 264, 268, 272] -diff_levels = [-15, -10, -7.5, -5, -2.5, -1, 1, 2.5, 5, 7.5, 10, 15] - -test_name = "system tests" -short_test_name = "short_system tests" -ref_name = "ERA-Interim" -reference_name = "ERA-Interim Reanalysis 1979-2015" -reference_data_path = "tests/integration/integration_test_data" -ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration/integration_test_data" -test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" - -backend = "mpl" -results_dir = "tests/integration/all_sets_results" -debug = True - - -[cosp_histogram] -sets = ["cosp_histogram"] -case_id = "MISR-COSP" -variables = ["COSP_HISTOGRAM_MISR"] -seasons = ["ANN"] -contour_levels = [0,0.5,1.0,1.5,2.0,2.5,3.0,3.5,4.0,4.5,5.0,5.5] -diff_levels = [-3.0,-2.5,-2.0,-1.5,-1.0,-0.5,0,0.5,1.0,1.5,2.0,2.5,3.0] - -test_name = "system tests" -short_test_name = "short_system tests" -ref_name = "MISRCOSP" -reference_name = "MISR COSP (2000-2009)" -reference_data_path = "tests/integration/integration_test_data" -ref_file = "CLDMISR_ERA-Interim_ANN_198001_201401_climo.nc" -test_data_path = "tests/integration/integration_test_data" -test_file = "CLD_MISR_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" - -backend = "mpl" -results_dir = "tests/integration/all_sets_results" -debug = True - - -[area_mean_time_series] -sets = ["area_mean_time_series"] -variables = ["TREFHT"] - -test_name = "system tests" -short_test_name = "short_system tests" -test_data_path = "tests/integration/integration_test_data" -test_file = "TREFHT_201201_201312.nc" -start_yr = '2012' -end_yr = '2013' - -backend = "mpl" -results_dir = "tests/integration/all_sets_results" -debug = True -ref_names = [] -ref_timeseries_input = True -test_timeseries_input = True -ref_start_yr='2012' -ref_end_yr='2013' -test_start_yr='2012' -test_end_yr='2013' - - -[enso_diags] -sets = ["enso_diags"] -case_id = 'TREFHT-response' -debug = True -diff_colormap = "BrBG" -regions = ["20S20N"] -results_dir = "tests/integration/all_sets_results" -seasons = ["ANN"] -start_yr = '2012' -end_yr = '2013' -test_data_path = "tests/integration/integration_test_data" -test_file = 'TREFHT_201201_201312.nc' -reference_data_path = "tests/integration/integration_test_data" -ref_file = 'TREFHT_201201_201312.nc' -test_name = "system tests" -variables = ["TREFHT"] -print_statements = True -nino_region = "NINO34" -ref_timeseries_input = True -test_timeseries_input = True -ref_start_yr='2012' -ref_end_yr='2013' -test_start_yr='2012' -test_end_yr='2013' -backend = "mpl" -plot_type="map" diff --git a/tests/complete_run.py b/tests/integration/complete_run.py similarity index 95% rename from tests/complete_run.py rename to tests/integration/complete_run.py index 1d64f7339..cd8af9f7e 100644 --- a/tests/complete_run.py +++ b/tests/integration/complete_run.py @@ -12,7 +12,7 @@ # This test should be run with the latest E3SM Diags tutorial code. from examples.run_v2_6_0_all_sets_E3SM_machines import run_lcrc -from tests.integration.test_all_sets_image_diffs import _compare_images +from tests.integration.utils import _compare_images class TestCompleteRun: diff --git a/tests/integration/config.py b/tests/integration/config.py index f90f8cb51..7e26cc667 100644 --- a/tests/integration/config.py +++ b/tests/integration/config.py @@ -1,5 +1,5 @@ -# Paths and directories used in the integration test. Configurations in -# `all_sets.cfg` and `all_sets_modified.cfg` should match these paths. +# Paths and directories used in the integration test. All `.cfg` paths +# should align with these (e.g., `all_sets.cfg`). TEST_ROOT_PATH = "tests/integration/" TEST_DATA_DIR = "integration_test_data" TEST_IMAGES_DIR = "integration_test_images" diff --git a/tests/integration/test_all_sets_image_diffs.py b/tests/integration/test_all_sets_image_diffs.py index b8a195d83..2373d2ad4 100644 --- a/tests/integration/test_all_sets_image_diffs.py +++ b/tests/integration/test_all_sets_image_diffs.py @@ -1,16 +1,14 @@ import os import re -import shutil import sys from typing import List import pytest -from PIL import Image, ImageChops, ImageDraw from e3sm_diags.logger import custom_logger from e3sm_diags.run import runner from tests.integration.config import TEST_IMAGES_PATH, TEST_ROOT_PATH -from tests.integration.utils import _get_test_params +from tests.integration.utils import _compare_images, _get_test_params CFG_PATH = os.path.join(TEST_ROOT_PATH, "all_sets.cfg") @@ -375,68 +373,3 @@ def _check_streamflow_plots(self): # Check the full HTML path is the same as the expected. full_html_path = os.path.join(self.results_dir, html_path) self._check_html_image(full_html_path, png_path, full_png_path) - - -def _compare_images( - mismatched_images: List[str], - image_name: str, - path_to_actual_png: str, - path_to_expected_png: str, -) -> List[str]: - # https://stackoverflow.com/questions/35176639/compare-images-python-pil - - actual_png = Image.open(path_to_actual_png).convert("RGB") - expected_png = Image.open(path_to_expected_png).convert("RGB") - diff = ImageChops.difference(actual_png, expected_png) - - diff_dir = f"{TEST_ROOT_PATH}image_check_failures" - if not os.path.isdir(diff_dir): - os.mkdir(diff_dir) - - bbox = diff.getbbox() - # If `diff.getbbox()` is None, then the images are in theory equal - if bbox is None: - pass - else: - # Sometimes, a few pixels will differ, but the two images appear identical. - # https://codereview.stackexchange.com/questions/55902/fastest-way-to-count-non-zero-pixels-using-python-and-pillow - nonzero_pixels = ( - diff.crop(bbox) - .point(lambda x: 255 if x else 0) - .convert("L") - .point(bool) - .getdata() - ) - num_nonzero_pixels = sum(nonzero_pixels) - logger.info("\npath_to_actual_png={}".format(path_to_actual_png)) - logger.info("path_to_expected_png={}".format(path_to_expected_png)) - logger.info("diff has {} nonzero pixels.".format(num_nonzero_pixels)) - width, height = expected_png.size - num_pixels = width * height - logger.info("total number of pixels={}".format(num_pixels)) - fraction = num_nonzero_pixels / num_pixels - logger.info("num_nonzero_pixels/num_pixels fraction={}".format(fraction)) - - # Fraction of mismatched pixels should be less than 0.02% - if fraction >= 0.0002: - mismatched_images.append(image_name) - - simple_image_name = image_name.split("/")[-1].split(".")[0] - shutil.copy( - path_to_actual_png, - os.path.join(diff_dir, "{}_actual.png".format(simple_image_name)), - ) - shutil.copy( - path_to_expected_png, - os.path.join(diff_dir, "{}_expected.png".format(simple_image_name)), - ) - # https://stackoverflow.com/questions/41405632/draw-a-rectangle-and-a-text-in-it-using-pil - draw = ImageDraw.Draw(diff) - (left, upper, right, lower) = diff.getbbox() - draw.rectangle(((left, upper), (right, lower)), outline="red") - diff.save( - os.path.join(diff_dir, "{}_diff.png".format(simple_image_name)), - "PNG", - ) - - return mismatched_images diff --git a/tests/integration/test_all_sets_modified_image_counts.py b/tests/integration/test_all_sets_modified_image_counts.py deleted file mode 100644 index 9960f363b..000000000 --- a/tests/integration/test_all_sets_modified_image_counts.py +++ /dev/null @@ -1,34 +0,0 @@ -import os -import shutil -from typing import List - -from e3sm_diags.parameter.core_parameter import CoreParameter -from e3sm_diags.run import runner -from tests.integration.config import TEST_ROOT_PATH -from tests.integration.utils import _convert_cfg_to_param_objs, _count_images - -# The path to the integration test diagnostics .cfg file. -CFG_PATH = os.path.join(TEST_ROOT_PATH, "all_sets_modified.cfg") -CFG_PATH = os.path.abspath(CFG_PATH) - -# +1 is needed because of the E3SM logo that is used by the viewer HTML. -EXPECTED_NUM_IMAGES = 12 + 1 - - -def test_all_sets_modified_produces_the_expected_number_of_images(): - params = _convert_cfg_to_param_objs(CFG_PATH) - params_results: List[CoreParameter] = [] - - for param in params: - result = runner.run_diags([param], use_cfg=False) - params_results.extend(result) - - # The result directory should be the same for all diagnostic sets. - result_dir = params_results[0].results_dir - num_images = _count_images(result_dir) - - assert num_images == EXPECTED_NUM_IMAGES - - # TODO: Result dir should be set thet temporary pytest location that - # automatically gets cleaned up after every test. - shutil.rmtree(result_dir) diff --git a/tests/integration/utils.py b/tests/integration/utils.py index a7b01585c..be0277fa0 100644 --- a/tests/integration/utils.py +++ b/tests/integration/utils.py @@ -1,9 +1,13 @@ import ast import configparser import os +import shutil import subprocess from typing import List +from PIL import Image, ImageChops, ImageDraw + +from e3sm_diags.logger import custom_logger from e3sm_diags.parameter import SET_TO_PARAMETERS from e3sm_diags.parameter.area_mean_time_series_parameter import ( AreaMeanTimeSeriesParameter, @@ -12,6 +16,9 @@ from e3sm_diags.parameter.enso_diags_parameter import EnsoDiagsParameter from e3sm_diags.parameter.meridional_mean_2d_parameter import MeridionalMean2dParameter from e3sm_diags.parameter.zonal_mean_2d_parameter import ZonalMean2dParameter +from tests.integration.config import TEST_ROOT_PATH + +logger = custom_logger(__name__) def run_cmd_and_pipe_stderr(command: str) -> List[str]: @@ -95,6 +102,9 @@ def _convert_cfg_to_param_objs(cfg_path: str) -> List[CoreParameter]: ------- List[CoreParameter] A list of CoreParameter objects, one for each diagnotic set. + Notes + ----- + This function seems to be a duplicate of `CoreParser._get_cfg_paramters()`'. """ config = configparser.ConfigParser() config.read(cfg_path) @@ -124,3 +134,68 @@ def _count_images(directory: str): count += 1 return count + + +def _compare_images( + mismatched_images: List[str], + image_name: str, + path_to_actual_png: str, + path_to_expected_png: str, +) -> List[str]: + # https://stackoverflow.com/questions/35176639/compare-images-python-pil + + actual_png = Image.open(path_to_actual_png).convert("RGB") + expected_png = Image.open(path_to_expected_png).convert("RGB") + diff = ImageChops.difference(actual_png, expected_png) + + diff_dir = f"{TEST_ROOT_PATH}image_check_failures" + if not os.path.isdir(diff_dir): + os.mkdir(diff_dir) + + bbox = diff.getbbox() + # If `diff.getbbox()` is None, then the images are in theory equal + if bbox is None: + pass + else: + # Sometimes, a few pixels will differ, but the two images appear identical. + # https://codereview.stackexchange.com/questions/55902/fastest-way-to-count-non-zero-pixels-using-python-and-pillow + nonzero_pixels = ( + diff.crop(bbox) + .point(lambda x: 255 if x else 0) + .convert("L") + .point(bool) + .getdata() + ) + num_nonzero_pixels = sum(nonzero_pixels) + logger.info("\npath_to_actual_png={}".format(path_to_actual_png)) + logger.info("path_to_expected_png={}".format(path_to_expected_png)) + logger.info("diff has {} nonzero pixels.".format(num_nonzero_pixels)) + width, height = expected_png.size + num_pixels = width * height + logger.info("total number of pixels={}".format(num_pixels)) + fraction = num_nonzero_pixels / num_pixels + logger.info("num_nonzero_pixels/num_pixels fraction={}".format(fraction)) + + # Fraction of mismatched pixels should be less than 0.02% + if fraction >= 0.0002: + mismatched_images.append(image_name) + + simple_image_name = image_name.split("/")[-1].split(".")[0] + shutil.copy( + path_to_actual_png, + os.path.join(diff_dir, "{}_actual.png".format(simple_image_name)), + ) + shutil.copy( + path_to_expected_png, + os.path.join(diff_dir, "{}_expected.png".format(simple_image_name)), + ) + # https://stackoverflow.com/questions/41405632/draw-a-rectangle-and-a-text-in-it-using-pil + draw = ImageDraw.Draw(diff) + (left, upper, right, lower) = diff.getbbox() + draw.rectangle(((left, upper), (right, lower)), outline="red") + diff.save( + os.path.join(diff_dir, "{}_diff.png".format(simple_image_name)), + "PNG", + ) + + return mismatched_images From 9f1862ad55c0d6870af520ab843964e8278c467a Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Tue, 12 Dec 2023 14:40:42 -0800 Subject: [PATCH 29/31] Remove `test_dataset.py` and move `test_run.py` to unit tests dir - Remove `print_statements = True` in `all_sets.cfg` because test log output gets polluted --- e3sm_diags/run.py | 11 +- pyproject.toml | 1 + tests/{integration => e3sm_diags}/test_run.py | 14 +- tests/integration/all_sets.cfg | 15 -- tests/integration/test_dataset.py | 198 ------------------ 5 files changed, 19 insertions(+), 220 deletions(-) rename tests/{integration => e3sm_diags}/test_run.py (94%) delete mode 100644 tests/integration/test_dataset.py diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index aeb2b0ee0..b5d32aaac 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -25,6 +25,10 @@ def __init__(self): # The list of sets to run using parameter objects. self.sets_to_run = [] + # The path to the user-specified `.cfg` file using `-d/--diags` or + # the default diagnostics `.cfg` file. + self.cfg_path = None + @property def is_cfg_file_arg_set(self): """A property to check if `-d/--diags` was set to a `.cfg` filepath. @@ -35,8 +39,11 @@ def is_cfg_file_arg_set(self): True if list contains more than one path, else False. """ args = self.parser.view_args() + self.cfg_path = args.other_parameters + + is_set = len(self.cfg_path) > 0 - return len(args.other_parameters) > 0 + return is_set def run_diags( self, parameters: List[CoreParameter], use_cfg: bool = True @@ -233,6 +240,8 @@ def _get_default_params_from_cfg_file(self, run_type: str) -> List[CoreParameter path = get_default_diags_path(set_name, run_type, False) paths.append(path) + self.cfg_path = paths + # Convert the .cfg file(s) to parameter objects. params = self.parser.get_cfg_parameters( files_to_open=paths, argparse_vals_only=False diff --git a/pyproject.toml b/pyproject.toml index 41a9a2930..09d466ab5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,6 +38,7 @@ junit_family = "xunit2" addopts = "--cov=e3sm_diags --cov-report term --cov-report html:tests_coverage_reports/htmlcov --cov-report xml:tests_coverage_reports/coverage.xml -s" python_files = ["tests.py", "test_*.py"] # Only run the unit tests because integration tests take a long time. +# Integration tests can be executed manually with `test.sh` or `pytest tests/integration`. testpaths = "tests/e3sm_diags" [tool.mypy] diff --git a/tests/integration/test_run.py b/tests/e3sm_diags/test_run.py similarity index 94% rename from tests/integration/test_run.py rename to tests/e3sm_diags/test_run.py index 96a08e06f..3421b0aaf 100644 --- a/tests/integration/test_run.py +++ b/tests/e3sm_diags/test_run.py @@ -65,7 +65,7 @@ def test_all_sets_and_all_seasons(self): ] parameters = self.runner.get_run_parameters( - [self.core_param, ts_param, enso_param, streamflow_param] + [self.core_param, ts_param, enso_param, streamflow_param], use_cfg=True ) # Counts the number of each set and each seasons to run the diags on. set_counter, season_counter = ( @@ -88,12 +88,14 @@ def test_all_sets_and_all_seasons(self): # So, reduce the ANN count by the number of times these appear season_counter["ANN"] -= set_counter["enso_diags"] season_counter["ANN"] -= set_counter["streamflow"] - if not all(season_counter["ANN"] == count for count in season_counter.values()): - self.fail( - "In .cfg files, at least one season does not match the count for ANN: {}".format( - season_counter + + for season, count in season_counter.items(): + if count != season_counter["ANN"]: + self.fail( + "In .cfg files, at least one season does not match the count for ANN: {}".format( + season_counter + ) ) - ) def test_zonal_mean_2d(self): # Running zonal_mean_2d with the core param only. diff --git a/tests/integration/all_sets.cfg b/tests/integration/all_sets.cfg index 059155714..06277d1e0 100644 --- a/tests/integration/all_sets.cfg +++ b/tests/integration/all_sets.cfg @@ -17,7 +17,6 @@ test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" results_dir = "tests/integration/all_sets_results_test" debug = True - [zonal_mean_2d] sets = ["zonal_mean_2d"] case_id = "ERA-Interim" @@ -38,7 +37,6 @@ test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" results_dir = "tests/integration/all_sets_results_test" debug = True - [meridional_mean_2d] sets = ["meridional_mean_2d"] case_id = "ERA-Interim" @@ -59,7 +57,6 @@ test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" results_dir = "tests/integration/all_sets_results_test" debug = True - [lat_lon] sets = ["lat_lon"] case_id = "ERA-Interim" @@ -82,8 +79,6 @@ results_dir = "tests/integration/all_sets_results_test" debug = True regions=["CONUS_RRM","global"] - - [polar] sets = ["polar"] case_id = "ERA-Interim" @@ -106,7 +101,6 @@ test_file = "T_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" results_dir = "tests/integration/all_sets_results_test" debug = True - [cosp_histogram] sets = ["cosp_histogram"] case_id = "MISR-COSP" @@ -127,7 +121,6 @@ test_file = "CLD_MISR_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc" results_dir = "tests/integration/all_sets_results_test" debug = True - [area_mean_time_series] sets = ["area_mean_time_series"] variables = ["TREFHT"] @@ -158,7 +151,6 @@ reference_data_path = 'tests/integration/integration_test_data' ref_file = 'TREFHT_201201_201312.nc' test_name = "system tests" variables = ["TREFHT"] -print_statements = True [#] sets = ["enso_diags"] @@ -177,7 +169,6 @@ reference_data_path = 'tests/integration/integration_test_data' ref_file = 'TREFHT_201201_201312.nc' test_name = "system tests" variables = ["TREFHT"] -print_statements = True [#] sets = ["enso_diags"] @@ -197,7 +188,6 @@ reference_data_path = 'tests/integration/integration_test_data' ref_file = 'TREFHT_201201_201312.nc' test_name = "system tests" variables = ["TREFHT"] -print_statements = True [#] sets = ["enso_diags"] @@ -214,7 +204,6 @@ reference_data_path = 'tests/integration/integration_test_data' ref_file = 'TREFHT_201201_201312.nc' test_name = "system tests" variables = ["TREFHT"] -print_statements = True [#] sets = ["enso_diags"] @@ -232,7 +221,6 @@ reference_data_path = 'tests/integration/integration_test_data' ref_file = 'TREFHT_201201_201312.nc' test_name = "system tests" variables = ["TREFHT"] -print_statements = True [#] sets = ["enso_diags"] @@ -251,7 +239,6 @@ reference_data_path = 'tests/integration/integration_test_data' ref_file = 'TREFHT_201201_201312.nc' test_name = "system tests" variables = ["TREFHT"] -print_statements = True [qbo] sets = ["qbo"] @@ -283,7 +270,6 @@ test_start_yr = '1959' test_end_yr = '1961' results_dir = "tests/integration/all_sets_results_test" test_name = "system tests" -print_statements = True [diurnal_cycle] sets = ["diurnal_cycle"] @@ -304,7 +290,6 @@ test_file = "20180215.DECKv1b_H1.ne30_oEC.edison.cam.h4_JJA_200006_200908_climo. results_dir = "tests/integration/all_sets_results_test" debug = True - [arm_diags1] sets = ["arm_diags"] diags_set = "annual_cycle" diff --git a/tests/integration/test_dataset.py b/tests/integration/test_dataset.py deleted file mode 100644 index 312d8778c..000000000 --- a/tests/integration/test_dataset.py +++ /dev/null @@ -1,198 +0,0 @@ -import unittest - -import cdms2 - -from e3sm_diags.derivations import acme as acme_derivations -from e3sm_diags.driver.utils.dataset import Dataset -from e3sm_diags.parameter.core_parameter import CoreParameter -from tests.integration.config import TEST_DATA_PATH - - -class TestDataset(unittest.TestCase): - def setUp(self): - self.parameter = CoreParameter() - - def test_convert_units(self): - with cdms2.open(f"{TEST_DATA_PATH}/precc.nc") as precc_file: - var = precc_file("PRECC") - - new_var = acme_derivations.convert_units(var, "mm/day") - self.assertEqual(new_var.units, "mm/day") - - def test_add_user_derived_vars(self): - my_vars = { - "A_NEW_VAR": { - ("v1", "v2"): lambda v1, v2: v1 + v2, - ("v3", "v4"): lambda v3, v4: v3 - v4, - }, - "PRECT": {("MY_PRECT",): lambda my_prect: my_prect}, - } - self.parameter.derived_variables = my_vars - data = Dataset(self.parameter, test=True) - self.assertTrue("A_NEW_VAR" in data.derived_vars) - - # In the default my_vars, each entry - # ('PRECT', 'A_NEW_VAR', etc) is an OrderedDict. - # We must check that what the user inserted is - # first, so it's used first. - self.assertTrue(list(data.derived_vars["PRECT"].keys())[0] == ("MY_PRECT",)) - - def test_is_timeseries(self): - self.parameter.ref_timeseries_input = True - data = Dataset(self.parameter, ref=True) - self.assertTrue(data.is_timeseries()) - - self.parameter.test_timeseries_input = True - data = Dataset(self.parameter, test=True) - self.assertTrue(data.is_timeseries()) - - self.parameter.ref_timeseries_input = False - data = Dataset(self.parameter, ref=True) - self.assertFalse(data.is_timeseries()) - - self.parameter.test_timeseries_input = False - data = Dataset(self.parameter, test=True) - self.assertFalse(data.is_timeseries()) - - def test_is_climo(self): - self.parameter.ref_timeseries_input = True - data = Dataset(self.parameter, ref=True) - self.assertFalse(data.is_climo()) - - self.parameter.test_timeseries_input = True - data = Dataset(self.parameter, test=True) - self.assertFalse(data.is_climo()) - - self.parameter.ref_timeseries_input = False - data = Dataset(self.parameter, ref=True) - self.assertTrue(data.is_climo()) - - self.parameter.test_timeseries_input = False - data = Dataset(self.parameter, test=True) - self.assertTrue(data.is_climo()) - - def test_get_attr_from_climo(self): - # We pass in the path to a file, so the input directory - # to the tests doesn't need to be like how it is for when e3sm_diags - # is ran wit a bunch of diags. - self.parameter.reference_data_path = TEST_DATA_PATH - self.parameter.ref_file = "ta_ERA-Interim_ANN_198001_201401_climo.nc" - data = Dataset(self.parameter, ref=True) - self.assertEqual(data.get_attr_from_climo("Conventions", "ANN"), "CF-1.0") - - """ - def test_process_derived_var_passes(self): - derived_var = { - 'PRECT': { - ('pr'): lambda x: x, - ('PRECC'): lambda x: 'this is some function' - } - } - - precc_file = cdms2.open(get_abs_file_path('integration_test_data/precc.nc')) - acme_derivations.process_derived_var('PRECT', derived_var, - precc_file, self.parameter) - precc_file.close() - - def test_process_derived_var_with_wrong_dict(self): - # pr, nothing, and nothing2 are not variables in the file we open - wrong_derived_var = { - 'PRECT': { - ('pr'): lambda x: x, - ('nothing1', 'nothing2'): lambda x, y: 'this is some function' - } - } - - precc_file = cdms2.open(get_abs_file_path('integration_test_data/precc.nc')) - with self.assertRaises(RuntimeError): - acme_derivations.process_derived_var( - 'PRECT', wrong_derived_var, precc_file, self.parameter) - precc_file.close() - - def test_process_derived_var_adds_to_dict(self): - # the one that's usually in the parameters file - derived_var_dict = { - 'PRECT': {('test'): lambda x: x} - } - # use this instead of the acme.derived_variables one - default_derived_vars = { - 'PRECT': { - ('pr'): lambda x: x, - ('PRECC'): lambda x: 'this is some function' - } - } - - # add derived_var_dict to default_derived_vars - precc_file = cdms2.open(get_abs_file_path('integration_test_data/precc.nc')) - self.parameter.derived_variables = derived_var_dict - acme_derivations.process_derived_var( - 'PRECT', default_derived_vars, precc_file, self.parameter) - precc_file.close() - - if 'test' not in default_derived_vars['PRECT']: - self.fail("Failed to insert test derived variable") - - # make sure that test is the first one - if 'test' != default_derived_vars['PRECT'].keys()[0]: - self.fail( - "Failed to insert test derived variable before default derived vars") - - def test_process_derived_var_adds_duplicate_to_dict(self): - # the one that's usually in the parameters file - # the function for PRECC below is different than the one in - # default_derived_vars - derived_var_dict = { - 'PRECT': {('PRECC'): lambda x: 'PRECC'} - } - # use this instead of the acme_derivations.derived_variables one - default_derived_vars = { - 'PRECT': { - ('pr'): lambda x: x, - ('PRECC'): lambda x: 'this is some function' - } - } - - # add derived_var_dict to default_derived_vars - precc_file = cdms2.open(get_abs_file_path('integration_test_data/precc.nc')) - self.parameter.derived_variables = derived_var_dict - msg = acme_derivations.process_derived_var( - 'PRECT', default_derived_vars, precc_file, self.parameter) - precc_file.close() - if msg != 'PRECC': - self.fail("Failed to insert a duplicate test derived variable") - - def test_process_derived_var_works_with_ordereddict(self): - derived_var_dict = { - 'PRECT': OrderedDict([ - (('something'), lambda x: 'something') - ]) - } - - default_derived_vars = { - 'PRECT': OrderedDict([ - (('pr'), lambda x: x), - (('PRECC'), lambda x: 'this is some function') - ]) - } - - precc_file = cdms2.open(get_abs_file_path('integration_test_data/precc.nc')) - self.parameter.derived_variables = derived_var_dict - acme_derivations.process_derived_var( - 'PRECT', default_derived_vars, precc_file, self.parameter) - precc_file.close() - # Check that 'something' was inserted first - self.assertEqual(['something', 'pr', 'PRECC'], - default_derived_vars['PRECT'].keys()) - - def test_mask_by(self): - with cdms2.open(get_abs_file_path('integration_test_data/precc.nc')) as precc_file: - prcc = precc_file('PRECC') - with cdms2.open(get_abs_file_path('integration_test_data/precl.nc')) as precl_file: - prcl = precl_file('PRECL') - - acme_derivations.mask_by(prcc, prcl, low_limit=2.0) - """ - - -if __name__ == "__main__": - unittest.main() From 13547ad0f46b273ed75234eb6a9c361eec1eed70 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Tue, 12 Dec 2023 14:50:53 -0800 Subject: [PATCH 30/31] Update attr comment in Run class --- e3sm_diags/run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index b5d32aaac..ac2e72be9 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -22,7 +22,7 @@ class Run: def __init__(self): self.parser = CoreParser() - # The list of sets to run using parameter objects. + # The list of sets to run based on diagnostic parameters. self.sets_to_run = [] # The path to the user-specified `.cfg` file using `-d/--diags` or From 3ff0aa7dda389acb0992724ca67dc8d2af5383b7 Mon Sep 17 00:00:00 2001 From: tomvothecoder Date: Wed, 13 Dec 2023 10:51:14 -0800 Subject: [PATCH 31/31] Rename `debug_params` to `params` --- e3sm_diags/run.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/e3sm_diags/run.py b/e3sm_diags/run.py index ac2e72be9..c92f5ee83 100644 --- a/e3sm_diags/run.py +++ b/e3sm_diags/run.py @@ -287,7 +287,7 @@ def _get_parameters_with_api_only( Any non-CoreParameter objects will be replaced by a sub-class based on the set (``SETS_TO_PARAMETERS``). """ - debug_params = [] + params = [] if len(self.sets_to_run) == 0: sets_to_run = [param.sets for param in parameters] @@ -295,24 +295,24 @@ def _get_parameters_with_api_only( for set_name in self.sets_to_run: # For each of the set_names, get the corresponding parameter. - api_param = self._get_instance_of_param_class( + param = self._get_instance_of_param_class( SET_TO_PARAMETERS[set_name], parameters ) # Since each parameter will have lots of default values, we want to remove them. # Otherwise when calling get_parameters(), these default values # will take precedence over values defined in other_params. - self._remove_attrs_with_default_values(api_param) - api_param.sets = [set_name] + self._remove_attrs_with_default_values(param) + param.sets = [set_name] # Makes sure that any parameters that are selectors will be in param. - self._add_attrs_with_default_values(api_param) + self._add_attrs_with_default_values(param) - debug_params.append(api_param) + params.append(param) - self.parser.check_values_of_params(debug_params) + self.parser.check_values_of_params(params) - return debug_params + return params def _add_parent_attrs_to_children(self, parameters): """