-
Notifications
You must be signed in to change notification settings - Fork 315
Lazy masked fill_value retrieval for NC save #2723
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,6 +126,34 @@ def as_concrete_data(data): | |
| return data | ||
|
|
||
|
|
||
| def lazy_masked_fill_value(data): | ||
| """ | ||
| Return the fill value of a lazy masked array. | ||
|
|
||
| Args: | ||
|
|
||
| * data: | ||
| A dask array, NumPy `ndarray` or masked array | ||
|
|
||
| Returns: | ||
| The fill value of `data` if `data` represents a masked array, or None. | ||
|
|
||
| """ | ||
| # If lazy, get the smallest slice of the data from which we can retrieve | ||
| # the fill_value. | ||
| if is_lazy_data(data): | ||
| inds = tuple([0] * (data.ndim-1)) | ||
| smallest_slice = data[inds][:0] | ||
| data = as_concrete_data(smallest_slice) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dkillick You might want coverage for 0-dim arrays and MaskedConstants ... I've seen those being passed around on my travels
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should work; see these tests... although I'm not convinced that they cover the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would one produce a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The following works:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why calculate inds, why not just use [:0] i.e. if is_lazy_data(data):
smallest_slice = data[:0]
data = as_concrete_data(smallest_slice)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An interesting idea! I was going to say "Because then it won't be the smallest slice!", which turns out to not quite be accurate (look at the shape of the resultant array 😱): z = np.arange(24).reshape(2,3,4)
z[:0]
array([], shape=(0, 3, 4), dtype=int64)Apparently (according to an offline conversation) doing the suggested also causes the 0D case to not work properly. So, while I'm 👍 for reducing SLOC and code complexity, I think the extra line in this case adds some worthwhile resilience. |
||
|
|
||
| # Now get the fill value. | ||
| fill_value = None | ||
| if ma.isMaskedArray(data): | ||
| fill_value = data.fill_value | ||
|
|
||
| return fill_value | ||
|
|
||
|
|
||
| def multidim_lazy_stack(stack): | ||
| """ | ||
| Recursively build a multidimensional stacked dask array. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| # (C) British Crown Copyright 2017, Met Office | ||
| # | ||
| # This file is part of Iris. | ||
| # | ||
| # Iris is free software: you can redistribute it and/or modify it under | ||
| # the terms of the GNU Lesser General Public License as published by the | ||
| # Free Software Foundation, either version 3 of the License, or | ||
| # (at your option) any later version. | ||
| # | ||
| # Iris is distributed in the hope that it will be useful, | ||
| # but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| # GNU Lesser General Public License for more details. | ||
| # | ||
| # You should have received a copy of the GNU Lesser General Public License | ||
| # along with Iris. If not, see <http://www.gnu.org/licenses/>. | ||
| """Test the function :func:`iris._lazy data.lazy_masked_fill_value`.""" | ||
|
|
||
| from __future__ import (absolute_import, division, print_function) | ||
| from six.moves import (filter, input, map, range, zip) # noqa | ||
|
|
||
| # Import iris.tests first so that some things can be initialised before | ||
| # importing anything else. | ||
| import iris.tests as tests | ||
|
|
||
| import dask.array as da | ||
| import numpy as np | ||
| import numpy.ma as ma | ||
|
|
||
| from iris._lazy_data import lazy_masked_fill_value, _MAX_CHUNK_SIZE | ||
|
|
||
|
|
||
| class Test_as_lazy_data(tests.IrisTest): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs renaming
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good spot! Thought I'd got away with copying an existing file... |
||
| def setUp(self): | ||
| shape = (2, 3, 4) | ||
| data = np.arange(24).reshape(shape) | ||
| mask = np.zeros(shape) | ||
| mask[data % 5 == 1] = 1 | ||
| self.fill_value = 9999 | ||
| self.m = ma.masked_array(data, mask=mask, fill_value=self.fill_value) | ||
| self.dm = da.from_array(self.m, asarray=False, | ||
| chunks=_MAX_CHUNK_SIZE) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you not just call
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could, but I don't see how that benefits me / improves the existing approach... So if you have a good benefit / improvement for changing this I'd love to hear it!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well that's just the argument we initially had of whether to include
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fine... apart from the fact that it doesn't hold in the tests, which make widespread use of Either way, this intermediate step has now been banked, so do feel free to change this in a follow-up PR 😄 |
||
|
|
||
| def test_lazy_masked_ND(self): | ||
| fill_value = lazy_masked_fill_value(self.dm) | ||
| self.assertEqual(fill_value, self.fill_value) | ||
|
|
||
| def test_lazy_masked_0D(self): | ||
| data = self.dm[0, 0, :1] | ||
| fill_value = lazy_masked_fill_value(data) | ||
| self.assertEqual(fill_value, self.fill_value) | ||
|
|
||
| def test_lazy_masked_1D(self): | ||
| data = self.dm[0, 0, :] | ||
| fill_value = lazy_masked_fill_value(data) | ||
| self.assertEqual(fill_value, self.fill_value) | ||
|
|
||
| def test_lazy_masked_2D(self): | ||
| data = self.dm[0, :] | ||
| fill_value = lazy_masked_fill_value(data) | ||
| self.assertEqual(fill_value, self.fill_value) | ||
|
|
||
| def test_real_masked(self): | ||
| fill_value = lazy_masked_fill_value(self.m) | ||
| self.assertEqual(fill_value, self.fill_value) | ||
|
|
||
| def test_lazy_unmasked(self): | ||
| data = da.from_array(self.m.filled(), | ||
| chunks=_MAX_CHUNK_SIZE) | ||
| fill_value = lazy_masked_fill_value(data) | ||
| self.assertIsNone(fill_value) | ||
|
|
||
| def test_real_unmasked(self): | ||
| data = self.m.filled() | ||
| fill_value = lazy_masked_fill_value(data) | ||
| self.assertIsNone(fill_value) | ||
|
|
||
| def test_data_not_realised(self): | ||
| # Check that only the zero-element slice is realised. | ||
| data = self.dm[0, :] | ||
| lazy_masked_fill_value(data) | ||
| self.assertIsInstance(data, da.core.Array) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| tests.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you but into a rebrand, say "lazy_fill_value" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're going to have to try harder than that to convince me 😉
I guess that...
So I'm not currently buying into a rebrand! What do you see as the benefit of doing so?