Skip to content

Ensure cubelists only contain cubes #3238

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.

Sign up for GitHub

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

Closed
rcomer wants to merge 14 commits into from
Closed

Ensure cubelists only contain cubes #3238

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4ab7cfa
ensure cubelists only contain cubes
rcomer Dec 7, 2018
0b293ec
address __iadd__ and __setitem__
rcomer Dec 12, 2018
8a74df9
__setitem__ tests
rcomer Dec 12, 2018
2622254
test for setting more than 1 item
rcomer Dec 13, 2018
39d5f4d
Fix __setitem__ and Py2 tweaks
rcomer Dec 14, 2018
4187162
implement __setslice__ for Python2.7
rcomer Dec 17, 2018
569f35e
change exceptions to warnings
rcomer Feb 6, 2019
5b13dcc
stickler
rcomer Feb 6, 2019
8f272fb
duck type check; move helpers outside class
rcomer Feb 8, 2019
f7134c1
blank lines
rcomer Feb 8, 2019
bc7eb7a
proposed: revert warnings to exceptions
rcomer Feb 8, 2019
d961a68
remove stray extra 'test_fail'
rcomer Feb 8, 2019
a84823e
Merge branch 'master' of github.com:SciTools/iris into cubelist-conta…
rcomer Feb 20, 2019
7fe5d0a
pass sequences through __init__; _assert_is_cube
rcomer Feb 20, 2019
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
87 changes: 36 additions & 51 deletions lib/iris/cube.py
View file
Open in desktop
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 @@ -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):
Copy link
Member Author

@rcomer rcomer Feb 6, 2019 •
edited
Loading

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 •
edited
Loading

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

@rcomer rcomer Feb 7, 2019

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 •
edited
Loading

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 •
edited
Loading

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

@rcomer rcomer Feb 14, 2019

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

@rcomer rcomer Feb 14, 2019

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

@pelson pelson Feb 15, 2019

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

@rcomer rcomer Feb 15, 2019

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 •
edited
Loading

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 @@ -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):
"""
Expand All @@ -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."""
Expand Down
45 changes: 23 additions & 22 deletions lib/iris/tests/unit/cube/test_CubeList.py
View file
Open in desktop
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 Down Expand Up @@ -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):
Expand All @@ -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)


Expand Down Expand Up @@ -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))


Expand Down Expand Up @@ -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)


Expand All @@ -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)


Expand Down Expand Up @@ -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):
Expand Down