Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recipe_preprocessor_derive_test.yml is broken for CMIP6 toz #3709

Closed
bouweandela opened this issue Jul 2, 2024 · 12 comments · Fixed by ESMValGroup/ESMValCore#2471
Closed

Comments

@bouweandela
Copy link
Member

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
Copy link
Member Author

Potentially related issue: #3694

@bouweandela
Copy link
Member Author

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
Copy link
Contributor

schlunma commented Jul 2, 2024

I think I know what's going on: ESMValGroup/ESMValCore#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
Copy link
Member Author

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
Copy link
Contributor

schlunma commented Jul 2, 2024

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
Copy link
Member Author

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
Copy link
Contributor

schlunma commented Jul 2, 2024

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
Copy link
Member Author

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

@ehogan
Copy link
Contributor

ehogan commented Jul 2, 2024

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
Copy link
Contributor

schlunma commented Jul 2, 2024

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

@ehogan
Copy link
Contributor

ehogan commented Jul 2, 2024

@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
Copy link
Contributor

schlunma commented Jul 2, 2024

Also 100% fine for me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants