Rework lazy_masked_fill_value. #2729
Conversation
pelson
added
Blocked: CLA needed
|
Sorry about the noise on this PR. I've enabled the SciTools CLA checker and currently that doesn't include the people it needs to (I'll update the contributors.json on Monday). |
pelson
removed
the
Blocked: CLA needed
| data = as_concrete_data(data) | ||
| else: | ||
| if isinstance(data, ma.core.MaskedConstant): | ||
| data = ma.array(data.data, mask=data.mask) |
There was a problem hiding this comment.
Worth a comment why you need to do this ?
| inds = tuple([0] * (data.ndim-1)) | ||
| smallest_slice = data[inds][:0] | ||
| data = as_concrete_data(smallest_slice) | ||
| inds = [0] * data.ndim |
There was a problem hiding this comment.
I think I would use data = data.flatten()[0:0] instead of all this : The intent would be clearer, no ?
There was a problem hiding this comment.
Functionally quite different though - for NetCDF that could mean loading the entire dataset and then discarding all the actual values.
| fill_value = lazy_masked_fill_value(cube.lazy_data()) | ||
| else: | ||
| fill_value = None | ||
| fill_value = get_fill_value(cube.core_data()) |
| self.lazy_arrays = [as_lazy_data(array) for array in self.arrays] | ||
| self.lazy_masked = [as_lazy_data(array) for array in self.masked] | ||
| # Add the masked constant case. | ||
| mc = ma.array([0], mask=True)[0] |
There was a problem hiding this comment.
I think it would be better to specify a definite, non-default fill-value here, like all the other cases.
Just to make sure we're not getting a default instead.
|
|
||
| def test_arrays(self): | ||
| for array in self.arrays: | ||
| self.assertIsNone(get_fill_value(array)) |
There was a problem hiding this comment.
As you're testing multiple cases, it would be nice to have a custom context message so if one fails the output shows which one it was.
That goes for all these tests...
There was a problem hiding this comment.
Is this the moment to use test parametrisation from pytest? It's the future 😉
There was a problem hiding this comment.
Meanwhile, back in the past... nose has poor man's parametrisation in the form of test generators: http://nose.readthedocs.io/en/latest/writing_tests.html#test-generators
|
Still think there were things we could improve here, but we don't have time for it, so better to move on. |
5 participants
This PR addresses some of the outstanding issues on PR #2723