Fix units after derivation #754

schlunma · 2020-08-19T15:41:20Z

Adds checks if the units after executing a derivation script are compatible to the CMOR table and converts the units if necessary. Since not all derivation scripts have unit tests, I do not know if all of these still work. However, the changes in this PR only raise an exception if the units are not convertible (e.g. m and kg) and in this case the tool should definitely not silently set the units to the excepted ones like it does right now.

Tasks

Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
This pull request has a descriptive title that can be used in a changelog
Add unit tests
Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
If writing a new/modified preprocessor function, please update the documentation
Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
Please use yamllint to check that your YAML files do not contain mistakes
If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.

Closes #750

bouweandela · 2020-08-20T13:41:09Z

It would be good to test if the currently implemented derivation scripts actually work, instead of just adding some warnings and leaving the problems for our users. Maybe you could update the recipe examples/recipe_preprocessor_derive_test.yml so it includes all currently implemented derivation functions and then we can at least test it for for both a CMIP5 and a CMIP6 dataset.

schlunma · 2020-08-20T15:27:02Z

Good idea! I totally forgot that this recipe existed. I will update it and test all variables for CMIP5 and CMIP6 models.

schlunma · 2020-08-21T10:08:26Z

I updated the recipe in ESMValGroup/ESMValTool#1790 with the new derived variables and added CMIP6 models.

I had to fix several issues in the derivation scripts (but only one is related to this PR):

Compared to CMIP5, some standard_names of the required variables changed in CMIP6. Since some derivation scripts used this as a constraint for extracting cubes, these failed in CMIP5. I replaced these constraints with var_name_constraint.
In other cases, the short_names of the required variables changed from CMIP5 to CMIP6. Since the automatic conversion does not work right now (see Improve variable alias managament #595), I adapted the get_required function of the affected variables.
For toz, the desired units are Dobson units. Those can be either 0.4462 mmol m-2 or 2.687e20 m-2. Since cf_units uses the first definition and we used the latter, the derivation of toz in this PR raised a ValueError due to incompatible conversions. I fixed this in the derivation script.
For ohc I could not test CMIP6 data due to the lack of datasets that provide volcello and thetao.
For amoc, the required variables is msftmyz which does not exist in CMIP6. Since I do not know the CMIP6 equivalent, I skipped this for CMIP6. There is already some discussion about this in Problems with msftyz data #359, maybe @ledm or @zklaus can comment on this.

Please note that I only tested if the test recipe ran successful. I did not check if the resulting data is scientifically correct.

bouweandela · 2020-08-21T10:58:02Z

I updated the recipe in ESMValGroup/ESMValTool#1790 with the new derived variables and added CMIP6 models.

Great!

For ohc I could not test CMIP6 data due to the lack of datasets that provide volcello and thetao.

A synda search for volcello and thetao in CMIP6 gives me quite a few results:

synda search project=CMIP6 variable_id=thetao

For amoc, the required variables is msftmyz which does not exist in CMIP5.

The variable is defined in the CMIP5 Omon table. A synda search for msftmyz in CMIP5 also gives some results:

synda search project=CMIP5 variable=msftmyz

schlunma · 2020-08-21T11:36:44Z

A synda search for volcello and thetao in CMIP6 gives me quite a few results:
synda search project=CMIP6 variable_id=thetao

I found NorCPM1 (that was the only one with volcello for the historical run) and added it to the recipe.

For amoc, the required variables is msftmyz which does not exist in CMIP5.

The variable is defined in the CMIP5 Omon table. A synda search for msftmyz in CMIP5 also gives some results:
synda search project=CMIP5 variable=msftmyz

Sorry, I meant CMIP6. CMIP5 is already included in the recipe.

tests/integration/preprocessor/_derive/test_interface.py

bouweandela

Looks good, I left a small suggestion for writing the tests in a nicer way. Only if you have time.

bouweandela · 2020-10-01T07:24:32Z

@axel-lauer Do you think it would be possible to find someone to do a scientific review of this pull request? That would require running the recipe in ESMValGroup/ESMValTool#1790 and having a look at the results.

It would be great if this could still be included in v2.1, but the deadline is already this Monday, so maybe it will have to wait for the next release.

…ivation

schlunma · 2020-10-02T16:48:59Z

Change the derivation of vegfrac and ohc accoring to this comment, but did not test though.

bouweandela · 2020-10-05T13:32:20Z

@axel-lauer Would you be able to do a final test of the changes you proposed, so we can still include this in v2.1? We're doing the feature freeze today, but since this is a bugfix, we could still include it if it gets approved soon.

axel-lauer · 2020-10-06T09:02:37Z

I'll test the changes and report back soon...

axel-lauer · 2020-10-06T11:59:55Z

There were two corrections introduced: och and vegfrac. The changes to ohc look good, the values for CMIP6 models are now in the expected range. vegfrac still looks weird (e.g. vegfrac=100% in Antarctica). This is not related to this PR or the corrections introduced but I think it would be good to get this fixed now.

vegfrac is currently calculated as 100.0 - baresoilfrac (in %)

The CMOR tables (CMIP5) say:
baresoilFrac = fraction of entire grid cell that is covered by bare soil.
residualFrac = Fraction of Grid Cell that is Land but Neither Vegetation-Covered nor Bare Soil

From this, I would presume vegfrac should actually be calculated as follows:

vegfrac = 100.0 - baresoilfrac - residualfrac

@ledm , I think you introduced vegfrac. Can you confirm that this would be correct? Please let us know what you think s soon as possible.

axel-lauer · 2020-10-08T08:21:28Z

@ledm do you hav an opinion on correcting vegfrac? If not, I propose to change the calculation of vegfrac to what I think is correct:

vegfrac = 100.0 - baresoilfrac - residualfrac

@schlunma could you implement this so we can get this merged?

…ivation

schlunma · 2020-10-08T09:07:44Z

Fixed vegfrac (I also adapted the recipe in ESMValGroup/ESMValTool#1790). The output looks better now, but there are weird values at the coast of Antarctica and Greenland:

$vegfrac$

While testing this I encountered an issue with the CMOR checker. residualFrac has the coordinate typeresidual which has no standard name in the CMOR table, i.e. ''. However, since iris gives None for an empty standard name, the checker tool fails with

type: standard_name should be , not None

I added a fix for this:

                 try:
                     cube_coord = self._cube.coord(var_name=coordinate.out_name)
-                    if cube_coord.standard_name != coordinate.standard_name:
+                    if (cube_coord.standard_name is None and
+                            coordinate.standard_name == ''):
+                        pass
+                    elif cube_coord.standard_name != coordinate.standard_name:
                         self.report_critical(
                             self._attr_msg,
                             coordinate.out_name,

This isn't really nice but does the job. The optimal solution here would be when iris.cube.Cube.standard_name == '' evaluates True when the standard name is not set (i.e. is None).

bouweandela · 2020-10-08T13:01:05Z

This isn't really nice but does the job. The optimal solution here would be when iris.cube.Cube.standard_name == '' evaluates True when the standard name is not set (i.e. is None).

I think the best solution would be to change the way we read the cmor tables so we read this in as None instead of as '', but let's do that for the next release.

@axel-lauer Would you be able to test the latest changes?

axel-lauer · 2020-10-09T08:33:27Z

I tested the latest changes and it works. As @schlunma already noticed, however, there are spurious values along the coast lines. I presume these are caused by us assuming a land fraction of 100% for all land grid cells, i.e.:

vegfrac = 100% - baresoil - residualfrac

I think to get rid of those spurious coastal values, we would have to use the "real" land fraction and calculate vegfrac as:

vegfrac = landfrac - baresoil - residualfrac

Here is the "corrected" example from @schlunma. This now looks fine to me.

$vegfrac$

As "landfrac" I used the atmospheric fx file "sftlf". I guess to generalize this, a check would be needed if baresoil / residualfrac and sftlf are on the same grid.

axel-lauer · 2020-10-09T09:51:09Z

Thanks @schlunma! I tested this and it works. I would leave it like this for now. Let's get this merged then!

* Added check for units at the end of derivation function * Fixed tests for variable derivation interface * Adapted several derivation scripts * Adapted sm derivation script for CMIP6 * Fixed derivation of vegfrac and ohc * Fixed CMOR checker when coordinate standard_name is empty

valeriupredoi · 2020-10-09T14:33:11Z

good stuff, guys! we have just cherry-picked this PR in the 2.1 release 🍺

schlunma added 2 commits August 19, 2020 15:19

Added check for units at the end of derivation function

a8c2d18

Fixed tests for variable derivation interface

f9125e6

schlunma added bug Something isn't working variable derivation Related to variable derivation functions labels Aug 19, 2020

schlunma requested review from bouweandela and valeriupredoi August 19, 2020 15:41

schlunma self-assigned this Aug 19, 2020

schlunma added 2 commits August 21, 2020 11:38

Adapted several derivation scripts

5c70481

Adapted sm derivation script for CMIP6

1d04aaa

schlunma mentioned this pull request Aug 21, 2020

Updated derivation test recipe ESMValGroup/ESMValTool#1790

Merged

8 tasks

bouweandela reviewed Aug 21, 2020

View reviewed changes

tests/integration/preprocessor/_derive/test_interface.py Outdated Show resolved Hide resolved

bouweandela approved these changes Aug 21, 2020

View reviewed changes

Simplified tests using mocker fixture

3ab4e39

bouweandela mentioned this pull request Sep 24, 2020

Topics for ESMValTool workshop in November ESMValGroup/ESMValTool#1817

Closed

schlunma added 2 commits October 2, 2020 18:36

Merge remote-tracking branch 'origin/master' into fix_units_after_der…

938166f

…ivation

Fixed derivation of vegfrac and ohc

095481f

schlunma added 3 commits October 8, 2020 10:35

Merge remote-tracking branch 'origin/master' into fix_units_after_der…

8eaa6c4

…ivation

Fixed derivation of vegfrac

a8bfd02

Fixed CMOR checker when coordinate standard_name is empty

fbd7def

Fixed derivation of vegfrac

3233f57

Update changelog

dd571f5

bouweandela merged commit 04c029d into master Oct 9, 2020

bouweandela deleted the fix_units_after_derivation branch October 9, 2020 14:25

This was referenced Oct 23, 2020

Test before ESMValTool v2.1.0 release ESMValGroup/ESMValTool#1875

Closed

Fixing unit for derived variable rsnstcsnorm to prevent overcorrection2 #846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix units after derivation #754

Fix units after derivation #754

schlunma commented Aug 19, 2020 •

edited

Loading

bouweandela commented Aug 20, 2020

schlunma commented Aug 20, 2020

schlunma commented Aug 21, 2020 •

edited

Loading

bouweandela commented Aug 21, 2020

schlunma commented Aug 21, 2020

bouweandela left a comment

bouweandela commented Oct 1, 2020

schlunma commented Oct 2, 2020

bouweandela commented Oct 5, 2020

axel-lauer commented Oct 6, 2020

axel-lauer commented Oct 6, 2020

axel-lauer commented Oct 8, 2020

schlunma commented Oct 8, 2020

bouweandela commented Oct 8, 2020

axel-lauer commented Oct 9, 2020

axel-lauer commented Oct 9, 2020

valeriupredoi commented Oct 9, 2020

Fix units after derivation #754

Fix units after derivation #754

Conversation

schlunma commented Aug 19, 2020 • edited Loading

bouweandela commented Aug 20, 2020

schlunma commented Aug 20, 2020

schlunma commented Aug 21, 2020 • edited Loading

bouweandela commented Aug 21, 2020

schlunma commented Aug 21, 2020

bouweandela left a comment

Choose a reason for hiding this comment

bouweandela commented Oct 1, 2020

schlunma commented Oct 2, 2020

bouweandela commented Oct 5, 2020

axel-lauer commented Oct 6, 2020

axel-lauer commented Oct 6, 2020

axel-lauer commented Oct 8, 2020

schlunma commented Oct 8, 2020

bouweandela commented Oct 8, 2020

axel-lauer commented Oct 9, 2020

axel-lauer commented Oct 9, 2020

valeriupredoi commented Oct 9, 2020

schlunma commented Aug 19, 2020 •

edited

Loading

schlunma commented Aug 21, 2020 •

edited

Loading