Skip to content
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

Improved Aux factory error handling: better message, delivered earlier #3182

Merged
merged 6 commits into from
Oct 3, 2018
Merged

Improved Aux factory error handling: better message, delivered earlier #3182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
959419d
Tests for expected behaviour
hdyson Oct 2, 2018
da8dffa
Raise error if aux factory relies on a coordinate external to cube
hdyson Oct 2, 2018
92617c8
Include coordinate name in error message
hdyson Oct 2, 2018
eb47724
Don't check whether dependencies that are "None" are in the coords.
hdyson Oct 2, 2018
054059d
Review changes: check cube name in error and variable rename
hdyson Oct 3, 2018
a55a30d
Truncated variable for flake8
hdyson Oct 3, 2018
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
6 changes: 6 additions & 0 deletions lib/iris/cube.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,6 +994,12 @@ def add_aux_factory(self, aux_factory):
if not isinstance(aux_factory, iris.aux_factory.AuxCoordFactory):
raise TypeError('Factory must be a subclass of '
'iris.aux_factory.AuxCoordFactory.')
cube_coords = self.coords()
for dependency in aux_factory.dependencies:
ref_coord = aux_factory.dependencies[dependency]
if ref_coord is not None and ref_coord not in cube_coords:
msg = "{} coordinate for factory is not present on cube {}"
raise ValueError(msg.format(ref_coord.name(), self.name()))
self._aux_factories.append(aux_factory)

def add_cell_measure(self, cell_measure, data_dims=None):
Expand Down
27 changes: 26 additions & 1 deletion lib/iris/tests/unit/cube/test_Cube.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@
import iris.aux_factory
import iris.coords
import iris.exceptions
from iris import FUTURE
from iris.analysis import WeightedAggregator, Aggregator
from iris.analysis import MEAN
from iris.aux_factory import HybridHeightFactory
from iris.cube import Cube
from iris.coords import AuxCoord, DimCoord, CellMeasure
from iris.exceptions import (CoordinateNotFoundError, CellMeasureNotFoundError,
Expand Down Expand Up @@ -1546,6 +1546,31 @@ def test_add_cell_measure(self):
cube.add_cell_measure(a_cell_measure, [0, 1])
self.assertEqual(cube.cell_measure('area'), a_cell_measure)

def test_add_valid_aux_factory(self):
cube = Cube(np.arange(8).reshape(2, 2, 2))
delta = AuxCoord(points=[0, 1], long_name='delta', units='m')
sigma = AuxCoord(points=[0, 1], long_name='sigma')
orog = AuxCoord(np.arange(4).reshape(2, 2), units='m')
cube.add_aux_coord(delta, 0)
cube.add_aux_coord(sigma, 0)
cube.add_aux_coord(orog, (1, 2))
factory = HybridHeightFactory(delta=delta, sigma=sigma, orography=orog)
self.assertIsNone(cube.add_aux_factory(factory))
Copy link
Member

Choose a reason for hiding this comment

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

Sorry @hdyson, I'm a bit confuse about what this test is for. Can you maybe add a comment or two to explain please? I'm probably being slow but I don't really understand what we are asserting here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see an existing test for adding a valid aux factory. Since I wanted to test for a particular error with an invalid aux factory, it felt like a good idea to include that test to confirm I wasn't breaking existing behaviour. There are tests for adding dim coords, aux coords and cell measures, I think the lack of an existing test for an aux factory may be an oversight?

Copy link
Member

Choose a reason for hiding this comment

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

You're quite right. I often despair at the testing (or lack of), but let's not get into that. Adding tests that should already be there is great.

I'm still not quite sure what the assertIsNone asserts there is none of though. That last line is slightly information dense for my delicate and easily-confused taste.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay, now I understand. I'm using assertIsNone as an analogue for assertDoesNotRaise (which doesn't exist). Would a comment help, or just removing the assertion? Personally, it always makes me nervous when I see a test with no assert in it, it always makes me wonder if it's actually been finished or not.

Copy link
Member

Choose a reason for hiding this comment

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

I think just a comment explaining the assert (as you just have to me, thanks for that) is enough.


def test_error_for_add_invalid_aux_factory(self):
cube = Cube(np.arange(8).reshape(2, 2, 2), long_name='bar')
delta = AuxCoord(points=[0, 1], long_name='delta', units='m')
sigma = AuxCoord(points=[0, 1], long_name='sigma')
orog = AuxCoord(np.arange(4).reshape(2, 2), units='m', long_name='foo')
cube.add_aux_coord(delta, 0)
cube.add_aux_coord(sigma, 0)
# Note orography is not added to the cube here
factory = HybridHeightFactory(delta=delta, sigma=sigma, orography=orog)
expected_error = ("foo coordinate for factory is not present on cube "
"bar")
with self.assertRaisesRegexp(ValueError, expected_error):
cube.add_aux_factory(factory)


class Test_remove_metadata(tests.IrisTest):
def setUp(self):
Expand Down