-
Notifications
You must be signed in to change notification settings - Fork 300
Fix MDI and preserve lazy data in PP saves. #2744
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ | |
| import math | ||
| import os | ||
| import os.path | ||
| import re | ||
| import shutil | ||
| import subprocess | ||
| import sys | ||
|
|
@@ -527,6 +528,28 @@ def assertRaisesRegexp(self, *args, **kwargs): | |
| return six.assertRaisesRegex(super(IrisTest_nometa, self), | ||
| *args, **kwargs) | ||
|
|
||
| @contextlib.contextmanager | ||
| def assertGivesWarning(self, expected_regexp='', expect_warning=True): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This name could be confusing since this function can be used to check that code doesn't give a warning. |
||
| # Check that a warning is raised matching a given expression, or that | ||
| # no warning matching the given expression is raised. | ||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter('always') | ||
| yield | ||
| messages = [str(warning.message) for warning in w] | ||
| expr = re.compile(expected_regexp) | ||
| matches = [message for message in messages if expr.search(message)] | ||
| warning_raised = any(matches) | ||
| if expect_warning: | ||
| if not warning_raised: | ||
| msg = "Warning matching '{}' not raised." | ||
| msg = msg.format(expected_regexp) | ||
| self.assertEqual(expect_warning, warning_raised, msg) | ||
| else: | ||
| if warning_raised: | ||
| msg = "Unexpected warning(s) raised, matching '{}' : {!r}." | ||
| msg = msg.format(expected_regexp, matches) | ||
| self.assertEqual(expect_warning, warning_raised, msg) | ||
|
|
||
| def _assertMaskedArray(self, assertion, a, b, strict, **kwargs): | ||
| # Define helper function to extract unmasked values as a 1d | ||
| # array. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1199,7 +1199,8 @@ def test_save_and_merge(self): | |
| self.assertEqual(len(merged_cubes), 1, "expected a single merged cube") | ||
| merged_cube = merged_cubes[0] | ||
| self.assertEqual(merged_cube.dtype, dtype) | ||
| self.assertEqual(merged_cube.data.fill_value, fill_value) | ||
| # Check that the original masked-array fill-value is *ignored*. | ||
| self.assertArrayAllClose(merged_cube.data.fill_value, -1e30) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "assert_array_almost_equal" applies an absolute precision, 6 decimal places by default, i.e. value differences up to +/-0.000001. The problem is a mismatch between a float64 and float32 versions of "-1e30". That's why I prefer the "assert_allclose" approach -- the use of relative as well as absolute tolerance is more flexible, and also makes for a better default behaviour. |
||
|
|
||
|
|
||
| @tests.skip_data | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,9 @@ | |
| # importing anything else. | ||
| import iris.tests as tests | ||
|
|
||
| from contextlib import contextmanager | ||
| import warnings | ||
|
|
||
| import numpy as np | ||
|
|
||
| import iris.fileformats.pp as pp | ||
|
|
@@ -37,18 +40,22 @@ | |
| # items when written to disk and get consistent results. | ||
|
|
||
|
|
||
| DUMMY_HEADER = [('dummy1', (0, 13)), | ||
| DUMMY_HEADER = [('dummy1', (0, 11)), | ||
| ('lbtim', (12,)), | ||
| ('dummy2', (13,)), | ||
| ('lblrec', (14,)), | ||
| ('dummy2', (15, 18)), | ||
| ('dummy3', (15, 16)), | ||
| ('lbrow', (17,)), | ||
| ('dummy4', (18,)), | ||
| ('lbext', (19,)), | ||
| ('lbpack', (20,)), | ||
| ('dummy3', (21, 37)), | ||
| ('dummy5', (21, 37)), | ||
| ('lbuser', (38, 39, 40, 41, 42, 43, 44,)), | ||
| ('brsvd', (45, 46, 47, 48)), | ||
| ('bdatum', (49,)), | ||
| ('dummy4', (45, 63)), | ||
| ('dummy6', (50, 61)), | ||
| ('bmdi', (62, )), | ||
| ('dummy7', (63,)), | ||
| ] | ||
|
|
||
|
|
||
|
|
@@ -57,33 +64,40 @@ class TestPPField(PPField): | |
| HEADER_DEFN = DUMMY_HEADER | ||
| HEADER_DICT = dict(DUMMY_HEADER) | ||
|
|
||
| def _ready_for_save(self): | ||
| self.dummy1 = 0 | ||
| self.dummy2 = 0 | ||
| self.dummy3 = 0 | ||
| self.dummy4 = 0 | ||
| self.dummy5 = 0 | ||
| self.dummy6 = 0 | ||
| self.dummy7 = 0 | ||
| self.lbtim = 0 | ||
| self.lblrec = 0 | ||
| self.lbrow = 0 | ||
| self.lbext = 0 | ||
| self.lbpack = 0 | ||
| self.lbuser = 0 | ||
| self.brsvd = 0 | ||
| self.bdatum = 0 | ||
| self.bmdi = -1e30 | ||
| return self | ||
|
|
||
| @property | ||
| def t1(self): | ||
| return netcdftime.datetime(2013, 10, 14, 10, 4) | ||
| return None | ||
|
|
||
| @property | ||
| def t2(self): | ||
| return netcdftime.datetime(2013, 10, 14, 10, 5) | ||
| return None | ||
|
|
||
|
|
||
| class Test_save(tests.IrisTest): | ||
| def test_float64(self): | ||
| # Tests down-casting of >f8 data to >f4. | ||
|
|
||
| def field_checksum(data): | ||
| field = TestPPField() | ||
| field.dummy1 = 0 | ||
| field.dummy2 = 0 | ||
| field.dummy3 = 0 | ||
| field.dummy4 = 0 | ||
| field.lbtim = 0 | ||
| field.lblrec = 0 | ||
| field.lbrow = 0 | ||
| field.lbext = 0 | ||
| field.lbpack = 0 | ||
| field.lbuser = 0 | ||
| field.brsvd = 0 | ||
| field.bdatum = 0 | ||
| field = TestPPField()._ready_for_save() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it's nit-picky, but why make |
||
| field.data = data | ||
| with self.temp_filename('.pp') as temp_filename: | ||
| with open(temp_filename, 'wb') as pp_file: | ||
|
|
@@ -93,14 +107,49 @@ def field_checksum(data): | |
|
|
||
| data_64 = np.linspace(0, 1, num=10, endpoint=False).reshape(2, 5) | ||
| checksum_32 = field_checksum(data_64.astype('>f4')) | ||
| with mock.patch('warnings.warn') as warn: | ||
| msg = 'Downcasting array precision from float64 to float32 for save.' | ||
| with self.assertGivesWarning(msg): | ||
| checksum_64 = field_checksum(data_64.astype('>f8')) | ||
|
|
||
| self.assertEqual(checksum_32, checksum_64) | ||
| warn.assert_called_once_with( | ||
| 'Downcasting array precision from float64 to float32 for save.' | ||
| 'If float64 precision is required then please save in a ' | ||
| 'different format') | ||
|
|
||
| def test_masked_mdi_value_warning(self): | ||
| # Check that an unmasked MDI value raises a warning. | ||
| field = TestPPField()._ready_for_save() | ||
| field.bmdi = -123.4 | ||
| # Make float32 data, as float64 default produces an extra warning. | ||
| field.data = np.ma.masked_array([1., field.bmdi, 3.], dtype=np.float32) | ||
| msg = 'PPField data contains unmasked points' | ||
| with self.assertGivesWarning(msg): | ||
| with self.temp_filename('.pp') as temp_filename: | ||
| with open(temp_filename, 'wb') as pp_file: | ||
| field.save(pp_file) | ||
|
|
||
| def test_unmasked_mdi_value_warning(self): | ||
| # Check that MDI in *unmasked* data raises a warning. | ||
| field = TestPPField()._ready_for_save() | ||
| field.bmdi = -123.4 | ||
| # Make float32 data, as float64 default produces an extra warning. | ||
| field.data = np.array([1., field.bmdi, 3.], dtype=np.float32) | ||
| msg = 'PPField data contains unmasked points' | ||
| with self.assertGivesWarning(msg): | ||
| with self.temp_filename('.pp') as temp_filename: | ||
| with open(temp_filename, 'wb') as pp_file: | ||
| field.save(pp_file) | ||
|
|
||
| def test_mdi_masked_value_nowarning(self): | ||
| # Check that a *masked* MDI value does not raise a warning. | ||
| field = TestPPField()._ready_for_save() | ||
| field.bmdi = -123.4 | ||
| # Make float32 data, as float64 default produces an extra warning. | ||
| field.data = np.ma.masked_array([1., 2., 3.], mask=[0, 1, 0], | ||
| dtype=np.float32) | ||
| # Set underlying data value at masked point to BMDI value. | ||
| field.data.data[1] = field.bmdi | ||
| self.assertArrayAllClose(field.data.data[1], field.bmdi) | ||
| with self.assertGivesWarning(r'\(mask\|fill\)', expect_warning=False): | ||
| with self.temp_filename('.pp') as temp_filename: | ||
| with open(temp_filename, 'wb') as pp_file: | ||
| field.save(pp_file) | ||
|
|
||
|
|
||
| class Test_calendar(tests.IrisTest): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you not allowing the BMDI to be specified via a keyword to
iris.save?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On consideration, I didn't think it was necessary.
It's not the same as the netdcf case, because we don't have to worry about smaller integer types. So, especially if we don't provide an option to easily change BMDI, then chance collisions are extremely unlikely. Likewise, more recent version of the UM spec ("F3 document") don't actually admit the use of non-standard BMDI values.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not completely sold - I think there's an argument for providing a uniform interface for both PP and NetCDF. But I think it could easily be changed later (post Iris 2 release) without breaking anything so I won't worry about it now.