Skip to content
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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.
80 changes: 75 additions & 5 deletions lib/iris/cube.py
Original file line number Diff line number Diff line change
@@ -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.
#
Expand Down Expand Up @@ -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.')
Expand All @@ -219,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):
Copy link
Member Author

@rcomer rcomer Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style question: Is this the right place for these checking functions, or should they be defined outside the class?

Copy link
Member

@pp-mo pp-mo Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take : these don't use instance properties, so they could be static methods.
... They don't use any class properties either, so they don't really need to be in the class at all.

In fact, they don't use private properties of Cube, so they don't really need to be in the module.
At that point (they are just functions), they could go somewhere else.

However, for personal preference, I'd remove them from the class but keep them as private methods in cube.py, just in case they might need to use 'private' cube concepts in future.

Also ... the use of isinstance prevents any duck typing (lookalike objects can't masquerade as Cubes), which is arguably un-Pythonic.
We have previously used hasattr('add_aux_coord') for this elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so if we check whether the object quacks with an auxcoord instead of checking the type, is there any reason not to revert to raising an exception rather than a warning?

Copy link
Member

@pp-mo pp-mo Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dear, I had somehow skimmed over that latest discussion without taking it in.
Now I see that @pelson and I are just advocating different approaches, and I honestly don't know how to choose between.

Personally though, I must say I do hate all the warnings in Iris. There are still far too many, most occurrences are a pointless nuisance, and on the rare occasions when they aren't no-one is listening any more.

Copy link
Member Author

@rcomer rcomer Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem: I'm partly using this as a learning exercise, so exploring different solutions to the same issue is fine 👍

I think I prefer to have an exception on the grounds that, if my code is going to fail, it's better for it to fail sooner rather than later. Also, having the failure at the point that the object is included into the cubelist means that the traceback is going to point me a lot closer to where I made the mistake. Which so far has always been

cubelist.append(some_function_i_forgot_to_put_a_return_statement_in())

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should get our ducks in a row 🦆 🦆 🦆 before sending @rcomer on a wild goose chase

nicely done! 😆

As a user, I'd still rather have an exception if possible, for the reasons I gave above.

If something does go wrong with my cubelist, the first thing I'm going to do in an attempt to debug is print it. So if we're looking for a minimal set of cube-like attributes, summary ought to be up there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we thinking about this the wrong way round? Rather than trying to define which types should be allowed in a cubelist, it might be easier to focus on which types should definitely be rejected.

This started because a rogue None in a cubelist caused problems, and I wanted a more informative error message. So far I’m not aware of any other types that have caused issues. So my case would be solved by simply throwing an exception if object is None. We could generalise that a bit if we decide that, at minimum, the cubelist should be printable, so reject any objects that don’t have have a summary attribute.

The exception message needn’t say anything about how similar to a Cube the object is, but could just say ”object of type [whichever] does not belong in a cubelist”.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't have to be cube-like in all respects.

Are we thinking about this the wrong way round?

I don't think so in this case. Because users can create and use their own CubeList instances, the minimum set of behaviour required for an entry into a CubeList is precisely the Cube's behaviour (and no less).

in fact I was strongly opposed to creating another warning here

Useful to know, thank you. So my biggest concern is that we are essentially introducing a breaking change if we do this as an exception - if @rcomer has been adding None into a cube list by accident, just think of all the wild things that some of our less educated users have been doing! 😭
I guess there is a workaround though... if users really want to do this they can still do list.append(cube_list_instance, thing_that_isnt_a_cube) until they sort their 💩 out.

In an attempt to get consensus and prevent this conversation from being open-ended, my refined suggestion:

  • CubeList._assert_is_cube - raise a ValueError if not isinstance of cube.
  • CubeList._assert_is_iterable_of_cubes -> just construct a CubeList of the subset - that way you can honour iterators, and then add the constructed CubeList as necessary.
  • Update the existing call in CubeList.__new__ to use _assert_is_cube.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my biggest concern is that we are essentially introducing a breaking change if we do this as an exception... just think of all the wild things that some of our less educated users have been doing!

I hadn't considered the possibility of cubelists being used to store random types 😮 . Semi-serious question: how far away is Iris 3?

refined suggestion

Just to check I've understood: we make it strict so only Cube instances are allowed. Because the check is restricted to one method, someone who wants to include ducks in the cubelist just needs to replace that one method?

Points 1 and 2 sound good to me.

Point 3: I think I need to wait for #3264 to be merged, and then update __init__!

Copy link
Member

@pp-mo pp-mo Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pelson my biggest concern is that we are essentially introducing a breaking change if we do this as an exception - if @rcomer has been adding None into a cube list by accident, just think of all the wild things that some of our less educated users have been doing! sob

Not sure if it helps, but..
In my mind, even if it was previously possible to put non-cubes into a CubeList, that was never intended behaviour -- evidence the code covered by #3264.
So that is a bug, and fixing a bug is not a "breaking" change.
Weaselly, but we've accepted that principle before.

"""
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))

Expand All @@ -241,6 +261,56 @@ 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.
"""
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):
# should have single cube.
self._check_iscube(cube_or_sequence)
else:
# key is a slice (or exception will come from list method).
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]
self._check_cube_sequence(cubes)
super(CubeList, self).__setslice__(*args)

def append(self, cube):
"""
Append a cube.
"""
self._check_iscube(cube)
super(CubeList, self).append(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.
"""
self._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)
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."""
doc = Document()
Expand Down
120 changes: 119 additions & 1 deletion lib/iris/tests/unit/cube/test_CubeList.py
Original file line number Diff line number Diff line change
@@ -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.
#
Expand All @@ -24,6 +24,8 @@
import iris.tests as tests
import iris.tests.stock

import copy

from cf_units import Unit
import numpy as np

Expand All @@ -34,6 +36,25 @@
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):
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_warn(self):
with self.assertWarnsRegexp(NOT_CUBE_MSG):
self.cubelist.append(None)


class Test_concatenate_cube(tests.IrisTest):
def setUp(self):
Expand Down Expand Up @@ -64,6 +85,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 = copy.copy(self.cubelist1)
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):
with self.assertRaisesRegexp(TypeError, 'Cube is not iterable'):
self.cubelist1.extend(self.cube1)
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):
self.cubelist1.extend(range(3))


class Test_extract_overlapping(tests.IrisTest):
def setUp(self):
shape = (6, 14, 19)
Expand Down Expand Up @@ -118,6 +165,48 @@ 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 = copy.copy(self.cubelist1)
cubelist += self.cubelist2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed copy method to copy.copy function for Python2 compatibility, but now cubelist becomes None after the add. 🤷‍♀️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis output showing the above:
Python2.7

FAIL: test_pass (iris.tests.unit.cube.test_CubeList.Test_iadd)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/scitools_iris-2.3.0.dev0-py2.7.egg/iris/tests/unit/cube/test_CubeList.py", line 177, in test_pass
    self.assertEqual(cubelist, self.cubelist1 + self.cubelist2)
AssertionError: None != [<iris 'Cube' of foo / (unknown) (scalar cube)>,
<iris 'Cube' of bar / (unknown) (scalar cube)>]

Python3.6

FAIL: test_pass (iris.tests.unit.cube.test_CubeList.Test_iadd)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/test-environment/lib/python3.6/site-packages/scitools_iris-2.3.0.dev0-py3.6.egg/iris/tests/unit/cube/test_CubeList.py", line 177, in test_pass
    self.assertEqual(cubelist, self.cubelist1 + self.cubelist2)
AssertionError: None != [<iris 'Cube' of foo / (unknown) (scalar [50 chars]be)>]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes if I point it at the master branch. The equivalent test for extend passes on this branch. So this appearance of None is specific to:

  • Use of copy.copy function (or copying via cubelist = self.cubelist1[:]) rather than copy method.
  • Using __iadd__ with the changes I've made.

I'm officially confused.

self.assertEqual(cubelist, self.cubelist1 + self.cubelist2)
cubelist += [self.cube2]
self.assertEqual(cubelist[-1], self.cube2)

def test_fail(self):
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.

def test_warn(self):
with self.assertWarnsRegexp(NOT_CUBE_MSG):
self.cubelist1 += range(3)


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_warn(self):
with self.assertWarnsRegexp(NOT_CUBE_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")
Expand Down Expand Up @@ -241,6 +330,35 @@ 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.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_warn(self):
with self.assertWarnsRegexp(NOT_CUBE_MSG):
self.cubelist[0] = None
with self.assertWarnsRegexp(NOT_CUBE_MSG):
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
with self.assertRaisesRegexp(TypeError, msg):
self.cubelist[:1] = self.cube1


class Test_xml(tests.IrisTest):
def setUp(self):
self.cubes = CubeList([Cube(np.arange(3)),
Expand Down