Lazy bounds test #2590
Lazy bounds test #2590
Conversation
|
@dkillick @bjlittle Here is the test that I have popped in, I would appreciate opinions from both of you (if possible). @dkillick has suggested the adapt or duplicate the test 'Test__nd_points' in iris/tests/unit/aux_factory/test_AuxCoordFactory.py. I have my reservations about duplicating tests, but I could maybe adapt this class to test both points and bounds arrays. What do you think? |
|
@dkillick @bjlittle Bear in mind that adapting this test is not turning out as easy or straightforward as it first appeared... |
|
@corinnebosley I like the test you've added here - it directly tests the problem behaviour that we're observing. My suggestion for extra tests is that we add a new test class to test the behaviour of I definitely think we shouldn't update |
|
@dkillick Also, and this is one for you specifically @dkillick, you mentioned the other day that there were two extra things that I had to look for in a PR; one was a test and we never actually got round to the other. What was it? A What's New entry? Or something else? |
|
@corinnebosley ummm... I'll let you know if I remember what it was I was thinking of! |
lib/iris/tests/test_coord_api.py
Outdated
| ", standard_name='air_temperature', units=Unit('kelvin'))" | ||
| ) | ||
| self.assertEqual(result, str(a)) | ||
|
|
There was a problem hiding this comment.
I need to add a test data skipper here.
|
@dkillick So if I want to replicate the _nd_points tests for _nd_bounds, there are lots of things which suddenly become complicated. Like for example, first I have to either construct a bounds array to test the laziness of or I have to use existing test data. Similarly, when I come to make the result using a line like this: result = AuxCoordFactory._nd_bounds(coord, (3, 2), 5)Python complains, telling me that I need to give it an AuxCoordFactory when I am actually giving it an Aux Coord instance. Again, I could either spend a lot of time trying to construct a cube which can make derived coordinates, or I could use some existing test data. In both cases, the former solution seems like more trouble than it's worth and the latter option seems to end in replication of the test I have already pushed up, so I am tempted to just keep the test as it is rather than adding extras. Would appreciate your opinions though @dkillick and @bjlittle. |
| coord = iris.coords.AuxCoord(points, bounds=bounds) | ||
| result = AuxCoordFactory._nd_bounds(coord, (3, 2), 5) | ||
| expected = bounds.T[np.newaxis, np.newaxis, ..., np.newaxis] | ||
| self.assertArrayEqual(result, expected) |
There was a problem hiding this comment.
@bjlittle This test fails due to a mismatch between the resultant and expected order of coordinates after transpose.
There was a problem hiding this comment.
@corinnebosley Doing the bounds.T[...] isn't quite what we want here, I've addressed this in https://github.com/corinnebosley/iris/pull/10/files
| self.assertTrue(is_lazy_data(coord.core_bounds())) | ||
| self.assertTrue(is_lazy_data(result)) | ||
| expected = raw_bounds.T[np.newaxis, np.newaxis, ..., np.newaxis] | ||
| self.assertArrayEqual(result, expected) |
There was a problem hiding this comment.
@bjlittle This test fails due because of something about the number of chunks that dask wants to divide stuff up into.
There was a problem hiding this comment.
@corinnebosley Turns out that for this test we were setting the chunks incorrectly. By default, lazy coordinates will have chunks set to the shape of the coordinate. See https://github.com/corinnebosley/iris/pull/10/files
|
closes #2588 |
Fix aux-factory lazy bounds.
|
@corinnebosley and @dkillick Awesome, thanks 👍 |
4 participants
This is a new PR to replace #2588. This entails the one-line change which fixes the realization of derived coord lazy bounds upon printing the cube, along with a test to ensure its effectiveness.