fixed missing frequency for cmip6 cmor checks by valeriupredoi · Pull Request #985 · ESMValGroup/ESMValTool

valeriupredoi · 2019-03-19T22:03:31Z

should solve #981

mattiarighi · 2019-03-20T09:39:10Z

The problem with obs4mips persists (tried with dataset CERES-EBAF and variable lwcre).

valeriupredoi · 2019-03-20T09:56:30Z

Blast! lwcre is custom (derived) right? Ill have a look at the custom loader/checker. Can you test with say tas see if it works for standard variables at least? Dr Valeriu Predoi. Computational scientist NCAS-CMS University of Reading Department of Meteorology Reading RG6 6BB United Kingdom

…

On Wed, 20 Mar 2019, 09:39 Mattia Righi, ***@***.***> wrote: The problem with obs4mips persists (tried with dataset CERES-EBAF and variable lwcre). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#985 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AbpCow2_18hwM_HyYoFI89fiCWzjSgYVks5vYgG_gaJpZM4b9Nf1> .

mattiarighi · 2019-03-20T10:32:04Z

With hus (non-derived) and AIRS it works!

valeriupredoi · 2019-03-20T10:34:02Z

Awesome, cheers for testing, Mattia! on my way to work and will have a look at the derived case right away Dr Valeriu Predoi. Computational scientist NCAS-CMS University of Reading Department of Meteorology Reading RG6 6BB United Kingdom

…

On Wed, 20 Mar 2019, 10:32 Mattia Righi, ***@***.***> wrote: With hus (non-derived) and AIRS it works! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#985 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AbpCo3QEFnBZrNV5_T6u5gDSyOgtZt2Wks5vYg4lgaJpZM4b9Nf1> .

valeriupredoi · 2019-03-20T14:37:01Z

ok @mattiarighi now it should work for any derived variable (with base CMIP5, 6 or any other) - I tested with a synthetic derived variable since I don't have CMIP6 data and it works. Note that you need to add frequency: xxx in the custom CMOR table

valeriupredoi · 2019-03-20T14:40:54Z

guys, pls have a test or two and let me know if all works so I can fix the test that's failing (keeping that at the very end); also, it would be very useful, now that @jvegasbsc has gone on daddy leave, to ask @sloosvel to double check these changes when she got time (we should probably merge if all works fine, but keep that in mind) - I really am not a CMORChecks expert at all 😈

mattiarighi · 2019-03-20T15:28:50Z

Now I get a different error for CERES-EBAF with lwcre

2019-03-20 15:25:57,526 UTC [45300] ERROR   Failed to run fix_metadata([<iris 'Cube' of TOA Longwave Cloud Radiative Effect / (W m-2) (time: 36; latitude: 180; longitude: 360)>], {'project': 'obs4mips', 'dataset': 'CERES-EBAF', 'short_name': 'lwcre', 'cmor_table': 'obs4mips', 'mip': 'Amon', 'frequency': ''})
2019-03-20 15:25:57,878 UTC [32979] ERROR   Program terminated abnormally, see stack trace below for more information
multiprocessing.pool.RemoteTraceback:
"""
Traceback (most recent call last):
  File "/pf/b/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.6/multiprocessing/pool.py", line 119, in worker
    result = (True, func(*args, **kwds))
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_task.py", line 660, in _run_task
    output_files = task.run()
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_task.py", line 226, in run
    self.output_files = self._run(input_files)
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/preprocessor/__init__.py", line 392, in _run
    product.apply(step, self.debug)
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/preprocessor/__init__.py", line 259, in apply
    self.cubes = preprocess(self.cubes, step, **self.settings[step])
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/preprocessor/__init__.py", line 201, in preprocess
    result.append(_run_preproc_function(function, items, settings))
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/preprocessor/__init__.py", line 187, in _run_preproc_function
    return function(items, **kwargs)
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/cmor/fix.py", line 116, in fix_metadata
    checker(cube).check_metadata()
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/cmor/check.py", line 100, in check_metadata
    self._check_time_coord()
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/cmor/check.py", line 443, in _check_time_coord
    elif self.frequency.endswith('hr'):
AttributeError: 'NoneType' object has no attribute 'endswith'
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_main.py", line 228, in run
    conf = main(args)
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_main.py", line 156, in main
    process_recipe(recipe_file=recipe, config_user=cfg)
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_main.py", line 206, in process_recipe
    recipe.run()
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_recipe.py", line 1066, in run
    self.tasks, max_parallel_tasks=self._cfg['max_parallel_tasks'])
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_task.py", line 585, in run_tasks
    _run_tasks_parallel(tasks, max_parallel_tasks)
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/_task.py", line 630, in _run_tasks_parallel
    task.output_files, updated_products = result.get()
  File "/pf/b/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.6/multiprocessing/pool.py", line 670, in get
    raise self._value
AttributeError: 'NoneType' object has no attribute 'endswith'

valeriupredoi · 2019-03-20T15:32:17Z

you need to add frequency: xxx in CMOR_lwcre.dat, I think it's mon

mattiarighi · 2019-03-20T15:34:23Z

Ok, but how? As an additional variable attribute or at the top of the table?

mattiarighi · 2019-03-20T15:35:55Z

The frequency is also not unique for the custom variables, since sometimes the same table is used for mon and day.

valeriupredoi · 2019-03-20T15:37:43Z

it's a variable attribute just like units etc; in that case we're kinda screwed since we need to create separate files for different frequencies; the code needs that frequency from the CMOR table in light of the new CMIP6 CMOR table structure

valeriupredoi · 2019-03-20T15:43:20Z

I also found this discrepancy between CMIP5 and 6 standard names:

Amon:
prw:
CMIP5: "atmosphere_water_vapor_content"
CMIP6: "atmosphere_mass_content_of_water_vapor"

do we have a strategy for these cases yet?

mattiarighi · 2019-03-20T15:47:45Z

it's a variable attribute just like units etc; in that case we're kinda screwed since we need to create separate files for different frequencies; the code needs that frequency from the CMOR table in light of the new CMIP6 CMOR table structure

It's still not clear to me how this affects obs4mips and the custom variables, since both follow their own tables, the latter based on CMIP5. Changes in CMIP6 tables should not have any effect here.

mattiarighi · 2019-03-20T15:49:28Z

do we have a strategy for these cases yet?

Not yet, as far as I know @jvegasbsc only implemented a fix for the short_name (see #950).

valeriupredoi · 2019-03-20T16:07:05Z

cmor_type for obs4mips is CMIP6 as per config-developer

mattiarighi · 2019-03-20T16:09:20Z

Should we try to update the obs4mips tables as well?

mattiarighi · 2019-03-20T16:13:07Z

As a quick solution, can we revert the CMIP6 tables to the previous version until this is solved?

valeriupredoi · 2019-03-20T16:26:29Z

OK so here's the status:

with this PR we have all working with the current CMIP6 tables provided that the derived variables custom CMOR tables contain a valid frequency field; the standard variables should work fine as of now;
if we use cmor_type: CMIP5 in the various data types in config-developer.yml we revert to CMIP5 CMOR tables and we don't need to add frequency fields in the derived variables' CMOR files; or we revert to previous version of CMIP6 CMOR tables; I'd go with the first option since it involves less file tracking

valeriupredoi · 2019-03-20T16:33:51Z

there are small differences if we want to use CMIP5 CMOR tables for CMIP6 data eg:

    _CMIP_5to6_varname = {
        'sic': 'siconc',
        'sit': 'sithick',
        'tro3': 'o3',
    }

but these shouldn't bite us too badly

mattiarighi · 2019-03-20T16:37:28Z

provided that the derived variables custom CMOR tables contain a valid frequency field

But they don't, and adding it would mean redesigning the way the custom tables are defined (i.e., group them by mip) and regenerate our obs data pool again (including also adjusting most of the cmorizers).
This is something we would need to do anyway at some point, but not now.

if we use cmor_type: CMIP5 in the various data types in config-developer.yml we revert to CMIP5 CMOR tables

Isn't this also controlling the table format (plain text in CMIP5 vs json in CMIP6)?

we revert to previous version of CMIP6 CMOR tables

It still looks the quickest way out to me, at least until we have a clean solution for the issues you mentioned (which may be some work).

valeriupredoi · 2019-03-20T16:43:25Z

using cmor_type: CMIP5 for anything CMIP6 including CMIP6 data works fine no problemo, what you mean by "Isn't this also controlling the table format (plain text in CMIP5 vs json in CMIP6)?" - it just loads different CMOR tables and applies the same checks and cmorization, no worries about tables' format

mattiarighi · 2019-03-20T16:47:37Z

CMIP5 for anything CMIP6 including CMIP6 data works fine no problemo

That would probably not work for those variables with a different short_name (see #950).
@LisaBock or @axel-lauer any suggestion?

valeriupredoi · 2019-03-20T16:48:40Z

also if you temporarily just add frequency: something in the derived vars custom tables, that doesn't break any CMIP5 functionality and the bonus is that addition will make work with CMIP6 even before we start re-arranging them custom tables and rerun the obs's (OK we just add mon). Alas, the code changes in this PR are actually the ones we will need to be able to work with the latest CMIP6 tables (well, unless @jvegasbsc gets bored of his new wee baby and makes changes 😁 )

valeriupredoi · 2019-03-20T16:50:18Z

CMIP5 for anything CMIP6 including CMIP6 data works fine no problemo

That would probably not work for those variables with a different short_name (see #950).
@LisaBock or @axel-lauer any suggestion?

yesh indeed, but those are not on any ESGF node yet anyway so people can keep their CMIP5 names 😁

mattiarighi · 2019-03-20T16:51:26Z

yesh indeed, but those are not on any ESGF node yet anyway so people can keep their CMIP5 names

The CMIP6 data are flowing right now and people are start working with them.
Therefore I'm hesitant introducing changes now and would go back to the previous version (which has been quite extensively tested in our group).

valeriupredoi · 2019-05-23T15:42:54Z

I have been trying to say that myself since March 🤣 - I was thinking about adding the frequency info in the custom var based on all the component variables in an automatic way (and not by hand in the custom table) but I had left it for you to review and decide - it shouldn't be too hard since all the component vars should have consistent frequencies no?

mattiarighi · 2019-05-23T15:45:52Z

Some use fx variable together with monthly, e.g. nbp_grid.

valeriupredoi · 2019-05-23T15:52:45Z

good point! The fx var should be sidelined then - #1065 and #1037 are taking care of that the fx variable is treated correctly as a bloody fx variable anyway

jvegreg · 2019-05-23T20:11:04Z

I am counting the frequency and assigning the max value

axel-lauer · 2019-05-24T08:04:00Z

Just tested recipe_ecs_scatter_cmip6.yml successfully.

valeriupredoi · 2019-05-24T11:25:05Z

+                from collections import Counter
+                var_freqs = (var.frequency for var in table.values())
+                table_freq, _ = Counter(var_freqs).most_common(1)[0]
+                table.frequency = table_freq


Boss! I like it - I tested with a whole bunch of tables and the closest one I got was this one:

Counter(freqs).most_common(2) [(' "yr"', 10), (' "yrPt"', 9)]

for Eyr

jvegreg · 2019-05-24T13:27:30Z

Mattia, can you test again? Now custom vars will be assigned the table frequency, so I expect this will fix your issue

mattiarighi · 2019-05-24T13:50:14Z

Thanks, I will test asap.

mattiarighi · 2019-05-28T08:39:56Z

I can't test anything until #1097 is solved. 😭

…eqIssue981

mattiarighi · 2019-05-30T19:20:59Z

Tested successfully!

valeriupredoi · 2019-05-31T11:15:29Z

@mattiarighi @jvegasbsc shall we merge then? awesome work guys 🍺

bouweandela · 2019-06-07T14:09:13Z

Please re-open this pull request in the ESMValTool Core repository. You can do so by pushing this branch to that repository, using the following instructions.

First add the repository with

git remote add esmvalcore git@github.com:esmvalgroup/esmvalcore

or

git remote add esmvalcore https://github.com/esmvalgroup/esmvalcore

if you're not using ssh to connect to GitHub.

Next upload the branch with

git push esmvalcore your_branch

and open a pull request here

mattiarighi · 2019-06-11T09:07:52Z

Also note the new issues number #38 and #45.

sloosvel · 2019-07-12T09:48:46Z

sorry I did not mean to close that :/

mattiarighi · 2019-07-15T11:47:27Z

@valeriupredoi is this still relevant?

valeriupredoi · 2019-07-15T12:05:51Z

still is, somewhat relevant albeit the code is starting to become obsolete, see the discussion here ESMValGroup/ESMValCore#158

valeriupredoi · 2019-07-15T12:06:29Z

ie the code = CMOR tables which are even more modern in ESMValGroup/ESMValCore#158

mattiarighi · 2019-07-15T12:06:46Z

But shouldn't it be moved to Core?

valeriupredoi · 2019-07-15T12:11:44Z

yes, I believe @jvegasbsc is cherrypicking the changes in that Core PR, no?

jvegreg · 2019-07-15T14:04:02Z

I will close this when the pull requests is created for the core

jvegreg · 2019-07-15T15:50:02Z

Ported to /ESMValGroup/ESMValCore#158

fixed missing frequency for cmip6 cmor checks

ae4d974

valeriupredoi requested review from bouweandela, jvegreg and mattiarighi March 19, 2019 22:03

support for frequency for custom variables as well

6ae91e0

Add assignation of frequeny to table

2bde36b

valeriupredoi commented May 24, 2019

View reviewed changes

Added frequency table to varinfo for custom vars

0f9063b

Merge branch 'version2_development' into version2_development_CMIP6Fr…

bb8e4ba

…eqIssue981

mattiarighi approved these changes May 30, 2019

View reviewed changes

sloosvel closed this Jun 20, 2019

sloosvel reopened this Jul 12, 2019

valeriupredoi mentioned this pull request Jul 15, 2019

Update CMIP6 and obs4mips cmor tables to last release ESMValGroup/ESMValCore#158

Merged

jvegreg closed this Jul 15, 2019

jvegreg deleted the version2_development_CMIP6FreqIssue981 branch July 15, 2019 16:08

Uh oh!

Conversation

valeriupredoi commented Mar 19, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019 via email

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019 via email

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019

Uh oh!

mattiarighi commented Mar 20, 2019

Uh oh!

valeriupredoi commented May 23, 2019

Uh oh!

mattiarighi commented May 23, 2019

Uh oh!

valeriupredoi commented May 23, 2019

Uh oh!

jvegreg commented May 23, 2019

Uh oh!

axel-lauer commented May 24, 2019

Uh oh!

valeriupredoi May 24, 2019

Choose a reason for hiding this comment

Uh oh!

jvegreg commented May 24, 2019

Uh oh!

mattiarighi commented May 24, 2019

Uh oh!

mattiarighi commented May 28, 2019

Uh oh!

mattiarighi commented May 30, 2019

Uh oh!

valeriupredoi commented May 31, 2019

Uh oh!

bouweandela commented Jun 7, 2019

Uh oh!

valeriupredoi commented Mar 20, 2019 •

edited

Loading

mattiarighi commented Jun 11, 2019 •

edited

Loading