Fix cftime gt 101 #3259

pp-mo · 2019-02-04T11:08:30Z

Hopefully, this fixes test failures resulting from changes in cftime > 1.0.1.

The test problems seem to be confined to the PP/FF loading.
Hence the approach here.
It's still possible that the problem could occur in other areas though.
(Including knock-on problems with other tests, that this will turn up...)

Addresses #3248

Note : This ALSO unpins cftime

lib/iris/tests/unit/fileformats/pp_load_rules/test__epoch_date_hours.py

lbdreyer

Looks good @pp-mo !

I agree with the approach: it is best to keep consistent with the old behaviour rather than break compatibility.

I just have a couple of issues relating to the handling of the handling of the time component of the datetime and the years in the copyright message.
Also this PR will need rebasing as it is complaining about the requirements/core.txt file/

lbdreyer · 2019-02-20T10:25:33Z

lib/iris/fileformats/pp_load_rules.py

+    # Return an 'hours since epoch' number for a date.
+    # Also handle dates with a zero year, month or day in a special way : such
+    # dates were valid inputs for 'date2num' up to cftime version 1.0.1,
+    # but not since:  Results here are consistent with the "old" behaviour.


Why this style docstring rather than just

def _epoch_date_hours(): """ Returns ... """"

Ok. will fix

lbdreyer · 2019-02-20T10:32:53Z

lib/iris/tests/integration/test_pp.py

+#                               lbuser=[0] * 7, lbrsvd=[0] * 4,
+#                               brsvd=[0] * 4, brlev=0,
+#                               core_data=mock_core_data,
+#                               realised_dtype=mock_data.dtype)


Are you leaving these commented out lines for a specific reason?

No. Oops, will remove !

lbdreyer · 2019-02-20T10:36:03Z

lib/iris/tests/integration/test_pp.py

+        field._x_coord_name = lambda: 'longitude'
+        field._y_coord_name = lambda: 'latitude'
+        field.coord_system = lambda: None
+        field.configure_mock(**kwargs)


I definitely prefer approach with adding **kwargs here 👍

lbdreyer · 2019-02-20T10:44:31Z

lib/iris/fileformats/pp_load_rules.py

+    # dates were valid inputs for 'date2num' up to cftime version 1.0.1,
+    # but not since:  Results here are consistent with the "old" behaviour.
+    days_offset = None
+    if (datetime.year < 1 or datetime.month < 1 or datetime.day < 1):


Does the pp/ff spec ever allow for these to be negative values?

cftime (1.0.3.4) does allow it:

>>> cftime.datetime(year=-1, month=10, day=3) cftime._cftime.datetime(-1, 10, 3, 0, 0, 0, 0, -1, 1)

Although I guess it wouldn't matter as they they would still pass straight though as lower down (e.g. line 461) you check y==0
So why do datetime.year < 1 here but y ==0 further down?

Does the pp/ff spec ever allow for these to be negative values?

No, I don't think that can happen.

why do datetime.year < 1 here but y ==0 further down?

I think it was less characters, this just barely fits on one line !!
I will tidy up ...

lbdreyer · 2019-02-20T10:57:25Z

lib/iris/fileformats/pp_load_rules.py

+            days_offset += days_in_year_0
+
+        # Replace with a modified date, that cftime will accept.
+        datetime = nc_datetime(y, m, d)


What has happened to the time component of the datetime (i.e. the hour, minute, second)?

Ooh, good spot !! 💐
I will fix.

lbdreyer · 2019-02-20T11:03:50Z

lib/iris/tests/unit/fileformats/pp_load_rules/test__epoch_date_hours.py

@@ -0,0 +1,134 @@
+# (C) British Crown Copyright 2014 - 2017, Met Office


Need to change these years. And it looks you need to update the dates on the other files as well

Ok, will fix.

It's annoying that this test has stopped working in Travis, not sure when that happened but several headers are now out-of-date.
There was also talk of ditching the file headers, or at least the dates, but discussions with internal legal team are not complete AFAIK.
#3272 Attempts to fix ..

Can now wait for #3272, and rebase when that is in.

pp-mo · 2019-02-21T17:03:18Z

Thanks for your engagement @lbdreyer
I think you also suggested that this should target v2.2.x ?
I'd like to resolve the issues you've raised first though.

pp-mo · 2019-02-21T17:04:50Z

Hi @lbdreyer hope this addresses the outstanding.
I added a testcase for 'not dropping the time-of-day'.

lib/iris/fileformats/pp_load_rules.py

lbdreyer · 2019-02-25T15:56:26Z

Should this not target the 2.2.x branch?

pp-mo · 2019-02-25T18:13:07Z

@lbdreyer Should this not target the 2.2.x branch?

Ok, now re-targetted.
N.B.

Note : This ALSO unpins cftime
Note : This now tests against cf-units rather than cf_units (old name no longer updated), which is like this change, now on master

lbdreyer · 2019-02-26T15:31:12Z

requirements/core.txt

@@ -5,8 +5,8 @@

 cartopy
 #conda: proj4<5
-cf-units>=2
-cftime==1.0.1
+cf_units>=2


Can you undo the change to cf-units?
It would be better to keep the hyphened name

I think this came from it still being cf_units on master, as the fix has gone onto the v2.2x branch which has not yet been mergedback onto master

My mistake, I think I was reading the diffs the wrong way round !

pp-mo · 2019-02-26T17:54:33Z

@lbdreyer undo the change to cf-units?

👍 I have force-pushed again, to avoid confusion when we merge back later

SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 4, 2019

pp-mo changed the base branch from v2.2.x to master February 4, 2019 11:08

SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 4, 2019

stickler-ci reviewed Feb 4, 2019

View reviewed changes

This was referenced Feb 5, 2019

cftime again #3239

Closed

Unpin cftime #3260

Closed

lbdreyer reviewed Feb 20, 2019

View reviewed changes

lbdreyer added the Release: Patch label Feb 21, 2019

stickler-ci reviewed Feb 21, 2019

View reviewed changes

lib/iris/fileformats/pp_load_rules.py Outdated Show resolved Hide resolved

lbdreyer added this to the v2.2.1 milestone Feb 22, 2019

pp-mo mentioned this pull request Feb 23, 2019

Get licence header test working again. #3272

Merged

pp-mo changed the base branch from master to v2.2.x February 25, 2019 17:39

SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 25, 2019

pp-mo force-pushed the fix_cftime_gt_101 branch from 8c479a3 to 57ed1e7 Compare February 25, 2019 18:08

SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 25, 2019

lbdreyer reviewed Feb 26, 2019

View reviewed changes

Fix for non-calendar datetimes.

3e0672f

pp-mo force-pushed the fix_cftime_gt_101 branch from 57ed1e7 to 3e0672f Compare February 26, 2019 17:51

lbdreyer merged commit b7cd98a into SciTools:v2.2.x Feb 27, 2019

lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request May 28, 2019

Fix for non-calendar datetimes. (SciTools#3259)

7cb289a

lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request Jun 3, 2019

Fix for non-calendar datetimes. (SciTools#3259)

97dea5d

lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request Jun 3, 2019

Fix for non-calendar datetimes. (SciTools#3259)

bbcfae7

rcomer mentioned this pull request Jun 7, 2019

Adding a utility for flattening Aux Coords #3030

Closed

rcomer mentioned this pull request Sep 13, 2019

Fix "zero dates" handling for cftime > 1.0.1 #3248

Closed

pp-mo deleted the fix_cftime_gt_101 branch October 17, 2019 10:46

		@@ -0,0 +1,134 @@
		# (C) British Crown Copyright 2014 - 2017, Met Office

Fix cftime gt 101 #3259

Fix cftime gt 101 #3259

Uh oh!

Conversation

pp-mo commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lbdreyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Feb 21, 2019

Uh oh!

pp-mo commented Feb 21, 2019

Uh oh!

Uh oh!

lbdreyer commented Feb 25, 2019

Uh oh!

pp-mo commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbdreyer Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pp-mo commented Feb 4, 2019 •

edited

Loading

pp-mo commented Feb 25, 2019 •

edited

Loading

lbdreyer Feb 26, 2019 •

edited

Loading

pp-mo commented Feb 26, 2019 •

edited

Loading