Repository: ESMValGroup/ESMValCore

Issue:
Use of areacella and areacello in area_statistics not supported for CMIP6
Using the following preprocessor

```yaml
  spatial_mean_sea:
    mask_landsea:
      mask_out: land
    area_statistics:
      operator: mean
      fx_files: ['areacello']
```

fails at the moment:

```
2020-01-15 13:48:55,511 UTC [22398] ERROR   Program terminated abnormally, see stack trace below for more information
Traceback (most recent call last):
  File "ESMValCore/esmvalcore/_main.py", line 229, in run
    conf = main(args)
  File "ESMValCore/esmvalcore/_main.py", line 157, in main
    process_recipe(recipe_file=recipe, config_user=cfg)
  File "ESMValCore/esmvalcore/_main.py", line 203, in process_recipe
    recipe = read_recipe_file(recipe_file, config_user)
  File "ESMValCore/esmvalcore/_recipe.py", line 73, in read_recipe_file
    recipe_file=filename)
  File "ESMValCore/esmvalcore/_recipe.py", line 909, in __init__
    self.tasks = self.initialize_tasks() if initialize_tasks else None
  File "ESMValCore/esmvalcore/_recipe.py", line 1255, in initialize_tasks
    task_name=task_name,
  File "ESMValCore/esmvalcore/_recipe.py", line 880, in _get_preprocessor_task
    name=task_name,
  File "ESMValCore/esmvalcore/_recipe.py", line 738, in _get_single_preprocessor_task
    config_user=config_user,
  File "ESMValCore/esmvalcore/_recipe.py", line 689, in _get_preprocessor_products
    config_user=config_user)
  File "ESMValCore/esmvalcore/_recipe.py", line 478, in _update_fx_settings
    for fxvar in var['fx_files']}
  File "ESMValCore/esmvalcore/_recipe.py", line 478, in <dictcomp>
    for fxvar in var['fx_files']}
  File "ESMValCore/esmvalcore/_recipe.py", line 408, in _get_correct_fx_file
    fx_files = _get_input_files(fx_var, config_user)
UnboundLocalError: local variable 'fx_var' referenced before assignment
```

because certain fx variables are hardcoded in `_recipe.py` for CMIP6. I will take care of that.

Discussion:

Patch:
diff --git a/esmvalcore/_config.py b/esmvalcore/_config.py
index 6f44f1673..c8e08381d 100644
--- a/esmvalcore/_config.py
+++ b/esmvalcore/_config.py
@@ -168,7 +168,10 @@ def configure_logging(cfg_file=None, output=None, console_log_level=None):
 def get_project_config(project):
     """Get developer-configuration for project."""
     logger.debug("Retrieving %s configuration", project)
-    return CFG[project]
+    if project in CFG:
+        return CFG[project]
+    else:
+        raise ValueError(f"Project '{project}' not in config-developer.yml")
 
 
 def get_institutes(variable):
diff --git a/esmvalcore/_recipe.py b/esmvalcore/_recipe.py
index 7b5f194f8..c94022b5b 100644
--- a/esmvalcore/_recipe.py
+++ b/esmvalcore/_recipe.py
@@ -386,26 +386,45 @@ def _add_fxvar_keys(fx_var_dict, variable):
 
 
 def _get_correct_fx_file(variable, fx_varname, config_user):
-    """Wrapper to standard file getter to recover the correct fx file."""
+    """Get fx files (searching all possible mips)."""
+    # TODO: allow user to specify certain mip if desired
     var = dict(variable)
-    if var['project'] in ['CMIP5', 'OBS', 'OBS6', 'obs4mips']:
-        fx_var = _add_fxvar_keys({'short_name': fx_varname, 'mip': 'fx'}, var)
-    elif var['project'] == 'CMIP6':
-        if fx_varname == 'sftlf':
-            fx_var = _add_fxvar_keys({'short_name': fx_varname, 'mip': 'fx'},
-                                     var)
-        elif fx_varname == 'sftof':
-            fx_var = _add_fxvar_keys({'short_name': fx_varname, 'mip': 'Ofx'},
-                                     var)
-        # TODO allow availability for multiple mip's for sftgif
-        elif fx_varname == 'sftgif':
-            fx_var = _add_fxvar_keys({'short_name': fx_varname, 'mip': 'fx'},
-                                     var)
+    var_project = variable['project']
+    cmor_table = CMOR_TABLES[var_project]
+
+    # Get all fx-related mips ('fx' always first, original mip last)
+    fx_mips = ['fx']
+    fx_mips.extend(
+        [key for key in cmor_table.tables if 'fx' in key and key != 'fx'])
+    fx_mips.append(variable['mip'])
+
+    # Search all mips for available variables
+    searched_mips = []
+    for fx_mip in fx_mips:
+        fx_variable = cmor_table.get_variable(fx_mip, fx_varname)
+        if fx_variable is not None:
+            searched_mips.append(fx_mip)
+            fx_var = _add_fxvar_keys(
+                {'short_name': fx_varname, 'mip': fx_mip}, var)
+            logger.debug("For CMIP6 fx variable '%s', found table '%s'",
+                         fx_varname, fx_mip)
+            fx_files = _get_input_files(fx_var, config_user)
+
+            # If files found, return them
+            if fx_files:
+                logger.debug("Found CMIP6 fx variables '%s':\n%s",
+                             fx_varname, pformat(fx_files))
+                break
     else:
+        # No files found
+        fx_files = []
+
+    # If fx variable was not found in any table, raise exception
+    if not searched_mips:
         raise RecipeError(
-            f"Project {var['project']} not supported with fx variables")
+            f"Requested fx variable '{fx_varname}' not available in "
+            f"any 'fx'-related CMOR table ({fx_mips}) for '{var_project}'")
 
-    fx_files = _get_input_files(fx_var, config_user)
     # allow for empty lists corrected for by NE masks
     if fx_files:
         fx_files = fx_files[0]
@@ -726,7 +745,7 @@ def _get_single_preprocessor_task(variables,
     order = _extract_preprocessor_order(profile)
     ancestor_products = [p for task in ancestor_tasks for p in task.products]
 
-    if variables[0]['frequency'] == 'fx':
+    if variables[0].get('frequency') == 'fx':
         check.check_for_temporal_preprocs(profile)
         ancestor_products = None
 


Test patch:
diff --git a/tests/integration/test_recipe.py b/tests/integration/test_recipe.py
index f2dce12c9..682885cb8 100644
--- a/tests/integration/test_recipe.py
+++ b/tests/integration/test_recipe.py
@@ -1420,7 +1420,6 @@ def test_extract_shape_raises(tmp_path, patched_datafinder, config_user,
 
 
 def test_weighting_landsea_fraction(tmp_path, patched_datafinder, config_user):
-
     content = dedent("""
         preprocessors:
           landfrac_weighting:
@@ -1471,7 +1470,6 @@ def test_weighting_landsea_fraction(tmp_path, patched_datafinder, config_user):
 
 def test_weighting_landsea_fraction_no_fx(tmp_path, patched_failing_datafinder,
                                           config_user):
-
     content = dedent("""
         preprocessors:
           landfrac_weighting:
@@ -1524,7 +1522,6 @@ def test_weighting_landsea_fraction_no_fx(tmp_path, patched_failing_datafinder,
 
 def test_weighting_landsea_fraction_exclude(tmp_path, patched_datafinder,
                                             config_user):
-
     content = dedent("""
         preprocessors:
           landfrac_weighting:
@@ -1577,7 +1574,6 @@ def test_weighting_landsea_fraction_exclude(tmp_path, patched_datafinder,
 
 def test_weighting_landsea_fraction_exclude_fail(tmp_path, patched_datafinder,
                                                  config_user):
-
     content = dedent("""
         preprocessors:
           landfrac_weighting:
@@ -1611,7 +1607,6 @@ def test_weighting_landsea_fraction_exclude_fail(tmp_path, patched_datafinder,
 
 
 def test_landmask(tmp_path, patched_datafinder, config_user):
-
     content = dedent("""
         preprocessors:
           landmask:
@@ -1658,13 +1653,12 @@ def test_landmask(tmp_path, patched_datafinder, config_user):
 
 
 def test_landmask_no_fx(tmp_path, patched_failing_datafinder, config_user):
-
     content = dedent("""
         preprocessors:
           landmask:
             mask_landsea:
               mask_out: sea
-              always_use_ne_mask: true
+              always_use_ne_mask: false
 
         diagnostics:
           diagnostic_name:
@@ -1679,6 +1673,8 @@ def test_landmask_no_fx(tmp_path, patched_failing_datafinder, config_user):
                 ensemble: r1i1p1
                 additional_datasets:
                   - {dataset: CanESM2}
+                  - {dataset: CanESM5, project: CMIP6, grid: gn,
+                     ensemble: r1i1p1f1}
                   - {dataset: TEST, project: obs4mips, level: 1, version: 1,
                      tier: 1}
             scripts: null
@@ -1690,14 +1686,363 @@ def test_landmask_no_fx(tmp_path, patched_failing_datafinder, config_user):
     task = recipe.tasks.pop()
     assert task.name == 'diagnostic_name' + TASKSEP + 'gpp'
 
-    # Check weighting
-    assert len(task.products) == 2
+    # Check masking
+    assert len(task.products) == 3
     for product in task.products:
         assert 'mask_landsea' in product.settings
         settings = product.settings['mask_landsea']
         assert len(settings) == 3
         assert settings['mask_out'] == 'sea'
-        assert settings['always_use_ne_mask'] is True
+        assert settings['always_use_ne_mask'] is False
         fx_files = settings['fx_files']
         assert isinstance(fx_files, list)
         assert fx_files == []
+
+
+def test_fx_vars_mip_change_cmip6(tmp_path, patched_datafinder, config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           area_statistics:
+             operator: mean
+             fx_files: [
+               'areacella',
+               'areacello',
+               'clayfrac',
+               'sftlf',
+               'sftgif',
+               'sftof',
+             ]
+           mask_landsea:
+             mask_out: sea
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              tas:
+                preprocessor: preproc
+                project: CMIP6
+                mip: Amon
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1f1
+                grid: gn
+                additional_datasets:
+                  - {dataset: CanESM5}
+            scripts: null
+        """)
+    recipe = get_recipe(tmp_path, content, config_user)
+
+    # Check generated tasks
+    assert len(recipe.tasks) == 1
+    task = recipe.tasks.pop()
+    assert task.name == 'diagnostic_name' + TASKSEP + 'tas'
+    assert len(task.products) == 1
+    product = task.products.pop()
+
+    # Check area_statistics
+    assert 'area_statistics' in product.settings
+    settings = product.settings['area_statistics']
+    assert len(settings) == 2
+    assert settings['operator'] == 'mean'
+    fx_files = settings['fx_files']
+    assert isinstance(fx_files, dict)
+    assert len(fx_files) == 6
+    assert '_fx_' in fx_files['areacella']
+    assert '_Ofx_' in fx_files['areacello']
+    assert '_Efx_' in fx_files['clayfrac']
+    assert '_fx_' in fx_files['sftlf']
+    assert '_fx_' in fx_files['sftgif']
+    assert '_Ofx_' in fx_files['sftof']
+
+    # Check mask_landsea
+    assert 'mask_landsea' in product.settings
+    settings = product.settings['mask_landsea']
+    assert len(settings) == 2
+    assert settings['mask_out'] == 'sea'
+    fx_files = settings['fx_files']
+    assert isinstance(fx_files, list)
+    assert len(fx_files) == 2
+    for fx_file in fx_files:
+        if 'sftlf' in fx_file:
+            assert '_fx_' in fx_file
+        elif 'sftof' in fx_file:
+            assert '_Ofx_' in fx_file
+        else:
+            assert False
+
+
+def test_fx_vars_volcello_in_ofx_cmip6(tmp_path, patched_datafinder,
+                                       config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           volume_statistics:
+             operator: mean
+             fx_files: ['volcello']
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              tos:
+                preprocessor: preproc
+                project: CMIP6
+                mip: Omon
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1f1
+                grid: gn
+                additional_datasets:
+                  - {dataset: CanESM5}
+            scripts: null
+        """)
+    recipe = get_recipe(tmp_path, content, config_user)
+
+    # Check generated tasks
+    assert len(recipe.tasks) == 1
+    task = recipe.tasks.pop()
+    assert task.name == 'diagnostic_name' + TASKSEP + 'tos'
+    assert len(task.products) == 1
+    product = task.products.pop()
+
+    # Check volume_statistics
+    assert 'volume_statistics' in product.settings
+    settings = product.settings['volume_statistics']
+    assert len(settings) == 2
+    assert settings['operator'] == 'mean'
+    fx_files = settings['fx_files']
+    assert isinstance(fx_files, dict)
+    assert len(fx_files) == 1
+    assert '_Ofx_' in fx_files['volcello']
+    assert '_Omon_' not in fx_files['volcello']
+
+
+def test_fx_vars_volcello_in_omon_cmip6(tmp_path, patched_failing_datafinder,
+                                        config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           volume_statistics:
+             operator: mean
+             fx_files: ['volcello']
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              tos:
+                preprocessor: preproc
+                project: CMIP6
+                mip: Omon
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1f1
+                grid: gn
+                additional_datasets:
+                  - {dataset: CanESM5}
+            scripts: null
+        """)
+    recipe = get_recipe(tmp_path, content, config_user)
+
+    # Check generated tasks
+    assert len(recipe.tasks) == 1
+    task = recipe.tasks.pop()
+    assert task.name == 'diagnostic_name' + TASKSEP + 'tos'
+    assert len(task.products) == 1
+    product = task.products.pop()
+
+    # Check volume_statistics
+    assert 'volume_statistics' in product.settings
+    settings = product.settings['volume_statistics']
+    assert len(settings) == 2
+    assert settings['operator'] == 'mean'
+    fx_files = settings['fx_files']
+    assert isinstance(fx_files, dict)
+    assert len(fx_files) == 1
+    assert '_Ofx_' not in fx_files['volcello']
+    assert '_Omon_' in fx_files['volcello']
+
+
+def test_fx_vars_volcello_in_oyr_cmip6(tmp_path, patched_failing_datafinder,
+                                       config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           volume_statistics:
+             operator: mean
+             fx_files: ['volcello']
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              o2:
+                preprocessor: preproc
+                project: CMIP6
+                mip: Oyr
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1f1
+                grid: gn
+                additional_datasets:
+                  - {dataset: CanESM5}
+            scripts: null
+        """)
+    recipe = get_recipe(tmp_path, content, config_user)
+
+    # Check generated tasks
+    assert len(recipe.tasks) == 1
+    task = recipe.tasks.pop()
+    assert task.name == 'diagnostic_name' + TASKSEP + 'o2'
+    assert len(task.products) == 1
+    product = task.products.pop()
+
+    # Check volume_statistics
+    assert 'volume_statistics' in product.settings
+    settings = product.settings['volume_statistics']
+    assert len(settings) == 2
+    assert settings['operator'] == 'mean'
+    fx_files = settings['fx_files']
+    assert isinstance(fx_files, dict)
+    assert len(fx_files) == 1
+    assert '_Ofx_' not in fx_files['volcello']
+    assert '_Oyr_' in fx_files['volcello']
+
+
+def test_fx_vars_volcello_in_fx_cmip5(tmp_path, patched_datafinder,
+                                      config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           volume_statistics:
+             operator: mean
+             fx_files: ['volcello']
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              tos:
+                preprocessor: preproc
+                project: CMIP5
+                mip: Omon
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1
+                additional_datasets:
+                  - {dataset: CanESM2}
+            scripts: null
+        """)
+    recipe = get_recipe(tmp_path, content, config_user)
+
+    # Check generated tasks
+    assert len(recipe.tasks) == 1
+    task = recipe.tasks.pop()
+    assert task.name == 'diagnostic_name' + TASKSEP + 'tos'
+    assert len(task.products) == 1
+    product = task.products.pop()
+
+    # Check volume_statistics
+    assert 'volume_statistics' in product.settings
+    settings = product.settings['volume_statistics']
+    assert len(settings) == 2
+    assert settings['operator'] == 'mean'
+    fx_files = settings['fx_files']
+    assert isinstance(fx_files, dict)
+    assert len(fx_files) == 1
+    assert '_fx_' in fx_files['volcello']
+    assert '_Omon_' not in fx_files['volcello']
+
+
+def test_wrong_project(tmp_path, patched_datafinder, config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           volume_statistics:
+             operator: mean
+             fx_files: ['volcello']
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              tos:
+                preprocessor: preproc
+                project: CMIP7
+                mip: Omon
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1
+                additional_datasets:
+                  - {dataset: CanESM2}
+            scripts: null
+        """)
+    with pytest.raises(ValueError) as wrong_proj:
+        get_recipe(tmp_path, content, config_user)
+        assert wrong_proj == "Project CMIP7 not in config-developer"
+
+
+def test_invalid_fx_var_cmip6(tmp_path, patched_datafinder, config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           area_statistics:
+             operator: mean
+             fx_files: [
+               'areacella',
+               'wrong_fx_variable',
+             ]
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              tas:
+                preprocessor: preproc
+                project: CMIP6
+                mip: Amon
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1f1
+                grid: gn
+                additional_datasets:
+                  - {dataset: CanESM5}
+            scripts: null
+        """)
+    msg = ("Requested fx variable 'wrong_fx_variable' for CMIP6 not "
+           "available in any 'fx'-related CMOR table")
+    with pytest.raises(RecipeError) as rec_err_exp:
+        get_recipe(tmp_path, content, config_user)
+        assert msg in rec_err_exp
+
+
+def test_fx_var_invalid_project(tmp_path, patched_datafinder, config_user):
+    content = dedent("""
+        preprocessors:
+          preproc:
+           area_statistics:
+             operator: mean
+             fx_files: ['areacella']
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              tas:
+                preprocessor: preproc
+                project: EMAC
+                mip: Amon
+                exp: historical
+                start_year: 2000
+                end_year: 2005
+                ensemble: r1i1p1f1
+                grid: gn
+                additional_datasets:
+                  - {dataset: CanESM5}
+            scripts: null
+        """)
+    msg = 'Project EMAC not supported with fx variables'
+    with pytest.raises(RecipeError) as rec_err_exp:
+        get_recipe(tmp_path, content, config_user)
+        assert msg in rec_err_exp