ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 #1098

tomaslovato · 2021-05-05T13:10:02Z

Based on PR #1095 create the ocean depth mip-level fixes for other problematic CMIP6 datastest, namely cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr

Closes #1069 (and also #527 and #1100)

Link to documentation:

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

🧪 The new functionality is relevant and scientifically sound
🛠 This pull request has a descriptive title and labels
🛠 Code is written according to the code quality guidelines
🧪 and 🛠 Documentation is available
🛠 Unit tests have been added
🛠 Changes are backward compatible
🛠 Any changed dependencies have been added or removed correctly
🛠 The list of authors is up to date
🛠 All checks below this pull request were successful

To help with the number pull requests:

🙏 We kindly ask you to review two other open pull requests in this repository

tomaslovato · 2021-05-05T13:17:03Z

@schlunma could you please give me a feedback on test_cnrm_esm2_1.py before I continue with the other datasets (I feel like something is missing)... Thanks!

schlunma · 2021-05-05T13:23:11Z

Looks good already! The only thing that's missing is a test of the actual fix, so something like this:

ESMValCore/tests/integration/cmor/_fixes/cmip6/test_cesm2.py

Lines 228 to 253 in 4e9eb33

    
           @pytest.fixture 
        
           def thetao_cubes(): 
        
               """Cubes to test fixes for ``thetao``.""" 
        
               time_coord = iris.coords.DimCoord( 
        
                   [0.0004, 1.09776], var_name='time', standard_name='time', 
        
                   units='days since 1850-01-01 00:00:00') 
        
               lat_coord = iris.coords.DimCoord( 
        
                   [0.0, 1.0], var_name='lat', standard_name='latitude', units='degrees') 
        
               lon_coord = iris.coords.DimCoord( 
        
                   [0.0, 1.0], var_name='lon', standard_name='longitude', units='degrees') 
        
               lev_coord = iris.coords.DimCoord( 
        
                   [500.0, 1000.0], bounds=[[2.5, 7.5], [7.5, 12.5]], 
        
                   var_name='lev', standard_name=None, units='cm', 
        
                   attributes={'positive': 'up'}) 
        
               coord_specs = [ 
        
                   (time_coord, 0), 
        
                   (lev_coord, 1), 
        
                   (lat_coord, 2), 
        
                   (lon_coord, 3), 
        
               ] 
        
               thetao_cube = iris.cube.Cube( 
        
                   np.ones((2, 2, 2, 2)), 
        
                   var_name='thetao', 
        
                   dim_coords_and_dims=coord_specs, 
        
               ) 
        
               return iris.cube.CubeList([thetao_cube])

and this

ESMValCore/tests/integration/cmor/_fixes/cmip6/test_cesm2.py

Lines 310 to 325 in 4e9eb33

    
           def test_thetao_fix_metadata(thetao_cubes): 
        
               """Test ``fix_metadata`` for ``thetao``.""" 
        
               vardef = get_var_info('CMIP6', 'Omon', 'thetao') 
        
               fix = Omon(vardef) 
        
               out_cubes = fix.fix_metadata(thetao_cubes) 
        
               assert out_cubes is thetao_cubes 
        
               assert len(out_cubes) == 1 
        
               out_cube = out_cubes[0] 
        
               # Check metadata of depth coordinate 
        
               depth_coord = out_cube.coord('depth') 
        
               assert depth_coord.standard_name == 'depth' 
        
               assert depth_coord.var_name == 'lev' 
        
               assert depth_coord.long_name == 'ocean depth coordinate' 
        
               assert depth_coord.units == 'm' 
        
               assert depth_coord.attributes == {'positive': 'down'}

I think you can just copy-pasting these lines should work!

tomaslovato · 2021-05-06T08:15:52Z

@schlunma would it make this PR too long if I include also changes for MCM-Ua-1-0 dataset (#527)?

schlunma · 2021-05-06T08:18:50Z

No, that should be fine! Just let me know when it's ready for review 👍

tomaslovato · 2021-05-06T10:25:05Z

@schlunma @remi-kazeroni the code is ready for review 🚀 .
I already made a test with a recipe to check that it works for ocean variables thetao and no3.
Note that for mcm_ua_1_0 I decided to integrate needed changes in the original code under the Allvars fix.

schlunma · 2021-05-06T10:26:13Z

Great stuff! Having a look at it right now!

remi-kazeroni · 2021-05-06T10:35:51Z

@schlunma @remi-kazeroni the code is ready for review 🚀 .
I already made a test with a recipe to check that it works for ocean variables thetao and no3.
Note that for mcm_ua_1_0 I decided to integrate needed changes in the original code under the Allvars fix.

Thanks a lot! I'll have a look later today, in particular for the MCM_UA_1_0 fixes.

schlunma

Nice work @tomaslovato, looks really good already!

I have some suggestions to the fix of MCM and its test.

esmvalcore/cmor/_fixes/cmip6/mcm_ua_1_0.py

tests/integration/cmor/_fixes/cmip6/test_mcm_ua_1_0.py

tomaslovato · 2021-05-06T12:26:13Z

@schlunma I commited all the review changes in d9988fb

tomaslovato · 2021-05-06T12:54:01Z

@schlunma I got a bit confused about test_mcm_ua_1_0.py (I'm not skilled enough on this) as it is by now it only goes through Omon without applying changes from AllVars to roll the cube and coordinates... any suggestion?

schlunma · 2021-05-06T13:02:48Z

No worries! There are two options here:

Split the test function into two tests (maybe test_omon_thetao_fix_metadata and test_allvars_thetao_fix_metadata), use fix = Omon(...) and fix = AllVars(...), respectively, and only inlcude the asserts that belong to the respective fix.
Apply both fixes in the current test function:

    fix_omon = Omon(vardef)
    fix_allvars = AllVars(vardef)
    out_cubes = fix_omon.fix_metadata(thetao_cubes)
    out_cubes = fix_allvars.fix_metadata(out_cubes)

Option 2 is probably easier to implement, option 1 clearly seperates the two individual fixes. Optimally we would have both tests (i.e. option 1 and 2), but I'm fine with whatever you like to implement.

tomaslovato · 2021-05-06T13:07:14Z

    fix_omon = Omon(vardef)
    fix_allvars = AllVars(vardef)
    out_cubes = fix_omon.fix_metadata(thetao_cubes)
    out_cubes = fix_allvars.fix_metadata(out_cubes)

I think that for the time being option 2 will be ok, as it applies the fix to thetao including both Omon and AllVars as it should do when calling it from a recipe ...

schlunma · 2021-05-06T13:28:42Z

Tests are passing now, thank you really much for the fixes @tomaslovato! 🎉

Would you mind if I push 1-2 commits to this branch directly? I just realized that a test for Fgco2 of GFDL is missing (which is also missing for CESM2 and which I wanted to add for a long time) and that I forgot to add some documentation about the mip-level fixes.

I will approve afterwards and merge when @remi-kazeroni had the chance to test the fixes. Cheers!

tomaslovato · 2021-05-06T13:32:55Z

@schlunma thank you for the nice interaction! About you addition just go for it !
I've a little request ... could you have a look to #1097, that is only one line code change leftover from the former objective of this issue.

schlunma

@remi-kazeroni Please merge when you tested successfully 👍

tomaslovato · 2021-05-07T13:58:42Z

While working on the time coordinate issue for climate_statistic I found a problem with the dataset fix for ipsl_cm6a_lr.

The cell_area variable was oddly added to almost all ocean variables of this dataset, so a specific fix was setup about 2 years ago

ESMValCore/esmvalcore/cmor/_fixes/cmip6/ipsl_cm6a_lr.py

Lines 27 to 47 in 79ee767

    
           try: 
        
               cell_area = cubes.extract_cube('cell_area') 
        
           except ConstraintMismatchError: 
        
               return cubes 
        
           cell_area = AuxCoord( 
        
               cell_area.data, 
        
               standard_name=cell_area.standard_name, 
        
               long_name=cell_area.long_name, 
        
               var_name=cell_area.var_name, 
        
               units=cell_area.units, 
        
           ) 
        
           new_list = CubeList() 
        
           for cube in cubes: 
        
               if cube.name() == 'cell_area': 
        
                   continue 
        
               cube.add_aux_coord(cell_area, cube.coord_dims('latitude')) 
        
               cube.coord('latitude').var_name = 'lat' 
        
               cube.coord('longitude').var_name = 'lon' 
        
               new_list.append(cube) 
        
           return CubeList(new_list)

After the introduction of cube cell_measures trough _ancillary_vars.py, the ingestion of areacello from this dataset fails with an empty cubelist error.
Looking at the dump of the areacello file (here below), You will see that there are two variables (cell_area, areacello) both having as standard_name cell_area which is used by iris to assign cube name.
The general fix control

ESMValCore/esmvalcore/cmor/_fixes/cmip6/ipsl_cm6a_lr.py

Lines 41 to 42 in 79ee767

    
           if cube.name() == 'cell_area': 
        
               continue

is responsible for trashing both variables and produce an empty cubelist.

For me the simplest solution is to modify the if statement as in the following
if cube.name() == 'cell_area' and cube.var_name != 'areacello':

but probably we should also think to remove the creation of the auxcoord 'cell_area' (likely kept in there at the time for some potential use) which is a potentially misleading duplicate of the cube cell_measure property.

areacello ipsl_cm6a_lr

netcdf areacello_Ofx_IPSL-CM6A-LR_historical_r1i1p1f1_gn {
dimensions:
	axis_nbounds = 2 ;
	x = 362 ;
	y = 332 ;
	nvertex = 4 ;
	time = UNLIMITED ; // (0 currently)
variables:
	float nav_lat(y, x) ;
		nav_lat:standard_name = "latitude" ;
		nav_lat:long_name = "Latitude" ;
		nav_lat:units = "degrees_north" ;
		nav_lat:bounds = "bounds_nav_lat" ;
	float nav_lon(y, x) ;
		nav_lon:standard_name = "longitude" ;
		nav_lon:long_name = "Longitude" ;
		nav_lon:units = "degrees_east" ;
		nav_lon:bounds = "bounds_nav_lon" ;
	float bounds_nav_lon(y, x, nvertex) ;
	float bounds_nav_lat(y, x, nvertex) ;
	float area(y, x) ;
		area:standard_name = "cell_area" ;
		area:units = "m2" ;
		area:coordinates = "nav_lon nav_lat" ;
	float areacello(y, x) ;
		areacello:standard_name = "cell_area" ;
		areacello:long_name = "Grid-Cell Area" ;
		areacello:units = "m2" ;
		areacello:online_operation = "once" ;
		areacello:cell_methods = "area: sum" ;
		areacello:cell_measures = "area: area" ;
		areacello:_FillValue = 1.e+20f ;
		areacello:missing_value = 1.e+20f ;
		areacello:coordinates = "nav_lat nav_lon" ;
		areacello:description = "Cell areas for any grid used to report ocean variables and variables which are requested as used on the model ocean grid (e.g. hfsso, which is a downward heat flux from the atmosphere interpolated onto the ocean grid). These cell areas should be defined to enable exact calculation of global integrals (e.g., of vertical fluxes of energy at the surface and top of the atmosphere)." ;
		areacello:history = "none" ;

// global attributes:
		:Conventions = "CF-1.7 CMIP-6.2" ;
		:creation_date = "2018-07-11T07:26:30Z" ;
		:tracking_id = "hdl:21.14100/718e5096-9a8f-4794-a867-0ac583371b62" ;
		:description = "CMIP6 historical" ;
		:title = "IPSL-CM6A-LR model output prepared for CMIP6 / CMIP historical" ;
		:activity_id = "CMIP" ;
...

schlunma · 2021-05-07T15:32:04Z

Good find @tomaslovato! I just pushed a tiny fix for this, please let me know if it works for you! As you said, we do not need the cell_area coordinate since this is included via cell_measures.

tomaslovato · 2021-05-07T16:28:30Z

@schlunma I re-run the recipe with the modified code and it works great!

schlunma · 2021-05-07T16:32:34Z

Great! Ready to merge from my side

zklaus · 2021-05-10T13:08:17Z

@remi-kazeroni, could you please have a quick look?

remi-kazeroni · 2021-05-10T13:53:13Z

@remi-kazeroni, could you please have a quick look?

Yes, I'm on it. I'm done opening issues and PRs for the Core. It's time to start closing them 🍺

remi-kazeroni

Everything looks and works fine! Thanks a lot for your work @tomaslovato 👍 This can be merged

add ocean depth fix for cnrm_esm2_1 (#1069)

0b2724f

tomaslovato added the fix for dataset Related to dataset-specific fix files label May 5, 2021

tomaslovato requested a review from jvegreg as a code owner May 5, 2021 13:10

tomaslovato self-assigned this May 5, 2021

tomaslovato requested review from schlunma and removed request for jvegreg May 5, 2021 13:10

add missing code to test thetao in test_cnrm_esm2_1

f9af8a2

tomaslovato added this to the v2.3.0 milestone May 5, 2021

tomaslovato added 3 commits May 5, 2021 15:56

add depth fix for gfdl_esm4

b7a00df

add ocean depth Omon fix for ipsl_cm6a_lr

f0639f2

fix test_ipsl_cm6a_lr.py

ee6af57

remi-kazeroni mentioned this pull request May 5, 2021

Dataset problem: MCM-UA-1-0 has longitude values below 0 #527

Closed

add ocean depth and longitude fixes for mcm_ua_1_0 (#527)

62fbfea

tomaslovato changed the title ~~ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets~~ ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 (#527) May 6, 2021

tomaslovato changed the title ~~ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 (#527)~~ ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 May 6, 2021

remi-kazeroni self-requested a review May 6, 2021 09:10

remi-kazeroni added the AR6 Necessary changes for IPCC AR6 label May 6, 2021

tomaslovato added 2 commits May 6, 2021 11:12

adjust lon grid size derivation in mcm_ua_1_0

78d3732

adjust test_mcm_ua_1_0.py

2071ce2

schlunma reviewed May 6, 2021

View reviewed changes

apply changes to mcm_ua_1_0 after schlunma revision

d9988fb

add forgotten Omon case from last commit

4eede5d

setup a first test for thetao with both Omon and AllVars

c775491

schlunma added 2 commits May 6, 2021 16:00

Added missing tests for fgco2

968c3a7

Updated doc with mip-level fixes

79ee767

schlunma approved these changes May 6, 2021

View reviewed changes

Fixed allvars fix for ipsl model

0e69c9d

remi-kazeroni approved these changes May 10, 2021

View reviewed changes

remi-kazeroni merged commit e468d8d into master May 10, 2021

remi-kazeroni deleted the fix_depth_c6data branch May 10, 2021 15:11

remi-kazeroni mentioned this pull request May 10, 2021

CMIP6 data problem with MCM-UA-1-0 #1100

Closed

sloosvel mentioned this pull request Jun 14, 2021

Masked values when regridding datasets with coord circular=False #1175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 #1098

ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 #1098

tomaslovato commented May 5, 2021 •

edited by remi-kazeroni

Loading

tomaslovato commented May 5, 2021

schlunma commented May 5, 2021

tomaslovato commented May 6, 2021

schlunma commented May 6, 2021

tomaslovato commented May 6, 2021 •

edited

Loading

schlunma commented May 6, 2021

remi-kazeroni commented May 6, 2021

schlunma left a comment

tomaslovato commented May 6, 2021

tomaslovato commented May 6, 2021

schlunma commented May 6, 2021

tomaslovato commented May 6, 2021

schlunma commented May 6, 2021

tomaslovato commented May 6, 2021

schlunma left a comment

tomaslovato commented May 7, 2021

schlunma commented May 7, 2021

tomaslovato commented May 7, 2021

schlunma commented May 7, 2021

zklaus commented May 10, 2021

remi-kazeroni commented May 10, 2021

remi-kazeroni left a comment

ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 #1098

ocean depth fix for cnrm_esm2_1, gfdl_esm4, ipsl_cm6a_lr datasets + mcm_ua_1_0 #1098

Conversation

tomaslovato commented May 5, 2021 • edited by remi-kazeroni Loading

Checklist

tomaslovato commented May 5, 2021

schlunma commented May 5, 2021

tomaslovato commented May 6, 2021

schlunma commented May 6, 2021

tomaslovato commented May 6, 2021 • edited Loading

schlunma commented May 6, 2021

remi-kazeroni commented May 6, 2021

schlunma left a comment

Choose a reason for hiding this comment

tomaslovato commented May 6, 2021

tomaslovato commented May 6, 2021

schlunma commented May 6, 2021

tomaslovato commented May 6, 2021

schlunma commented May 6, 2021

tomaslovato commented May 6, 2021

schlunma left a comment

Choose a reason for hiding this comment

tomaslovato commented May 7, 2021

schlunma commented May 7, 2021

tomaslovato commented May 7, 2021

schlunma commented May 7, 2021

zklaus commented May 10, 2021

remi-kazeroni commented May 10, 2021

remi-kazeroni left a comment

Choose a reason for hiding this comment

tomaslovato commented May 5, 2021 •

edited by remi-kazeroni

Loading

tomaslovato commented May 6, 2021 •

edited

Loading