Update CMIP6 and obs4mips cmor tables to last release

[jvegreg]

Updating tables to last release

https://github.com/PCMDI/cmip6-cmor-tables/releases/tag/6.5.29

Applied a fix to Efx tables (generic level missing). It is already on master, but I prefer to use tags for this so it is easier to know the exact version we use

Closes #150 and closes #45

As a bonus, I added a better error message when trying to get a coordinate that is not defined

[zklaus]

Thanks, I think this helps a lot!

There is a problem that we are slowly approaching: CMIP6 data will not follow a uniform data_specs_version. This matters because it means that for example data from different models will have different standard_names. For example the two already published datasets

{dataset: GISS-E2-1-G, exp: historical, ensemble: r1i1p1f1, mip: Amon, short_name: psl, grid: gn}
{dataset: GFDL-CM4, exp: historical, ensemble: r1i1p1f1, mip: Amon, short_name: psl, grid: gr1}

use standard_name air_pressure_at_sea_level and air_pressure_at_mean_sea_level, respectively.

Both of them do so following the data_specs_version that they give in the file, namely 01.00.23 and 01.00.27. But how do you compare this?

The oldest data_specs_version I have seen in CMIP6 data is 01.00.23, but there is no guarantee that there isn't and older one and I have not checked what has changed since then.

I am not sure what, if anything, to do about it right now, but at some point we will get this kind of error.
Broadly speaking there seem to be three approaches:

• Do nothing
• Support multiple data spec versions
• Choose one data spec version and provide "fixes" to align other data with that.

What do you think?

[jvegreg]

Do nothing

I think this is not possible

Support multiple data spec versions

This will be a nightmare, not only for us but for users. They will find different CMIP6 specifications in their diagnostic code and will have to adapt to it.

Choose one data spec version and provide "fixes" to align other data with that.

I prefer this, with one condition: we choose last version released and keep updating. We should assume that changes are done for a reason. Most variables will be the same in all versions anyway, so I think the number of fixes will be manageable. We can add fixes at PROJECT level that are applied after MODEL fixes to convert data to the latest version of the request.

In your example, we can add a PROJECT fix that changes the standard_name for psl to the new one automatically if it finds the old one.

[zklaus]

Ok, @jvegasbsc I agree with that approach. I opened a new issue for that (#159) so that we can finish this PR.

Thanks!

[bouweandela]

It looks like frequency is now always defined in the variable and never in the table header. Could you simplify the code so it only reads the frequency from the variable section, i.e. remove the frequency related code from CMIP6Info._load_table and add it in VariableInfo.read_json?

[bouweandela]

@valeriupredoi I believe you worked on this topic while @jvegasbsc was on parental leave. Can you also have a look at this pull request?

[bouweandela]

There is also this code

ESMValCore/esmvalcore/_recipe.py

Lines 127 to 129 in d94e6a7

	mip_info = CMOR_TABLES[cmor_table].get_table(mip)
	if mip_info:
	table_entry.frequency = mip_info.frequency

which will not work anymore for CMIP6 tables. I'm not entirely sure why it is here anyway, but it seems somehow related to derived variables.

[jvegreg]

which will not work anymore for CMIP6 tables.

Out of the box it will not work, but we are assigning a frequency to the mip by ourselves: the most common in the variables.

I'm not entirely sure why it is here anyway, but it seems somehow related to derived variables.

Yes, it is for assigning a frequency for the checks for custom and derived variables

[jvegreg]

It looks like frequency is now always defined in the variable and never in the table header. Could you simplify the code so it only reads the frequency from the variable section, i.e. remove the frequency related code from CMIP6Info._load_table and add it in VariableInfo.read_json?

There are other projects that are be using CMOR3 format for their tables (i.e. PRIMAVERA) I don't know if all of them made this change.

By the way, we are using CMIP5 and CMIP6 as cmor types when we are really refering to CMOR 2 and CMOR 3. I think it is easier for users to match, but the naming is not entirely correct

[bouweandela]

I'm not entirely sure why it is here anyway, but it seems somehow related to derived variables.

Yes, it is for assigning a frequency for the checks for custom and derived variables

Would it be possible to replace

ESMValCore/esmvalcore/_recipe.py

Lines 116 to 120 in d94e6a7

	table_entry = CMOR_TABLES[cmor_table].get_variable(mip, short_name)
	if derive and table_entry is None:
	custom_table = CMOR_TABLES['custom']
	table_entry = custom_table.get_variable(mip, short_name)

ESMValCore/esmvalcore/_recipe.py

Lines 127 to 129 in d94e6a7

	mip_info = CMOR_TABLES[cmor_table].get_table(mip)
	if mip_info:
	table_entry.frequency = mip_info.frequency

by something like

    cmor_strict = False if derive else 'default'
    table_entry = CMOR_TABLES[cmor_table].get_variable(mip, short_name, cmor_strict)

and change the table readers so this works (i.e. if 'default' is passed it should use whatever is configured in config-developer.yml)? I think it's confusing to change the content of the cmor variable definitions outside of the cmor table module and also using the custom or default table outside of the table module seems a bit hacky, because it's not a proper table.

[bouweandela]

which will not work anymore for CMIP6 tables.

Out of the box it will not work, but we are assigning a frequency to the mip by ourselves: the most common in the variables.

I don't see any code that does this, can you point me to it?

[jvegreg]

I don't see any code that does this, can you point me to it?

Here: https://github.com/ESMValGroup/ESMValTool/pull/985/files

I thought this was merged, but it seems it got lost in the split

[zklaus]

Notice how that PR actually contains updated tables.

[valeriupredoi]

yes, my old PR! And now @mattiarighi is busting my balls about it 😁

[valeriupredoi]

Notice how that PR actually contains updated tables.

well, they were the latest at their time (the time of the PR), now they've become obsolete as well, I believe the latest CMOR version is 01.00.30 (if not 31beta)

[valeriupredoi]

@jvegasbsc are you going to port ESMValGroup/ESMValTool#985 in here?

[jvegreg]

Yes, I will start working on it now

[mattiarighi]

Please do not merge this until is tested with several recipes.
The last tables update gave us lots of headaches...

[zklaus]

Re Mattias concern: In principle, the headaches should be reduced this time round simply because of the split. No change to the master branch here impacts recipe development, so I wonder if we should not rather get this merged relatively quickly, but make sure that things work or at least that there is a clear way to adjust existing recipes, only before we produce the next (pre-)release?

Also, there are very few CMIP6 recipes in the public part of ESMValTool at the moment. Which recipes do you think should be checked?

[mattiarighi]

make sure that things work or at least that there is a clear way to adjust existing recipes?

That's what I meant, just run a few recipes to make sure nothing gets broken.
We should make sure that the development branch is always stable, independently of the official releases.

Also, there are very few CMIP6 recipes in the public part of ESMValTool at the moment. Which recipes do you think should be checked?

Some of the ones using obs4mips (I can do that with perfmetrics), since last time there were issues with this project, and maybe one or two with CMIP6.

So just give me your OK when this is ready for testing.
It won't take long...

[zklaus]

make sure that things work or at least that there is a clear way to adjust existing recipes?

That's what I meant, just run a few recipes to make sure nothing gets broken.

Hah, what a sneaky way to quote :) The important bit for me was of course

only before a release.

We should make sure that the development branch is always stable, independently of the official releases.

Sure, but what does stable mean? We will want to introduce changes that break existing recipes. That's ok and should not in and of itself delay a release of the core, nor should the core be considered unstable because of it. Rather the ESMValTool should be judicious in adjusting the dependency on a new version of the core.

Also, there are very few CMIP6 recipes in the public part of ESMValTool at the moment. Which recipes do you think should be checked?
Some of the ones using obs4mips (I can do that with perfmetrics), since last time there were issues with this project, and maybe one or two with CMIP6.

So just give me your OK when this is ready for testing.
It won't take long...

Great, that should definitely work for this PR. 👍

[mattiarighi]

We will want to introduce changes that break existing recipes.

Yes, but not at this stage.
A lot of people are working hard on their diagnostics to finalize the papers and we should avoid introducing changes that could break existing recipes and delay the diagnostic development.

But as I said, the usual testing procedure plus a couple of more recipes with CMIP6 models should be enough.

[zklaus]

The flip side of course is that we surely won't want to release with 01.00.10 tables, so getting this in quickly will allow people to develop there recipes and diagnostics with a version of the tables that is close to the one used in all their data instead of trying to bend the data to a long out-of-date version of the standards.

But yeah, point taken, smooth development experience is important now.

[jvegreg]

Any update on this?

[valeriupredoi]

if it works (as it looks like it does), me happy 🍺

[zklaus]

Any update on this?

Would like to see this go in. At the moment though, there are a bunch of open conversations that should be resolved one way or another first.

[jvegreg]

Ready to merge from my side

Outdated

[bouweandela]

Code looks good, but I did not test.

[mattiarighi]

@schlunma can you test it with one of your cmip6 recipes?

[valeriupredoi]

On my way to work, shall do first thing Dr Valeriu Predoi. Computational scientist NCAS-CMS University of Reading Department of Meteorology Reading RG6 6BB United Kingdom

…

On Fri, 6 Sep 2019, 10:40 Klaus Zimmermann, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In esmvalcore/cmor/table.py <#158 (comment)> : > self._assign_dimensions(var, generic_levels) table[var_name] = var + self.var_to_freq[table.name][var_name] = var.frequency + + if not table.frequency: @valeriupredoi <https://github.com/valeriupredoi> care to answer and/or resolve (in the github sense) this conversation? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#158?email_source=notifications&email_token=AG5EFI5N5XHVXYN5KFJLGZTQIIQQHA5CNFSM4IA35HQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCD4RR2I#discussion_r321657249>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG5EFI6XXUW26FA2EM3QKADQIIQQHANCNFSM4IA35HQQ> .

[schlunma]

Successfully tested with 3 recipes with ~16 CMIP6 models each (included CMIP5 and OBS data as well). I did not run diagnostic scripts, though.