ESMValGroup / ESMValTool

ESMValTool: A community diagnostic and performance metrics tool for routine evaluation of Earth system models in CMIP
https://www.esmvaltool.org
Apache License 2.0
210 stars 122 forks source link

`recipe_preprocessor_derive_test.yml` is broken for CMIP6 toz #3709

Closed bouweandela closed 6 days ago

bouweandela commented 6 days ago

The error is

  File "/home/bandela/src/esmvalgroup/esmvalcore/esmvalcore/preprocessor/__init__.py", line 346, in _run_preproc_function
    return function(items, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/bandela/src/esmvalgroup/esmvalcore/esmvalcore/preprocessor/_derive/__init__.py", line 123, in derive
    raise ValueError(
ValueError: Units 'DU' after executing derivation script of 'toz' cannot be converted to target units 'm'

The units of toz are indeed in m in esmvalcore/cmor/tables/cmip6/Tables/CMIP6_AERmon.json and esmvalcore/cmor/tables/cmip6/Tables/CMIP6_AERday.json, but the derive preprocessor produces a cube with units in DU, which are not convertible to m according to cf_units:

In [1]: from cf_units import Unit

In [2]: Unit('DU').convert(1, 'm')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[2], line 1
----> 1 Unit('DU').convert(1, 'm')

File ~/mambaforge/envs/esmvaltool2/lib/python3.11/site-packages/cf_units/__init__.py:1918, in Unit.convert(self, value, other, ctype, inplace)
   1916     return result
   1917 else:
-> 1918     raise ValueError(
   1919         "Unable to convert from '%r' to '%r'." % (self, other)
   1920     )

ValueError: Unable to convert from 'Unit('DU')' to 'Unit('m')'.

@schlunma Do you understand what is going wrong here?

bouweandela commented 6 days ago

Potentially related issue: #3694

bouweandela commented 6 days ago

Note that the recipe ran fine for the previous release: https://esmvaltool.dkrz.de/shared/esmvaltool/v2.10.0/recipe_preprocessor_derive_test_20231212_230320/

schlunma commented 6 days ago

I think I know what's going on: https://github.com/ESMValGroup/ESMValCore/pull/2256 changed the way tables are read for derived variables. Before that, toz was always read from the custom toz table, which uses DU as units. Now, for CMIP6, it's read from another CMIP6 table that contains it, e.g. AERmon.

bouweandela commented 6 days ago

Thanks! My fault then ;-) Any idea how to solve this? Could we set the units of the custom CMIP5 toz variable to meters as is done in the CMIP6 cmor table? Or would that cause problems for the diagnostic scripts using these variables?

schlunma commented 6 days ago

Yes, I'm 99% percent sure this will break existing diagnostics. This is used by many NCL diagnostics and these are fragile...

We have a convert_units preprocessor that actually allows this special conversion m <-> DU. We could use that anywhere in our code instead of cube.convert_units. However, I guess this would require a rc3 for ESMValCore...

An alternative could be to change the units in the custom table and add a convert_units preprocessor step to the affected recipes. This would probably work, but may expect extra work because an individual preprocessor for toz needs to be used.

bouweandela commented 6 days ago

However, I guess this would require a rc3 for ESMValCore...

It looks like apart from recipe_preprocessor_derive_test.yml, only recipe_perfmetrics_CMIP5.yml and recipe_eyring06jgr.yml use toz and both use it with CMIP5 data where this works fine, so maybe we can leave the issue open for this release since no 'real' recipes have been affected.

schlunma commented 6 days ago

Sounds like a good idea. So we leave the recipe_preprocessor_derive_test.yml as it is for now and then update the Core for v2.12?

bouweandela commented 6 days ago

That sounds easiest. @ehogan What's your opinion?

ehogan commented 6 days ago

So leave this in the broken recipe list for v2.11.0 and point to this issue instead of the HDF one? Or have I misunderstood? 🤔

schlunma commented 6 days ago

@ehogan in my opinion you can leave it out of that list, it's already been fixed in the current main: https://github.com/ESMValGroup/ESMValCore/pull/2471

ehogan commented 6 days ago

@ehogan in my opinion you can leave it out of that list, it's already been fixed in the current main: ESMValGroup/ESMValCore#2471

Wow, that was quick! 😁 The Broken recipe list documentation states "The table is always valid for the latest stable release of ESMValTool.", so I feel we should keep it in the table, since it will be broken for v2.11.0. I would be very happy to remove it as soon as the release is complete! 🤪

schlunma commented 6 days ago

Also 100% fine for me 👍