added fix for WACCM ua time coordinate #200

hb326 · 2019-08-06T11:51:20Z

Time was not monotonic for a WACCM ua file, so we fixed it.
Fixing the bounds did not work. We assume because iris.guess_bounds() does not work for coordinate time.

bouweandela · 2019-08-23T14:38:58Z

@jvegasbsc can you please review this?

esmvalcore/cmor/_fixes/cmip6/cesm2_waccm.py

bouweandela · 2019-11-15T08:43:17Z

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

+
+
+def test_ua_fix_metadata(ua_cubes):
+    expected_coord = iris.coords.AuxCoord(


Suggested change

expected_coord = iris.coords.AuxCoord(

expected_coord = iris.coords.DimCoord(

bouweandela · 2019-11-15T08:47:03Z

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

+        [1.0, 2.0, 3.0], standard_name='time', units='days since 1850-01-01',
+        bounds=None)
+    expected_cube = iris.cube.Cube([7.0, 9.0, 8.0], var_name='ua',
+                                   aux_coords_and_dims=[(expected_coord, 0)])


Suggested change

aux_coords_and_dims=[(expected_coord, 0)])

dim_coords_and_dims=[(expected_coord, 0)])

bouweandela · 2019-11-15T08:51:39Z

esmvalcore/cmor/_fixes/cmip6/cesm2_waccm.py

+        idx_sorted = np.argsort(coord.points)
+        coord.points = coord.points[idx_sorted]
+        coord.bounds = None
+        # coord.guess_bounds('time')  # fails for coordinate 'time'


What is failing exactly?

Agree with @bouweandela question. @hb326, note that coord already refers to time, so I am not sure why there is another mention of time in the args. Certainly guess_bounds works with time coordinates in general.

looks like time could be only AuxCoord in some cases and the time points are mangled - but if the data has enough dimensions to account for a (pseudo)time coordinate then what's in DimCoords that replaces time?

JaroCamphuijsen · 2019-11-28T08:35:49Z

@hb326 can you have a look at the comments above? Then we can probably merge this.

Co-authored-by: Bouwe Andela <[email protected]>

valeriupredoi · 2021-02-08T12:24:55Z

OK I started committing stuff that @bouweandela suggested (I know, I'm nasty - changing code in a PR that has nothing to do with meself heh, but was trying to see if we can get this through to the 2.2 release) but I stopped coz it looks like we really need @hb326 to have a look and comment/commit 👍

hb326 · 2021-05-04T09:50:01Z

OK I started committing stuff that @bouweandela suggested (I know, I'm nasty - changing code in a PR that has nothing to do with meself heh, but was trying to see if we can get this through to the 2.2 release) but I stopped coz it looks like we really need @hb326 to have a look and comment/commit 👍

I am looking at it now. Sorry, took a while. Hopefully we can get it sorted for release 2.3!

valeriupredoi · 2021-05-04T10:35:28Z

cool stuff! just ping me here if you need a review 🍺

hb326 · 2021-05-04T12:56:38Z

@valeriupredoi: it seems like the problem vanished! I just now ran a recipe where I try to read the WACCM data without any fix, in different variations (historical, amip, piControl), and it seems like everything is read correctly. Could you please check if you can read the following data, no need for a script, just reading the data with the ESMValTool:

{dataset: CESM2-WACCM, project: CMIP6, mip: day, exp: historical, grid: gn, ensemble: r1i1p1f1, start_year: 1960, end_year: 2014}

Maybe some update to the Core has fixed the problem without the dataset fix?

hb326 · 2021-05-04T13:12:46Z

@valeriupredoi: it seems like the problem vanished! I just now ran a recipe where I try to read the WACCM data without any fix, in different variations (historical, amip, piControl), and it seems like everything is read correctly. Could you please check if you can read the following data, no need for a script, just reading the data with the ESMValTool:
* {dataset: CESM2-WACCM,  project: CMIP6,  mip: day,  exp: historical,  grid: gn, ensemble: r1i1p1f1,  start_year: 1960,  end_year: 2014}
Maybe some update to the Core has fixed the problem without the dataset fix?

I should add that I am using the current master branch for the ESMValTool and the ESMValCore for this test.

hb326 · 2021-05-06T11:44:32Z

@valeriupredoi: did you check yet if WACCM would run for you without fix as well?

hb326 · 2021-05-11T09:34:17Z

@valeriupredoi: this was one of the PR I wanted you to check on.

valeriupredoi

@hb326 I understand the special case but I think it's best you add a note (in-line comment) about this particular case (it was visible to me from the test case, cheers for adding it!). But my question is - what sort of DimCoord does the cube have to account for the time axis, if time itself is an AuxCoord

valeriupredoi · 2021-05-11T10:36:40Z

esmvalcore/cmor/_fixes/cmip6/cesm2_waccm.py

+        idx_sorted = np.argsort(coord.points)
+        coord.points = coord.points[idx_sorted]
+        coord.bounds = None
+        # coord.guess_bounds('time')  # fails for coordinate 'time'


looks like time could be only AuxCoord in some cases and the time points are mangled - but if the data has enough dimensions to account for a (pseudo)time coordinate then what's in DimCoords that replaces time?

valeriupredoi · 2021-05-11T10:37:39Z

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

+    ta_cube = iris.cube.Cube([1.0], var_name='ta')
+    time_coord = iris.coords.AuxCoord(
+        [1.0, 3.0, 2.0], standard_name='time', units='days since 1850-01-01',
+        bounds=[[0.5, 1.5], [2.5, 3.5], [1.5, 2.5]])


ah yes, this is accounted for in this test case @bouweandela and @zklaus

valeriupredoi · 2021-05-11T10:45:12Z

@hb326 could you pls do a mergy merge of master into this, some bits are causing age-related issues, and fix the conflicts as well pls (lemme know if you need help with that) 🍺

hb326 · 2021-05-11T11:26:22Z

@hb326 could you pls do a mergy merge of master into this, some bits are causing age-related issues, and fix the conflicts as well pls (lemme know if you need help with that) 🍺

@valeriupredoi, my thinking is that we can just forget this pull request since it seems to me that some change in the ESMValCore actually fixed the problem for which I had written the fix. Did you try to read in the WACCM ua data with the current master branch of ESMValCore and ESMValTool? Because for me it works like a charm, WITHOUT using the fix.

valeriupredoi · 2021-05-11T12:04:43Z

@hb326 could you pls do a mergy merge of master into this, some bits are causing age-related issues, and fix the conflicts as well pls (lemme know if you need help with that) beer

@valeriupredoi, my thinking is that we can just forget this pull request since it seems to me that some change in the ESMValCore actually fixed the problem for which I had written the fix. Did you try to read in the WACCM ua data with the current master branch of ESMValCore and ESMValTool? Because for me it works like a charm, WITHOUT using the fix.

I'll have a looksee now 👍

valeriupredoi · 2021-05-11T12:27:17Z

the file on JASMIN which is version 27-Feb-2019 I get after a raw CMOR check (just by importing the checker and running it):

esmvalcore.cmor.check.CMORCheckError: There were errors in variable ua:
lon: units should be degrees_east, not degrees
 lat: units should be degrees_north, not degrees
 in cube:
eastward_wind / (m s-1)             (time: 1980; air_pressure: 19; latitude: 192; longitude: 288)

so all fine, no issues with time 👍
Up to you if you just wanna nuke the PR, Birgit 🍺

hb326 · 2021-05-11T12:41:26Z

Ok, it does seem like the problem that this PR was supposed to fix was fixed with an update to the ESMValCore recently. So this PR is redundant.

valeriupredoi · 2021-05-11T12:45:06Z

fairly sure it's the data that got updated @hb326

bouweandela · 2021-05-11T13:49:16Z

Can the branch be deleted?

hb326 · 2021-05-11T15:23:25Z

Can the branch be deleted?

@bouweandela, I would say yes, it can be deleted.

valeriupredoi · 2021-05-11T15:58:22Z

brancho deletado, cheers @hb326

added fix for WACCM ua time coordinate

96a15d0

hb326 added the fix for dataset Related to dataset-specific fix files label Aug 6, 2019

hb326 requested review from bouweandela and jvegreg August 6, 2019 11:51

hb326 self-assigned this Aug 6, 2019

Added check if WACCM fix is really necessary

6218c8c

bouweandela removed their request for review August 23, 2019 14:38

bouweandela reviewed Nov 15, 2019

View reviewed changes

esmvalcore/cmor/_fixes/cmip6/cesm2_waccm.py Outdated Show resolved Hide resolved

bouweandela reviewed Nov 15, 2019

View reviewed changes

bouweandela changed the base branch from development to master January 3, 2020 12:12

Merge remote-tracking branch 'origin/master' into waccm_ua_fix

3b5a1c4

bouweandela mentioned this pull request Sep 24, 2020

Topics for ESMValTool workshop in November ESMValGroup/ESMValTool#1817

Closed

bouwe call

41d9681

Co-authored-by: Bouwe Andela <[email protected]>

valeriupredoi requested changes May 11, 2021

View reviewed changes

hb326 closed this May 11, 2021

valeriupredoi deleted the waccm_ua_fix branch May 11, 2021 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added fix for WACCM ua time coordinate #200

added fix for WACCM ua time coordinate #200

hb326 commented Aug 6, 2019

bouweandela commented Aug 23, 2019

bouweandela Nov 15, 2019

bouweandela Nov 15, 2019

bouweandela Nov 15, 2019

zklaus May 4, 2021

valeriupredoi May 11, 2021

JaroCamphuijsen commented Nov 28, 2019

valeriupredoi commented Feb 8, 2021 •

edited

Loading

hb326 commented May 4, 2021

valeriupredoi commented May 4, 2021

hb326 commented May 4, 2021 •

edited

Loading

hb326 commented May 4, 2021

hb326 commented May 6, 2021

hb326 commented May 11, 2021

valeriupredoi left a comment

valeriupredoi May 11, 2021

valeriupredoi May 11, 2021

valeriupredoi commented May 11, 2021

hb326 commented May 11, 2021

valeriupredoi commented May 11, 2021

valeriupredoi commented May 11, 2021

hb326 commented May 11, 2021

valeriupredoi commented May 11, 2021

bouweandela commented May 11, 2021

hb326 commented May 11, 2021

valeriupredoi commented May 11, 2021



		def test_ua_fix_metadata(ua_cubes):
		expected_coord = iris.coords.AuxCoord(

	expected_coord = iris.coords.AuxCoord(
	expected_coord = iris.coords.DimCoord(

	aux_coords_and_dims=[(expected_coord, 0)])
	dim_coords_and_dims=[(expected_coord, 0)])

added fix for WACCM ua time coordinate #200

added fix for WACCM ua time coordinate #200

Conversation

hb326 commented Aug 6, 2019

bouweandela commented Aug 23, 2019

bouweandela Nov 15, 2019

Choose a reason for hiding this comment

bouweandela Nov 15, 2019

Choose a reason for hiding this comment

bouweandela Nov 15, 2019

Choose a reason for hiding this comment

zklaus May 4, 2021

Choose a reason for hiding this comment

valeriupredoi May 11, 2021

Choose a reason for hiding this comment

JaroCamphuijsen commented Nov 28, 2019

valeriupredoi commented Feb 8, 2021 • edited Loading

hb326 commented May 4, 2021

valeriupredoi commented May 4, 2021

hb326 commented May 4, 2021 • edited Loading

hb326 commented May 4, 2021

hb326 commented May 6, 2021

hb326 commented May 11, 2021

valeriupredoi left a comment

Choose a reason for hiding this comment

valeriupredoi May 11, 2021

Choose a reason for hiding this comment

valeriupredoi May 11, 2021

Choose a reason for hiding this comment

valeriupredoi commented May 11, 2021

hb326 commented May 11, 2021

valeriupredoi commented May 11, 2021

valeriupredoi commented May 11, 2021

hb326 commented May 11, 2021

valeriupredoi commented May 11, 2021

bouweandela commented May 11, 2021

hb326 commented May 11, 2021

valeriupredoi commented May 11, 2021

valeriupredoi commented Feb 8, 2021 •

edited

Loading

hb326 commented May 4, 2021 •

edited

Loading