From 4ab7cfaf2ea920085336aff02089b7b1f03df01b Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Fri, 7 Dec 2018 11:22:26 +0000 Subject: [PATCH 01/13] ensure cubelists only contain cubes --- ...ix_2018-Dec-07_only_cubes_in_cubelists.txt | 3 + lib/iris/cube.py | 47 ++++++++++++-- lib/iris/tests/unit/cube/test_CubeList.py | 62 +++++++++++++++++++ 3 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 docs/iris/src/whatsnew/contributions_2.3.0/bugfix_2018-Dec-07_only_cubes_in_cubelists.txt diff --git a/docs/iris/src/whatsnew/contributions_2.3.0/bugfix_2018-Dec-07_only_cubes_in_cubelists.txt b/docs/iris/src/whatsnew/contributions_2.3.0/bugfix_2018-Dec-07_only_cubes_in_cubelists.txt new file mode 100644 index 0000000000..2af5b2f537 --- /dev/null +++ b/docs/iris/src/whatsnew/contributions_2.3.0/bugfix_2018-Dec-07_only_cubes_in_cubelists.txt @@ -0,0 +1,3 @@ +* The `append`, `extend` and `insert` methods of :class:`iris.cube.CubeList` +now perform a check to ensure that only :class:`iris.cube.Cube` instances are +added. diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 81152a1293..62e201f662 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -196,10 +196,7 @@ def __new__(cls, list_of_cubes=None): """Given a :class:`list` of cubes, return a CubeList instance.""" cube_list = list.__new__(cls, list_of_cubes) - # Check that all items in the incoming list are cubes. Note that this - # checking does not guarantee that a CubeList instance *always* has - # just cubes in its list as the append & __getitem__ methods have not - # been overridden. + # Check that all items in the incoming list are cubes. if not all([isinstance(cube, Cube) for cube in cube_list]): raise ValueError('All items in list_of_cubes must be Cube ' 'instances.') @@ -241,6 +238,48 @@ def __getslice__(self, start, stop): result = CubeList(result) return result + def append(self, cube): + """ + Append a cube. + """ + if isinstance(cube, Cube): + super(CubeList, self).append(cube) + else: + raise TypeError('Only Cube instances can be appended to cubelists.' + ' Got {}.'.format(type(cube))) + + def extend(self, other_cubes): + """ + Extend cubelist by appending the cubes contained in other_cubes. + + Args: + + * other_cubes: + A cubelist or other sequence of cubes. + """ + if isinstance(other_cubes, Cube): + raise TypeError( + 'Use append to add a single cube to the cubelist.') + elif not isinstance(other_cubes, collections.Iterable): + raise TypeError( + 'Can only extend with a sequece of Cube instances.') + elif isinstance(other_cubes, CubeList) or all( + [isinstance(cube, Cube) for cube in other_cubes]): + super(CubeList, self).extend(other_cubes) + else: + raise TypeError('All items in other_cubes must be Cube ' + 'instances.') + + def insert(self, index, cube): + """ + Insert a cube before index. + """ + if isinstance(cube, Cube): + super(CubeList, self).insert(index, cube) + else: + raise TypeError('Only Cube instances can be inserted into ' + 'cubelists. Got {}.'.format(type(cube))) + def xml(self, checksum=False, order=True, byteorder=True): """Return a string of the XML that this list of cubes represents.""" doc = Document() diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index 258408c6f8..0a13f0aabb 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -35,6 +35,25 @@ from iris.tests import mock +class Test_append(tests.IrisTest): + def setUp(self): + self.cubelist = iris.cube.CubeList() + self.cube1 = iris.cube.Cube(1, long_name='foo') + self.cube2 = iris.cube.Cube(1, long_name='bar') + + def test_pass(self): + self.cubelist.append(self.cube1) + self.assertEqual(self.cubelist[-1], self.cube1) + self.cubelist.append(self.cube2) + self.assertEqual(self.cubelist[-1], self.cube2) + + def test_fail(self): + msg = ("Only Cube instances can be appended to cubelists. Got " + "") + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist.append(None) + + class Test_concatenate_cube(tests.IrisTest): def setUp(self): self.units = Unit('days since 1970-01-01 00:00:00', @@ -64,6 +83,32 @@ def test_empty(self): CubeList([]).concatenate_cube() +class Test_extend(tests.IrisTest): + def setUp(self): + self.cube1 = iris.cube.Cube(1, long_name='foo') + self.cube2 = iris.cube.Cube(1, long_name='bar') + self.cubelist1 = iris.cube.CubeList([self.cube1]) + self.cubelist2 = iris.cube.CubeList([self.cube2]) + + def test_pass(self): + cubelist = self.cubelist1.copy() + cubelist.extend(self.cubelist2) + self.assertEqual(cubelist, self.cubelist1 + self.cubelist2) + cubelist.extend([self.cube2]) + self.assertEqual(cubelist[-1], self.cube2) + + def test_fail(self): + msg = 'Use append to add a single cube to the cubelist.' + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist1.extend(self.cube1) + msg = 'Can only extend with a sequece of Cube instances.' + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist1.extend(None) + msg = 'All items in other_cubes must be Cube instances.' + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist1.extend(range(3)) + + class Test_extract_overlapping(tests.IrisTest): def setUp(self): shape = (6, 14, 19) @@ -118,6 +163,23 @@ def test_different_orders(self): self.assertEqual(b.coord('time'), self.cube.coord('time')[2:4]) +class Test_insert(tests.IrisTest): + def setUp(self): + self.cube1 = iris.cube.Cube(1, long_name='foo') + self.cube2 = iris.cube.Cube(1, long_name='bar') + self.cubelist = iris.cube.CubeList([self.cube1] * 3) + + def test_pass(self): + self.cubelist.insert(1, self.cube2) + self.assertEqual(self.cubelist[1], self.cube2) + + def test_fail(self): + msg = ("Only Cube instances can be inserted into cubelists. Got " + "") + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist.insert(0, None) + + class Test_merge_cube(tests.IrisTest): def setUp(self): self.cube1 = Cube([1, 2, 3], "air_temperature", units="K") From 0b293ec73f0e6eeadbb2d6814d18098b0beaae7d Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Wed, 12 Dec 2018 17:47:41 +0000 Subject: [PATCH 02/13] address __iadd__ and __setitem__ --- lib/iris/cube.py | 28 +++++++++++++++++++--- lib/iris/tests/unit/cube/test_CubeList.py | 29 +++++++++++++++++++++-- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 62e201f662..aec0c66036 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -238,6 +238,28 @@ def __getslice__(self, start, stop): result = CubeList(result) return result + def __iadd__(self, other_cubes): + """ + Add a sequence of cubes to the cubelist in place. + """ + if isinstance(other_cubes, Cube) or not isinstance( + other_cubes, collections.Iterable): + raise TypeError( + 'Can only add a sequence of Cube instances.') + elif isinstance(other_cubes, CubeList) or all( + [isinstance(cube, Cube) for cube in other_cubes]): + super(CubeList, self).__iadd__(other_cubes) + else: + raise ValueError('All items in added sequence must be Cube ' + 'instances.') + + def __setitem__(self, key, cube): + """Set self[key] to cube""" + if isinstance(cube, Cube): + super(CubeList, self).__setitem__(key, cube) + else: + raise TypeError('Elements of cubelists must be Cube instances') + def append(self, cube): """ Append a cube. @@ -262,13 +284,13 @@ def extend(self, other_cubes): 'Use append to add a single cube to the cubelist.') elif not isinstance(other_cubes, collections.Iterable): raise TypeError( - 'Can only extend with a sequece of Cube instances.') + 'Can only extend with a sequence of Cube instances.') elif isinstance(other_cubes, CubeList) or all( [isinstance(cube, Cube) for cube in other_cubes]): super(CubeList, self).extend(other_cubes) else: - raise TypeError('All items in other_cubes must be Cube ' - 'instances.') + raise ValueError('All items in other_cubes must be Cube ' + 'instances.') def insert(self, index, cube): """ diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index 0a13f0aabb..25984bfc62 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -101,11 +101,11 @@ def test_fail(self): msg = 'Use append to add a single cube to the cubelist.' with self.assertRaisesRegexp(TypeError, msg): self.cubelist1.extend(self.cube1) - msg = 'Can only extend with a sequece of Cube instances.' + msg = 'Can only extend with a sequence of Cube instances.' with self.assertRaisesRegexp(TypeError, msg): self.cubelist1.extend(None) msg = 'All items in other_cubes must be Cube instances.' - with self.assertRaisesRegexp(TypeError, msg): + with self.assertRaisesRegexp(ValueError, msg): self.cubelist1.extend(range(3)) @@ -163,6 +163,31 @@ def test_different_orders(self): self.assertEqual(b.coord('time'), self.cube.coord('time')[2:4]) +class Test_iadd(tests.IrisTest): + def setUp(self): + self.cube1 = iris.cube.Cube(1, long_name='foo') + self.cube2 = iris.cube.Cube(1, long_name='bar') + self.cubelist1 = iris.cube.CubeList([self.cube1]) + self.cubelist2 = iris.cube.CubeList([self.cube2]) + + def test_pass(self): + cubelist = self.cubelist1.copy() + cubelist += self.cubelist2 + self.assertEqual(cubelist, self.cubelist1 + self.cubelist2) + cubelist += [self.cube2] + self.assertEqual(cubelist[-1], self.cube2) + + def test_fail(self): + msg = 'Can only add a sequence of Cube instances.' + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist1 += self.cube1 + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist1 += 1. + msg = 'All items in added sequence must be Cube instances.' + with self.assertRaisesRegexp(ValueError, msg): + self.cubelist1 += range(3) + + class Test_insert(tests.IrisTest): def setUp(self): self.cube1 = iris.cube.Cube(1, long_name='foo') From 8a74df986150d6ad2ff2ea5be1d6e4d60db05a29 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Wed, 12 Dec 2018 18:01:06 +0000 Subject: [PATCH 03/13] __setitem__ tests --- lib/iris/cube.py | 3 ++- lib/iris/tests/unit/cube/test_CubeList.py | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index aec0c66036..41bef7478f 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -258,7 +258,8 @@ def __setitem__(self, key, cube): if isinstance(cube, Cube): super(CubeList, self).__setitem__(key, cube) else: - raise TypeError('Elements of cubelists must be Cube instances') + raise TypeError('Elements of cubelists must be Cube instances. ' + 'Got {}.'.format(type(cube))) def append(self, cube): """ diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index 25984bfc62..e82464c7a6 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -328,6 +328,23 @@ def test_combination_with_extra_triple(self): self.assertCML(cube, checksum=False) +class Test_setitem(tests.IrisTest): + def setUp(self): + self.cube1 = iris.cube.Cube(1, long_name='foo') + self.cube2 = iris.cube.Cube(1, long_name='bar') + self.cubelist = iris.cube.CubeList([self.cube1] * 3) + + def test_pass(self): + self.cubelist[1] = self.cube2 + self.assertEqual(self.cubelist[1], self.cube2) + + def test_fail(self): + msg = ("Elements of cubelists must be Cube instances. Got " + ".") + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist[0] = None + + class Test_xml(tests.IrisTest): def setUp(self): self.cubes = CubeList([Cube(np.arange(3)), From 26222546d61657f21308c98141618245f14950c8 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Thu, 13 Dec 2018 11:47:31 +0000 Subject: [PATCH 04/13] test for setting more than 1 item --- lib/iris/tests/unit/cube/test_CubeList.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index e82464c7a6..62016c05e0 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -332,17 +332,24 @@ class Test_setitem(tests.IrisTest): def setUp(self): self.cube1 = iris.cube.Cube(1, long_name='foo') self.cube2 = iris.cube.Cube(1, long_name='bar') + self.cube3 = iris.cube.Cube(1, long_name='boo') self.cubelist = iris.cube.CubeList([self.cube1] * 3) def test_pass(self): self.cubelist[1] = self.cube2 self.assertEqual(self.cubelist[1], self.cube2) + self.cubelist[:2] = (self.cube2, self.cube3) + self.assertEqual( + self.cubelist, + iris.cube.Cubelist([self.cube2, self.cube3, self.cube1])) def test_fail(self): msg = ("Elements of cubelists must be Cube instances. Got " ".") with self.assertRaisesRegexp(TypeError, msg): self.cubelist[0] = None + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist[0:2] = [self.cube3, None] class Test_xml(tests.IrisTest): From 39d5f4d5249e876360eb90cf63dec1c95611e535 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Fri, 14 Dec 2018 18:42:42 +0000 Subject: [PATCH 05/13] Fix __setitem__ and Py2 tweaks --- lib/iris/cube.py | 23 ++++++++++++++------ lib/iris/tests/unit/cube/test_CubeList.py | 26 ++++++++++++++--------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 41bef7478f..234696db0e 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -253,13 +253,24 @@ def __iadd__(self, other_cubes): raise ValueError('All items in added sequence must be Cube ' 'instances.') - def __setitem__(self, key, cube): - """Set self[key] to cube""" - if isinstance(cube, Cube): - super(CubeList, self).__setitem__(key, cube) + def __setitem__(self, key, cube_or_sequence): + """Set self[key] to cube or sequence of cubes""" + if isinstance(key, int): + if not isinstance(cube_or_sequence, Cube): + raise TypeError('Elements of cubelists must be Cube instances.' + ' Got {}.'.format(type(cube_or_sequence))) else: - raise TypeError('Elements of cubelists must be Cube instances. ' - 'Got {}.'.format(type(cube))) + # key is a slice (or exception will come from list method). + if isinstance(cube_or_sequence, Cube) or not isinstance( + cube_or_sequence, collections.Iterable): + raise TypeError('Assigning to a slice of a cubelist requires ' + 'a sequence of cubes.') + elif not all([isinstance(cube, Cube) for + cube in cube_or_sequence]): + raise ValueError( + 'Elements of cubelists must be Cube instances.') + + super(CubeList, self).__setitem__(key, cube_or_sequence) def append(self, cube): """ diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index 62016c05e0..a3c05beed1 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -24,6 +24,8 @@ import iris.tests as tests import iris.tests.stock +import copy + from cf_units import Unit import numpy as np @@ -48,8 +50,7 @@ def test_pass(self): self.assertEqual(self.cubelist[-1], self.cube2) def test_fail(self): - msg = ("Only Cube instances can be appended to cubelists. Got " - "") + msg = "Only Cube instances can be appended to cubelists. Got " with self.assertRaisesRegexp(TypeError, msg): self.cubelist.append(None) @@ -91,7 +92,7 @@ def setUp(self): self.cubelist2 = iris.cube.CubeList([self.cube2]) def test_pass(self): - cubelist = self.cubelist1.copy() + cubelist = copy.copy(self.cubelist1) cubelist.extend(self.cubelist2) self.assertEqual(cubelist, self.cubelist1 + self.cubelist2) cubelist.extend([self.cube2]) @@ -171,7 +172,7 @@ def setUp(self): self.cubelist2 = iris.cube.CubeList([self.cube2]) def test_pass(self): - cubelist = self.cubelist1.copy() + cubelist = copy.copy(self.cubelist1) cubelist += self.cubelist2 self.assertEqual(cubelist, self.cubelist1 + self.cubelist2) cubelist += [self.cube2] @@ -199,8 +200,7 @@ def test_pass(self): self.assertEqual(self.cubelist[1], self.cube2) def test_fail(self): - msg = ("Only Cube instances can be inserted into cubelists. Got " - "") + msg = "Only Cube instances can be inserted into cubelists. Got " with self.assertRaisesRegexp(TypeError, msg): self.cubelist.insert(0, None) @@ -341,15 +341,21 @@ def test_pass(self): self.cubelist[:2] = (self.cube2, self.cube3) self.assertEqual( self.cubelist, - iris.cube.Cubelist([self.cube2, self.cube3, self.cube1])) + iris.cube.CubeList([self.cube2, self.cube3, self.cube1])) def test_fail(self): - msg = ("Elements of cubelists must be Cube instances. Got " - ".") + msg = "Elements of cubelists must be Cube instances. Got " with self.assertRaisesRegexp(TypeError, msg): self.cubelist[0] = None - with self.assertRaisesRegexp(TypeError, msg): + msg = "Elements of cubelists must be Cube instances." + with self.assertRaisesRegexp(ValueError, msg): self.cubelist[0:2] = [self.cube3, None] + msg = ('Assigning to a slice of a cubelist requires a sequence of ' + 'cubes.') + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist[:1] = 2.5 + with self.assertRaisesRegexp(TypeError, msg): + self.cubelist[:1] = self.cube1 class Test_xml(tests.IrisTest): From 41871622361750b57018920e2d739e164d082448 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Mon, 17 Dec 2018 11:32:32 +0000 Subject: [PATCH 06/13] implement __setslice__ for Python2.7 --- lib/iris/cube.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 234696db0e..71491ece19 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -272,6 +272,18 @@ def __setitem__(self, key, cube_or_sequence): super(CubeList, self).__setitem__(key, cube_or_sequence) + # __setslice__ is only required for python2.7 compatibility. + def __setslice__(self, *args): + cubes = args[-1] + if isinstance(cubes, Cube) or not isinstance( + cubes, collections.Iterable): + raise TypeError('Assigning to a slice of a cubelist requires ' + 'a sequence of cubes.') + elif not all([isinstance(cube, Cube) for cube in cubes]): + raise ValueError('Elements of cubelists must be Cube instances.') + + super(CubeList, self).__setslice__(*args) + def append(self, cube): """ Append a cube. From 569f35e2e97a4193ee1069072d52d33372ce0f20 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Wed, 6 Feb 2019 11:50:30 +0000 Subject: [PATCH 07/13] change exceptions to warnings --- lib/iris/cube.py | 87 ++++++++++------------- lib/iris/tests/unit/cube/test_CubeList.py | 45 ++++++------ 2 files changed, 59 insertions(+), 73 deletions(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 71491ece19..5747a3e999 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -1,4 +1,4 @@ -# (C) British Crown Copyright 2010 - 2018, Met Office +# (C) British Crown Copyright 2010 - 2019, Met Office # # This file is part of Iris. # @@ -216,7 +216,30 @@ def __repr__(self): """Runs repr on every cube.""" return '[%s]' % ',\n'.join([repr(cube) for cube in self]) + def _check_iscube(self, obj): + """ + Raise a warning if obj is not a cube. + """ + if not isinstance(obj, Cube): + msg = ('Cubelist now contains object of type {}. This may ' + 'adversely affect subsequent operations.') + warnings.warn(msg.format(type(obj))) + + def _check_cube_sequence(self, sequence): + """ + Raise one or more warnings if sequence contains elements that are not + Cubes. Skip this if the sequence is a CubeList, as we can assume it + was already checked. + """ + if (isinstance(sequence, collections.Iterable) and + not isinstance(sequence, Cube) and + not isinstance(sequence, CubeList)): + for obj in sequence: + self._check_iscube(obj) + + # TODO #370 Which operators need overloads? + def __add__(self, other): return CubeList(list.__add__(self, other)) @@ -242,57 +265,32 @@ def __iadd__(self, other_cubes): """ Add a sequence of cubes to the cubelist in place. """ - if isinstance(other_cubes, Cube) or not isinstance( - other_cubes, collections.Iterable): - raise TypeError( - 'Can only add a sequence of Cube instances.') - elif isinstance(other_cubes, CubeList) or all( - [isinstance(cube, Cube) for cube in other_cubes]): - super(CubeList, self).__iadd__(other_cubes) - else: - raise ValueError('All items in added sequence must be Cube ' - 'instances.') + self._check_cube_sequence(other_cubes) + super(CubeList, self).__iadd__(other_cubes) def __setitem__(self, key, cube_or_sequence): """Set self[key] to cube or sequence of cubes""" if isinstance(key, int): - if not isinstance(cube_or_sequence, Cube): - raise TypeError('Elements of cubelists must be Cube instances.' - ' Got {}.'.format(type(cube_or_sequence))) + # should have single cube. + self._check_iscube(cube_or_sequence) else: # key is a slice (or exception will come from list method). - if isinstance(cube_or_sequence, Cube) or not isinstance( - cube_or_sequence, collections.Iterable): - raise TypeError('Assigning to a slice of a cubelist requires ' - 'a sequence of cubes.') - elif not all([isinstance(cube, Cube) for - cube in cube_or_sequence]): - raise ValueError( - 'Elements of cubelists must be Cube instances.') + self._check_cube_sequence(cube_or_sequence) super(CubeList, self).__setitem__(key, cube_or_sequence) # __setslice__ is only required for python2.7 compatibility. def __setslice__(self, *args): cubes = args[-1] - if isinstance(cubes, Cube) or not isinstance( - cubes, collections.Iterable): - raise TypeError('Assigning to a slice of a cubelist requires ' - 'a sequence of cubes.') - elif not all([isinstance(cube, Cube) for cube in cubes]): - raise ValueError('Elements of cubelists must be Cube instances.') - + self._check_cube_sequence(cubes) super(CubeList, self).__setslice__(*args) def append(self, cube): """ Append a cube. """ - if isinstance(cube, Cube): - super(CubeList, self).append(cube) - else: - raise TypeError('Only Cube instances can be appended to cubelists.' - ' Got {}.'.format(type(cube))) + self._check_iscube(cube) + super(CubeList, self).append(cube) def extend(self, other_cubes): """ @@ -303,28 +301,15 @@ def extend(self, other_cubes): * other_cubes: A cubelist or other sequence of cubes. """ - if isinstance(other_cubes, Cube): - raise TypeError( - 'Use append to add a single cube to the cubelist.') - elif not isinstance(other_cubes, collections.Iterable): - raise TypeError( - 'Can only extend with a sequence of Cube instances.') - elif isinstance(other_cubes, CubeList) or all( - [isinstance(cube, Cube) for cube in other_cubes]): - super(CubeList, self).extend(other_cubes) - else: - raise ValueError('All items in other_cubes must be Cube ' - 'instances.') + self._check_cube_sequence(other_cubes) + super(CubeList, self).extend(other_cubes) def insert(self, index, cube): """ Insert a cube before index. """ - if isinstance(cube, Cube): - super(CubeList, self).insert(index, cube) - else: - raise TypeError('Only Cube instances can be inserted into ' - 'cubelists. Got {}.'.format(type(cube))) + self._check_iscube(cube) + super(CubeList, self).insert(index, cube) def xml(self, checksum=False, order=True, byteorder=True): """Return a string of the XML that this list of cubes represents.""" diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index a3c05beed1..c343c118be 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -1,4 +1,4 @@ -# (C) British Crown Copyright 2014 - 2018, Met Office +# (C) British Crown Copyright 2014 - 2019, Met Office # # This file is part of Iris. # @@ -36,6 +36,8 @@ from iris.fileformats.pp import STASH from iris.tests import mock +NOT_CUBE_MSG = 'Cubelist now contains object of type ' + class Test_append(tests.IrisTest): def setUp(self): @@ -49,9 +51,8 @@ def test_pass(self): self.cubelist.append(self.cube2) self.assertEqual(self.cubelist[-1], self.cube2) - def test_fail(self): - msg = "Only Cube instances can be appended to cubelists. Got " - with self.assertRaisesRegexp(TypeError, msg): + def test_warn(self): + with self.assertWarnsRegexp(NOT_CUBE_MSG): self.cubelist.append(None) @@ -99,14 +100,14 @@ def test_pass(self): self.assertEqual(cubelist[-1], self.cube2) def test_fail(self): - msg = 'Use append to add a single cube to the cubelist.' - with self.assertRaisesRegexp(TypeError, msg): + with self.assertRaisesRegexp(TypeError, 'Cube is not iterable'): self.cubelist1.extend(self.cube1) - msg = 'Can only extend with a sequence of Cube instances.' + msg = "'NoneType' object is not iterable" with self.assertRaisesRegexp(TypeError, msg): self.cubelist1.extend(None) - msg = 'All items in other_cubes must be Cube instances.' - with self.assertRaisesRegexp(ValueError, msg): + + def test_warn(self): + with self.assertWarnsRegexp(NOT_CUBE_MSG): self.cubelist1.extend(range(3)) @@ -179,13 +180,15 @@ def test_pass(self): self.assertEqual(cubelist[-1], self.cube2) def test_fail(self): - msg = 'Can only add a sequence of Cube instances.' + msg = 'Cube is not iterable' with self.assertRaisesRegexp(TypeError, msg): self.cubelist1 += self.cube1 + msg = "'float' object is not iterable" with self.assertRaisesRegexp(TypeError, msg): self.cubelist1 += 1. - msg = 'All items in added sequence must be Cube instances.' - with self.assertRaisesRegexp(ValueError, msg): + + def test_warn(self): + with self.assertWarnsRegexp(NOT_CUBE_MSG): self.cubelist1 += range(3) @@ -199,9 +202,8 @@ def test_pass(self): self.cubelist.insert(1, self.cube2) self.assertEqual(self.cubelist[1], self.cube2) - def test_fail(self): - msg = "Only Cube instances can be inserted into cubelists. Got " - with self.assertRaisesRegexp(TypeError, msg): + def test_warn(self): + with self.assertWarnsRegexp(NOT_CUBE_MSG): self.cubelist.insert(0, None) @@ -343,15 +345,14 @@ def test_pass(self): self.cubelist, iris.cube.CubeList([self.cube2, self.cube3, self.cube1])) - def test_fail(self): - msg = "Elements of cubelists must be Cube instances. Got " - with self.assertRaisesRegexp(TypeError, msg): + def test_warn(self): + with self.assertWarnsRegexp(NOT_CUBE_MSG): self.cubelist[0] = None - msg = "Elements of cubelists must be Cube instances." - with self.assertRaisesRegexp(ValueError, msg): + with self.assertWarnsRegexp(NOT_CUBE_MSG): self.cubelist[0:2] = [self.cube3, None] - msg = ('Assigning to a slice of a cubelist requires a sequence of ' - 'cubes.') + + def test_fail(self): + msg = "can only assign an iterable" with self.assertRaisesRegexp(TypeError, msg): self.cubelist[:1] = 2.5 with self.assertRaisesRegexp(TypeError, msg): From 5b13dcc8b997c9bfc4a80c91e7fb3760d7d87374 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Wed, 6 Feb 2019 11:54:10 +0000 Subject: [PATCH 08/13] stickler --- lib/iris/cube.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 5747a3e999..57275816d8 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -232,12 +232,11 @@ def _check_cube_sequence(self, sequence): was already checked. """ if (isinstance(sequence, collections.Iterable) and - not isinstance(sequence, Cube) and - not isinstance(sequence, CubeList)): + not isinstance(sequence, Cube) and + not isinstance(sequence, CubeList)): for obj in sequence: self._check_iscube(obj) - # TODO #370 Which operators need overloads? def __add__(self, other): From 8f272fbbcb62d02ea73e570988fffa26ce623840 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Fri, 8 Feb 2019 09:28:33 +0000 Subject: [PATCH 09/13] duck type check; move helpers outside class --- lib/iris/cube.py | 56 +++++++++++------------ lib/iris/tests/unit/cube/test_CubeList.py | 14 +++--- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 57275816d8..8032bf4c12 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -184,6 +184,26 @@ def merged(self, unique=False): return _CubeFilterCollection([pair.merged(unique) for pair in self.pairs]) +def _check_iscube(obj): + """ + Raise a warning if obj does not look like a cube. + """ + if not hasattr(obj, 'add_aux_coord'): + msg = ("Cubelist now contains object of type '{}'. This may " + "adversely affect subsequent operations.") + warnings.warn(msg.format(type(obj).__name__)) + +def _check_cube_sequence(sequence): + """ + Raise one or more warnings if sequence contains elements that are not + Cubes (or cube-like). Skip this if the sequence is a CubeList, as we can + assume it was already checked. + """ + if (isinstance(sequence, collections.Iterable) and + not isinstance(sequence, Cube) and + not isinstance(sequence, CubeList)): + for obj in sequence: + _check_iscube(obj) class CubeList(list): """ @@ -215,28 +235,6 @@ def __str__(self): def __repr__(self): """Runs repr on every cube.""" return '[%s]' % ',\n'.join([repr(cube) for cube in self]) - - def _check_iscube(self, obj): - """ - Raise a warning if obj is not a cube. - """ - if not isinstance(obj, Cube): - msg = ('Cubelist now contains object of type {}. This may ' - 'adversely affect subsequent operations.') - warnings.warn(msg.format(type(obj))) - - def _check_cube_sequence(self, sequence): - """ - Raise one or more warnings if sequence contains elements that are not - Cubes. Skip this if the sequence is a CubeList, as we can assume it - was already checked. - """ - if (isinstance(sequence, collections.Iterable) and - not isinstance(sequence, Cube) and - not isinstance(sequence, CubeList)): - for obj in sequence: - self._check_iscube(obj) - # TODO #370 Which operators need overloads? def __add__(self, other): @@ -264,31 +262,31 @@ def __iadd__(self, other_cubes): """ Add a sequence of cubes to the cubelist in place. """ - self._check_cube_sequence(other_cubes) + _check_cube_sequence(other_cubes) super(CubeList, self).__iadd__(other_cubes) def __setitem__(self, key, cube_or_sequence): """Set self[key] to cube or sequence of cubes""" if isinstance(key, int): # should have single cube. - self._check_iscube(cube_or_sequence) + _check_iscube(cube_or_sequence) else: # key is a slice (or exception will come from list method). - self._check_cube_sequence(cube_or_sequence) + _check_cube_sequence(cube_or_sequence) super(CubeList, self).__setitem__(key, cube_or_sequence) # __setslice__ is only required for python2.7 compatibility. def __setslice__(self, *args): cubes = args[-1] - self._check_cube_sequence(cubes) + _check_cube_sequence(cubes) super(CubeList, self).__setslice__(*args) def append(self, cube): """ Append a cube. """ - self._check_iscube(cube) + _check_iscube(cube) super(CubeList, self).append(cube) def extend(self, other_cubes): @@ -300,14 +298,14 @@ def extend(self, other_cubes): * other_cubes: A cubelist or other sequence of cubes. """ - self._check_cube_sequence(other_cubes) + _check_cube_sequence(other_cubes) super(CubeList, self).extend(other_cubes) def insert(self, index, cube): """ Insert a cube before index. """ - self._check_iscube(cube) + _check_iscube(cube) super(CubeList, self).insert(index, cube) def xml(self, checksum=False, order=True, byteorder=True): diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index c343c118be..5a9a3fe8f3 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -36,7 +36,7 @@ from iris.fileformats.pp import STASH from iris.tests import mock -NOT_CUBE_MSG = 'Cubelist now contains object of type ' +NOT_CUBE_MSG = "Cubelist now contains object of type '{}'" class Test_append(tests.IrisTest): @@ -52,7 +52,7 @@ def test_pass(self): self.assertEqual(self.cubelist[-1], self.cube2) def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG): + with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): self.cubelist.append(None) @@ -107,7 +107,7 @@ def test_fail(self): self.cubelist1.extend(None) def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG): + with self.assertWarnsRegexp(NOT_CUBE_MSG.format('int')): self.cubelist1.extend(range(3)) @@ -188,7 +188,7 @@ def test_fail(self): self.cubelist1 += 1. def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG): + with self.assertWarnsRegexp(NOT_CUBE_MSG.format('int')): self.cubelist1 += range(3) @@ -203,7 +203,7 @@ def test_pass(self): self.assertEqual(self.cubelist[1], self.cube2) def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG): + with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): self.cubelist.insert(0, None) @@ -346,9 +346,9 @@ def test_pass(self): iris.cube.CubeList([self.cube2, self.cube3, self.cube1])) def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG): + with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): self.cubelist[0] = None - with self.assertWarnsRegexp(NOT_CUBE_MSG): + with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): self.cubelist[0:2] = [self.cube3, None] def test_fail(self): From f7134c120e48aae750e161ccda2c7180332656da Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Fri, 8 Feb 2019 09:30:56 +0000 Subject: [PATCH 10/13] blank lines --- lib/iris/cube.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 8032bf4c12..93b43c70a4 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -184,6 +184,7 @@ def merged(self, unique=False): return _CubeFilterCollection([pair.merged(unique) for pair in self.pairs]) + def _check_iscube(obj): """ Raise a warning if obj does not look like a cube. @@ -193,6 +194,7 @@ def _check_iscube(obj): "adversely affect subsequent operations.") warnings.warn(msg.format(type(obj).__name__)) + def _check_cube_sequence(sequence): """ Raise one or more warnings if sequence contains elements that are not @@ -205,6 +207,7 @@ def _check_cube_sequence(sequence): for obj in sequence: _check_iscube(obj) + class CubeList(list): """ All the functionality of a standard :class:`list` with added "Cube" From bc7eb7a995961cf5740b7ec3de230dc86ca3cae4 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Fri, 8 Feb 2019 10:21:01 +0000 Subject: [PATCH 11/13] proposed: revert warnings to exceptions --- lib/iris/cube.py | 19 ++++++++------- lib/iris/tests/unit/cube/test_CubeList.py | 28 +++++++++++------------ 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 93b43c70a4..705d897bb5 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -187,25 +187,28 @@ def merged(self, unique=False): def _check_iscube(obj): """ - Raise a warning if obj does not look like a cube. + Raise an exception if obj does not look like a cube. """ if not hasattr(obj, 'add_aux_coord'): - msg = ("Cubelist now contains object of type '{}'. This may " - "adversely affect subsequent operations.") - warnings.warn(msg.format(type(obj).__name__)) + msg = ("Object of type '{}' does not look like a cube so can't be " + "included in cubelist.") + raise TypeError(msg.format(type(obj).__name__)) def _check_cube_sequence(sequence): """ - Raise one or more warnings if sequence contains elements that are not - Cubes (or cube-like). Skip this if the sequence is a CubeList, as we can - assume it was already checked. + Raise an exception if sequence contains an element that is not a Cube (or + cube-like). Skip this if the sequence is a CubeList, as we can assume it + was already checked. """ if (isinstance(sequence, collections.Iterable) and not isinstance(sequence, Cube) and not isinstance(sequence, CubeList)): for obj in sequence: - _check_iscube(obj) + if not hasattr(obj, 'add_aux_coord'): + msg = ("Sequence contains object of type '{}', which does not " + "look like a cube so can't be included in cubelist.") + raise ValueError(msg.format(type(obj).__name__)) class CubeList(list): diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index 5a9a3fe8f3..1699e082c5 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -36,7 +36,7 @@ from iris.fileformats.pp import STASH from iris.tests import mock -NOT_CUBE_MSG = "Cubelist now contains object of type '{}'" +NOT_CUBE_MSG = "bject of type '{}'" class Test_append(tests.IrisTest): @@ -51,8 +51,9 @@ def test_pass(self): self.cubelist.append(self.cube2) self.assertEqual(self.cubelist[-1], self.cube2) - def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): + def test_fail(self): + with self.assertRaisesRegexp(TypeError, + NOT_CUBE_MSG.format('NoneType')): self.cubelist.append(None) @@ -105,9 +106,7 @@ def test_fail(self): msg = "'NoneType' object is not iterable" with self.assertRaisesRegexp(TypeError, msg): self.cubelist1.extend(None) - - def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG.format('int')): + with self.assertRaisesRegexp(ValueError, NOT_CUBE_MSG.format('int')): self.cubelist1.extend(range(3)) @@ -186,9 +185,7 @@ def test_fail(self): msg = "'float' object is not iterable" with self.assertRaisesRegexp(TypeError, msg): self.cubelist1 += 1. - - def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG.format('int')): + with self.assertRaisesRegexp(ValueError, NOT_CUBE_MSG.format('int')): self.cubelist1 += range(3) @@ -202,8 +199,9 @@ def test_pass(self): self.cubelist.insert(1, self.cube2) self.assertEqual(self.cubelist[1], self.cube2) - def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): + def test_fail(self): + with self.assertRaisesRegexp(TypeError, + NOT_CUBE_MSG.format('NoneType')): self.cubelist.insert(0, None) @@ -345,10 +343,12 @@ def test_pass(self): self.cubelist, iris.cube.CubeList([self.cube2, self.cube3, self.cube1])) - def test_warn(self): - with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): + def test_fail(self): + with self.assertRaisesRegexp(TypeError, + NOT_CUBE_MSG.format('NoneType')): self.cubelist[0] = None - with self.assertWarnsRegexp(NOT_CUBE_MSG.format('NoneType')): + with self.assertRaisesRegexp(ValueError, + NOT_CUBE_MSG.format('NoneType')): self.cubelist[0:2] = [self.cube3, None] def test_fail(self): From d961a68aa775e2bd42ced27c2e74dc5459c72904 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Fri, 8 Feb 2019 10:23:47 +0000 Subject: [PATCH 12/13] remove stray extra 'test_fail' --- lib/iris/tests/unit/cube/test_CubeList.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index 1699e082c5..ad986a4002 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -351,7 +351,6 @@ def test_fail(self): NOT_CUBE_MSG.format('NoneType')): self.cubelist[0:2] = [self.cube3, None] - def test_fail(self): msg = "can only assign an iterable" with self.assertRaisesRegexp(TypeError, msg): self.cubelist[:1] = 2.5 From 7fe5d0ae59e12850b996b78deba20839b7d5a756 Mon Sep 17 00:00:00 2001 From: Ruth Comer Date: Wed, 20 Feb 2019 18:03:44 +0000 Subject: [PATCH 13/13] pass sequences through __init__; _assert_is_cube --- lib/iris/cube.py | 53 +++++++---------------- lib/iris/tests/unit/cube/test_CubeList.py | 8 ++-- 2 files changed, 20 insertions(+), 41 deletions(-) diff --git a/lib/iris/cube.py b/lib/iris/cube.py index 705d897bb5..2f38623c37 100644 --- a/lib/iris/cube.py +++ b/lib/iris/cube.py @@ -185,32 +185,6 @@ def merged(self, unique=False): self.pairs]) -def _check_iscube(obj): - """ - Raise an exception if obj does not look like a cube. - """ - if not hasattr(obj, 'add_aux_coord'): - msg = ("Object of type '{}' does not look like a cube so can't be " - "included in cubelist.") - raise TypeError(msg.format(type(obj).__name__)) - - -def _check_cube_sequence(sequence): - """ - Raise an exception if sequence contains an element that is not a Cube (or - cube-like). Skip this if the sequence is a CubeList, as we can assume it - was already checked. - """ - if (isinstance(sequence, collections.Iterable) and - not isinstance(sequence, Cube) and - not isinstance(sequence, CubeList)): - for obj in sequence: - if not hasattr(obj, 'add_aux_coord'): - msg = ("Sequence contains object of type '{}', which does not " - "look like a cube so can't be included in cubelist.") - raise ValueError(msg.format(type(obj).__name__)) - - class CubeList(list): """ All the functionality of a standard :class:`list` with added "Cube" @@ -241,6 +215,13 @@ def __str__(self): def __repr__(self): """Runs repr on every cube.""" return '[%s]' % ',\n'.join([repr(cube) for cube in self]) + + @staticmethod + def _assert_is_cube(obj): + if not isinstance(obj, Cube): + msg = ("Object of type '{}' does not belong in a cubelist.") + raise ValueError(msg.format(type(obj).__name__)) + # TODO #370 Which operators need overloads? def __add__(self, other): @@ -268,31 +249,30 @@ def __iadd__(self, other_cubes): """ Add a sequence of cubes to the cubelist in place. """ - _check_cube_sequence(other_cubes) - super(CubeList, self).__iadd__(other_cubes) + super(CubeList, self).__iadd__(CubeList(other_cubes)) def __setitem__(self, key, cube_or_sequence): """Set self[key] to cube or sequence of cubes""" if isinstance(key, int): # should have single cube. - _check_iscube(cube_or_sequence) + self._assert_is_cube(cube_or_sequence) else: # key is a slice (or exception will come from list method). - _check_cube_sequence(cube_or_sequence) + cube_or_sequence = CubeList(cube_or_sequence) super(CubeList, self).__setitem__(key, cube_or_sequence) # __setslice__ is only required for python2.7 compatibility. def __setslice__(self, *args): - cubes = args[-1] - _check_cube_sequence(cubes) - super(CubeList, self).__setslice__(*args) + args_list = list(args) + args_list[-1] = CubeList(args[-1]) + super(CubeList, self).__setslice__(*args_list) def append(self, cube): """ Append a cube. """ - _check_iscube(cube) + self._assert_is_cube(cube) super(CubeList, self).append(cube) def extend(self, other_cubes): @@ -304,14 +284,13 @@ def extend(self, other_cubes): * other_cubes: A cubelist or other sequence of cubes. """ - _check_cube_sequence(other_cubes) - super(CubeList, self).extend(other_cubes) + super(CubeList, self).extend(CubeList(other_cubes)) def insert(self, index, cube): """ Insert a cube before index. """ - _check_iscube(cube) + self._assert_is_cube(cube) super(CubeList, self).insert(index, cube) def xml(self, checksum=False, order=True, byteorder=True): diff --git a/lib/iris/tests/unit/cube/test_CubeList.py b/lib/iris/tests/unit/cube/test_CubeList.py index ad986a4002..e0a800f99a 100644 --- a/lib/iris/tests/unit/cube/test_CubeList.py +++ b/lib/iris/tests/unit/cube/test_CubeList.py @@ -36,7 +36,7 @@ from iris.fileformats.pp import STASH from iris.tests import mock -NOT_CUBE_MSG = "bject of type '{}'" +NOT_CUBE_MSG = "Object of type '{}' does not belong in a cubelist." class Test_append(tests.IrisTest): @@ -52,7 +52,7 @@ def test_pass(self): self.assertEqual(self.cubelist[-1], self.cube2) def test_fail(self): - with self.assertRaisesRegexp(TypeError, + with self.assertRaisesRegexp(ValueError, NOT_CUBE_MSG.format('NoneType')): self.cubelist.append(None) @@ -200,7 +200,7 @@ def test_pass(self): self.assertEqual(self.cubelist[1], self.cube2) def test_fail(self): - with self.assertRaisesRegexp(TypeError, + with self.assertRaisesRegexp(ValueError, NOT_CUBE_MSG.format('NoneType')): self.cubelist.insert(0, None) @@ -344,7 +344,7 @@ def test_pass(self): iris.cube.CubeList([self.cube2, self.cube3, self.cube1])) def test_fail(self): - with self.assertRaisesRegexp(TypeError, + with self.assertRaisesRegexp(ValueError, NOT_CUBE_MSG.format('NoneType')): self.cubelist[0] = None with self.assertRaisesRegexp(ValueError,