iris.util.reverse on cubes

[rcomer]

The iris.util.reverse documentation states that it can be used for reversing an array, but it works equally well for reversing a cube. Since I use it this way in my code, I'd like to make this usage official by:

• Updating the docstring to indicate the function works on a cube.
• Adding a test for reversing a cube.

I also moved the existing array tests into tests/unit and swapped out np.testing.assert_array_equal for self.assertArrayEqual.

Note that for numpy arrays there is also numpy.flip

[stickler-ci]

F401 'iris.tests.stock' imported but unused

[DPeterK]

Thanks for this clarification @rcomer. There are a few test changes that would be beneficial, and a bit of a bigger think required about the API for reversed given the clarification that you've made.

[DPeterK]

It's great that reverse can be used on cubes or NumPy arrays. This does, however, introduce a usage issue in that typically if I pass a cube to functionality that operates on a dimension I can specify the dimension of interest as a coordinate name as well as an axis.

It would be good if you could pass one or more coordinates or axes here, and have the function look up the axis from the specified coordinate (I'm pretty sure there are examples of doing such elsewhere in the codebase). Of course, with this you'll need to add more checking, for example if you pass a NumPy array and named dimensions... You'll also need to add testing for this functionality.

[rcomer]

Thanks for your comments @dkillick, particularly the pointers about testing conventions, as I'm pretty new to that. I'll make all those changes when I get a minute.

[stickler-ci]

E501 line too long (80 > 79 characters)

[rcomer]

If I put "these." on the next line to make this compliant, it looks wrong in the docs. There must be a simple solution here...?

[QuLogic]

Remove the .; you're only one character over, and no other types end in periods.

[rcomer]

Thanks @QuLogic !

[rcomer]

Hi @dkillick, I think I've covered everything.

I split the changes between two commits, so it's easy to find the new tests for the API change separately from the general test reorganisation.

[DPeterK]

@rcomer these changes are excellent and add all the extra functionality and testing that I was hoping for.

I'm afraid that I have just one more request, which relates to the recent additions that you've made to the testing. Could you please convert instances of self.assertRaises that you've added to self.assertRaisesRegexp ? You can also convert the examples of self.assertRaises that you inherited, but I'm less fussed about these as you've inherited them from a time when we didn't use self.assertRaisesRegexp as standard.

The benefit of the new approach is that we get to check both the error raised and the error message, which avoids potential situations where the right type of error is raised but coincidentally in a different part of the codebase.

As an example,

self.assertRaises(TypeError, reverse, a, 'latitude')

would become

with self.assertRaisesRegexp(TypeError, "To reverse an array, provide an int *"):
    reverse(a, 'latitude')

For an example of this in use, see https://github.com/SciTools/iris/blob/master/lib/iris/tests/unit/util/test__slice_data_with_keys.py#L282-L285 (and many others in the codebase). With this done I'm happy to merge 👍

[rcomer]

Well this is awkward.

isinstance(cube, collections.Iterable) evaluates to True. So iris.cube._is_single_item(cube) evaluates to False. So if I pass a cube for my coords_or_dims, as in one of my tests, reverse attempts to loop through the cube and I get a TypeError but not the one I was expecting: "Cube is not iterable".

If cubes are not iterable, how can they be instances of collections.Iterable? Are there other types like this?

And how should I fix this? I could pass some other wrong type for that test, but that seems like cheating!

All the other error tests are fine. 👍

[rcomer]

Explicitly check for a cube here, on the assumption that the issues in my main comment are specific to cubes.

[rcomer]

This is added because the tests that pass an empty list were failing at np.min (line 478 below) and that error message was less informative in context.

[DPeterK]

Well this is awkward.

Um, yes 😳 I think what's happening is collections.Iterable determines if an object is iterable by looking for an __iter__ method on the base class. Iris cubes have an __iter__ method but it just raises an error. These two things combined make cubes look a bit silly, so I think I might raise an issue regarding this...

how should I fix this?

I think my preferred fix for this situation would be a separate check specifically for coords_or_dims being an Iris cube, which raises its own special error for that case (and, of course, is tested). I had a quick think about whether we might want to be able to define a reverse operation based on a cube, but I think that's an operation that makes no sense so shouldn't be allowed.

[rcomer]

I think my preferred fix for this situation would be a separate check specifically for coords_or_dims being an Iris cube, which raises its own special error for that case (and, of course, is tested).

Done, I think.

[DPeterK]

@rcomer great stuff!