pylint feedback and tidy up

Author: ilaflott

fre/cmor/cmor_helpers.py

@@ -43,11 +43,11 @@
 import glob
 import json
 import logging
-import numpy as np
 import os
 from pathlib import Path
-from typing import Optional, Any, List, Union
+from typing import Optional, List, Union
 
+import numpy as np
 from netCDF4 import Dataset, Variable
 
 fre_logger = logging.getLogger(__name__)
@@ -351,15 +351,6 @@ def get_json_file_data( json_file_path: Optional[str] = None) -> dict:
         ) from exc
 
 
-def get_exp_cfg_mip_era(json_file_path: Optional[str] = None) -> str:
-    """
-    :param json_file_path: Path to the experiment metadata JSON file.
-    :type json_file_path: str
-    :return: string representing mip era for which the JSON file is intended
-    :rtype: str
-    """
-    mip_era=get_json_file_data(json_file_path)['mip_era']
-
 def update_grid_and_label( json_file_path: str,
                            new_grid_label: str,
                            new_grid: str,
@@ -419,7 +410,7 @@ def update_grid_and_label( json_file_path: str,
             fre_logger.info('Updated "nominal_resolution": %s', data["nominal_resolution"])
         except KeyError as e:
             fre_logger.error("Failed to update 'nominal_resolution': %s", e)
-            raise KeyError("Error while updating 'nominal_resolution'. Ensure the field exists and is modifiable.") from e
+            raise KeyError("Error updating 'nominal_resolution'. Ensure the field exists and is modifiable.") from e
 
         output_file_path = output_file_path or json_file_path
 
@@ -554,7 +545,7 @@ def conv_mip_to_bronx_freq(cmor_table_freq: str) -> Optional[str]:
     }
     bronx_freq = cmor_to_bronx_dict.get(cmor_table_freq)
     if bronx_freq is None:
-        fre_logger.warning(f'MIP table frequency = {cmor_table_freq} does not have a FRE-bronx equivalent')
+        fre_logger.warning('MIP table frequency = %s does not have a FRE-bronx equivalent', cmor_table_freq)
     if cmor_table_freq not in cmor_to_bronx_dict.keys():
         raise KeyError(f'MIP table frequency = "{cmor_table_freq}" is not a valid MIP frequency')
     return bronx_freq

fre/cmor/cmor_mixer.py

@@ -42,8 +42,7 @@
 
 from .cmor_helpers import ( print_data_minmax, from_dis_gimme_dis, find_statics_file, create_lev_bnds,
                             get_iso_datetime_ranges, check_dataset_for_ocean_grid, get_vertical_dimension,
-                            create_tmp_dir, get_json_file_data, update_grid_and_label, update_calendar_type,
-                            get_exp_cfg_mip_era )
+                            create_tmp_dir, get_json_file_data, update_grid_and_label, update_calendar_type )
 
 fre_logger = logging.getLogger(__name__)
 
@@ -151,7 +150,8 @@ def rewrite_netcdf_file_var( mip_var_cfgs: dict = None,
                 fre_logger.error('should not happen')
                 raise ValueError
         else:
-            fre_logger.error('cmip7 case detected but no brands found in the CMOR mip table config had the right dimensions. nothing to do for this var!')
+            fre_logger.error('cmip7 case detected, but dimensions of input data do not match '
+                             'any of those found for the associated brands.')
             raise ValueError
     else:
         fre_logger.debug('non-cmip7 case detected, skipping variable brands')
@@ -621,9 +621,9 @@ def rewrite_netcdf_file_var( mip_var_cfgs: dict = None,
         units = mip_var_cfgs["variable_entry"][target_var]["units"]
     fre_logger.info("units = %s", units)
 
-    #positive = mip_var_cfgs["variable_entry"][target_var]["positive"] 
+    #positive = mip_var_cfgs["variable_entry"][target_var]["positive"]
     if GLOBAL_MIP_ERA == 'CMIP7':
-       positive = mip_var_cfgs["variable_entry"][f'{target_var}_{var_brand}']["positive"]
+        positive = mip_var_cfgs["variable_entry"][f'{target_var}_{var_brand}']["positive"]
     else:
         positive = mip_var_cfgs["variable_entry"][target_var]["positive"]
     fre_logger.info("positive = %s", positive)
@@ -890,8 +890,6 @@ def cmorize_all_variables_in_dir(vars_to_run: Dict[str, Any],
         except Exception as exc: #uncovered
             return_status = 1
             fre_logger.warning('!!!EXCEPTION CAUGHT!!!   !!!READ THE NEXT LINE!!!')
-#            if str(exc) == "made it to break-point for current work, good job": # inl: annoying and i did this to myself
-#                assert False, "made it to break-point for current work, good job"
             fre_logger.warning('exc=%s', exc)
             fre_logger.warning('this message came from within cmorize_target_var_files')
             fre_logger.warning('COULD NOT PROCESS: %s/%s...moving on', local_var, target_var)
@@ -976,17 +974,20 @@ def cmor_run_subtool(indir: str = None,
     fre_logger.debug('cmip7_case = %s', cmip7_case)
     cmip6_case = exp_cfg_mip_era.upper() == 'CMIP6'
     fre_logger.debug('cmip6_case = %s', cmip6_case)
-    assert cmip7_case != cmip6_case # dont keep this assert, TEMP TODO
 
-    global GLOBAL_MIP_ERA
-    GLOBAL_MIP_ERA = exp_cfg_mip_era # i don't care that i'm changing a "constant", it's more like a static class member
-    if GLOBAL_MIP_ERA is None:
+    if exp_cfg_mip_era is None:
         raise Exception('CMOR input experimental configuration must contain a field called "mip_era"')
-    fre_logger.debug('GLOBAL_MIP_EREA=%s',GLOBAL_MIP_ERA)
 
-    if cmip7_case:
+    if exp_cfg_mip_era.upper() not in ['CMIP6', 'CMIP7']:
+        raise ValueError('cmor_mixer only supports CMIP6 and CMIP7 cases')
+
+    if exp_cfg_mip_era.upper() == 'CMIP7':
         fre_logger.warning('CMIP7 configuration detected, will be expecting and enforcing variable brands.')
 
+    global GLOBAL_MIP_ERA
+    GLOBAL_MIP_ERA = exp_cfg_mip_era.upper() # maybe, change to yet another input arg for down-stream function calls ?
+    fre_logger.debug('GLOBAL_MIP_EREA=%s',GLOBAL_MIP_ERA)
+
     # CHECK optional grid/grid_label/nom_res inputs from exp config, the function raises the potential error conditions
     if any( [ grid_label is not None,
               grid is not None,
@@ -1003,24 +1004,21 @@ def cmor_run_subtool(indir: str = None,
     # open CMOR table config file - need it here for checking the TABLE's variable list
     json_table_config = str(Path(json_table_config).resolve())
     fre_logger.info('loading json_table_config = \n%s', json_table_config)
-
     mip_var_cfgs = get_json_file_data(json_table_config)
     mip_fullvar_list = mip_var_cfgs["variable_entry"].keys()
     fre_logger.debug('the following variables were read from the table: %s', mip_fullvar_list)
-    mip_var_list = None
-    mip_var_brand_list = None
-    if cmip7_case:
-        fre_logger.warning('cmip7 capabilities in-development now. extracting brands from variables within MIP cmor table configs')
+    mip_var_list, mip_var_brand_list = None, None
+    if GLOBAL_MIP_ERA == 'CMIP7':
+        fre_logger.warning('cmip7 capabilities in-development now. extracting brands from variables '
+                           'within MIP cmor table configs')
         mip_var_list = [ var.split('_')[0] for var in mip_fullvar_list ]
         fre_logger.debug('the following unbranded variables were read from the table: %s', mip_var_list)
         mip_var_brand_list = [ var.split('_')[1] for var in mip_fullvar_list ]
         fre_logger.debug('the following brands were extracted from the variables: %s', mip_var_brand_list)
         if len(mip_var_list) != len(mip_var_brand_list):
             raise ValueError('the number of brands is not one-to-one with the number of variables. check config.')
-    elif cmip6_case:
+    elif GLOBAL_MIP_ERA == "CMIP6":
         mip_var_list = mip_fullvar_list
-    #else:
-    #    raise ValueError('uh oh')
 
     fre_logger.debug('variables in mip_var_cfgs["variable_entry"] is = \n %s', mip_var_list)
 

fre/cmor/cmor_yamler.py

@@ -225,7 +225,7 @@ def cmor_yaml_subtool( yamlfile: str = None,
                                  f'    calendar_type={calendar_type} '
                                   ')\n' )
                 continue
-            cmor_run_subtool(
+            cmor_run_subtool( #uncovered
                 indir = indir ,
                 json_var_list = json_var_list ,
                 json_table_config = json_table_config ,

fre/cmor/tests/test_cmor_helpers_update_grid_label.py

@@ -1,6 +1,12 @@
+'''
+unit tests for cmor_helpers.update_grid_and_label
+'''
+
 import json
-import pytest
+
 from pathlib import Path
+import pytest
+
 from fre.cmor.cmor_helpers import update_grid_and_label
 
 # Sample data for testing

fre/cmor/tests/test_cmor_run_subtool.py

@@ -1,4 +1,6 @@
-''' tests for fre.cmor.cmor_run_subtool '''
+'''
+tests for fre.cmor.cmor_run_subtool
+'''
 import subprocess
 import shutil
 from pathlib import Path
@@ -20,7 +22,9 @@
     f'{CMIP6_TABLE_REPO_PATH}/Tables/CMIP6_Omon.json'
 
 def test_setup_cmor_cmip_table_repo():
-    ''' setup routine, make sure the recursively cloned tables exist '''
+    '''
+    setup routine, make sure the recursively cloned tables exist
+    '''
     assert all( [ Path(CMIP6_TABLE_REPO_PATH).exists(),
                   Path(TABLE_CONFIG).exists()
                   ] )
@@ -53,7 +57,8 @@ def test_setup_cmor_cmip_table_repo():
 
 
 def test_setup_fre_cmor_run_subtool(capfd):
-    ''' The routine generates a netCDF file from an ascii (cdl) file. It also checks for a ncgen
+    '''
+    The routine generates a netCDF file from an ascii (cdl) file. It also checks for a ncgen
     output file from prev pytest runs, removes it if it's present, and ensures the new file is
     created without error.
     '''
@@ -285,19 +290,18 @@ def test_git_cleanup():
     git's record of changed files. It's supposed to change as part of the test.
     '''
     is_ci = os.environ.get("GITHUB_WORKSPACE") is not None
-    if is_ci:
-      #doesn't run happily in CI and not needed
-      assert True
-    else:
-      git_cmd = f"git restore {EXP_CONFIG}"
-      restore = subprocess.run(git_cmd,
-                    shell=True,
-                    check=False)
-      check_cmd = f"git status | grep {EXP_CONFIG}"
-      check = subprocess.run(check_cmd,
-                             shell = True, check = False)
-      #first command completed, second found no file in git status
-      assert all([restore.returncode == 0, check.returncode == 1])
+    if not is_ci:
+        git_cmd = f"git restore {EXP_CONFIG}"
+        restore = subprocess.run(git_cmd,
+                                 shell=True,
+                                 check=False)
+        check_cmd = f"git status | grep {EXP_CONFIG}"
+        check = subprocess.run(check_cmd,
+                               shell = True,
+                               check = False)
+        #first command completed, second found no file in git status
+        assert all([restore.returncode == 0,
+                    check.returncode == 1])
 
 def test_cmor_run_subtool_raise_value_error():
     '''

fre/cmor/tests/test_cmor_run_subtool_further_examples.py

@@ -1,24 +1,20 @@
 '''
-expanded set of tests for fre cmor run
-focus on
+expanded set of tests for fre cmor run focus on cases beyond test_cmor_run_subtool.py
 '''
 
 from datetime import date
 from pathlib import Path
 import shutil
-
 import glob
+#import time
+#import platform
+import subprocess
+import os
 
 import pytest
 
 from fre.cmor import cmor_run_subtool
 
-import time
-
-import platform
-
-import subprocess
-import os
 
 # global consts for these tests, with no/trivial impact on the results
 ROOTDIR='fre/tests/test_files'
@@ -187,15 +183,16 @@ def test_git_cleanup():
     git's record of changed files. It's supposed to change as part of the test.
     '''
     is_ci = os.environ.get("GITHUB_WORKSPACE") is not None
-    if is_ci: #git status/restore doesn't run happily in CI and is not needed
-      assert True
-    else:
-      git_cmd = f"git restore {EXP_CONFIG_DEFAULT}"
-      restore = subprocess.run(git_cmd,
-                    shell=True,
-                    check=False)
-      check_cmd = f"git status | grep {EXP_CONFIG_DEFAULT}"
-      check = subprocess.run(check_cmd,
-                             shell = True, check = False)
-      #first command completed, second found no file in git status
-      assert all([restore.returncode == 0, check.returncode == 1])
+    if not is_ci:
+        git_cmd = f"git restore {EXP_CONFIG_DEFAULT}"
+        restore = subprocess.run(git_cmd,
+                                 shell=True,
+                                 check=False)
+        check_cmd = f"git status | grep {EXP_CONFIG_DEFAULT}"
+        check = subprocess.run(check_cmd,
+                               shell = True,
+                               check = False)
+        #first command completed, second found no file in git status
+        assert all( [ restore.returncode == 0,
+                      check.returncode == 1 ] )
+