-
Notifications
You must be signed in to change notification settings - Fork 314
Reuse multidim_daskstack in merge + fast um loading. #2423
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 3 commits
d150fe6
975105b
717e7cf
62e25e5
e69c46f
2b9fe09
34f810d
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 |
|---|---|---|
|
|
@@ -32,12 +32,10 @@ | |
| from netCDF4 import netcdftime | ||
| import numpy as np | ||
|
|
||
| from iris._lazy_data import as_lazy_data, multidim_lazy_stack | ||
| from iris.fileformats.um._optimal_array_structuring import \ | ||
| optimal_array_structure | ||
|
|
||
| from iris.fileformats.pp import PPField3 | ||
| from iris._lazy_data import as_lazy_data | ||
|
|
||
|
|
||
| class FieldCollation(object): | ||
| """ | ||
|
|
@@ -89,15 +87,11 @@ def data(self): | |
| if not self._structure_calculated: | ||
| self._calculate_structure() | ||
| if self._data_cache is None: | ||
| data_arrays = [as_lazy_data(f._data, chunks=f._data.shape) | ||
| for f in self.fields] | ||
| vector_dims_list = list(self.vector_dims_shape) | ||
| vector_dims_list.reverse() | ||
| self._data_cache = data_arrays | ||
| for size in vector_dims_list: | ||
| self._data_cache = [da.stack(self._data_cache[i:i+size]) for i | ||
| in range(0, len(self._data_cache), size)] | ||
| self._data_cache, = self._data_cache | ||
| stack = np.empty(np.prod(self.vector_dims_shape), 'object') | ||
| for index, f in enumerate(self.fields): | ||
|
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. Haha, I so would have gone for "
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 point. I used f as that was what was used in the previous code. I will change it to |
||
| stack[index] = as_lazy_data(f._data, chunks=f._data.shape) | ||
| stack = stack.reshape(self.vector_dims_shape) | ||
| self._data_cache = multidim_lazy_stack(stack) | ||
| return self._data_cache | ||
|
|
||
| @property | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| # | ||
| # 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 :meth:`iris._lazy data.array_masked_to_nans` method.""" | ||
| """Test function :func:`iris._lazy data.array_masked_to_nans`.""" | ||
|
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. 👍 |
||
|
|
||
| from __future__ import (absolute_import, division, print_function) | ||
| from six.moves import (filter, input, map, range, zip) # noqa | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| # | ||
| # 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 :meth:`iris._lazy data.is_lazy_data` method.""" | ||
| """Test function :func:`iris._lazy data.is_lazy_data`.""" | ||
|
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. 👍 |
||
|
|
||
| from __future__ import (absolute_import, division, print_function) | ||
| from six.moves import (filter, input, map, range, zip) # noqa | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # (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 function :func:`iris._lazy data.multidim_lazy_stack`.""" | ||
|
|
||
| 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 numpy as np | ||
| import dask.array as da | ||
|
|
||
| from iris._lazy_data import as_lazy_data, multidim_lazy_stack | ||
|
|
||
|
|
||
| class Test_multidim_lazy_stack(tests.IrisTest): | ||
| def _check(self, stack_shape): | ||
| vals = np.arange(np.prod(stack_shape)).reshape(stack_shape) | ||
| stack = np.empty(stack_shape, 'object') | ||
| stack_element_shape = (4, 5) | ||
|
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. These magic numbers could probably do with a little explanation...
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. Not magic numbers. I just chose them as they were small enough that I could produce a 4D result (for It's equivalent to a pp field data of shape (4,5)
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. Technically they are, as they're numbers that appear without much introduction! A comment on the line above that said "equivalent to a pp field data of shape (4,5)" would nicely sort this and improve code understand-ability, I think.
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. It's possibly better to think of the numbers as "test input", similar to when you create an array to test an array operation, that array is thought of as "test input data". In these tests, I am creating a stack of My worry with the comment "equivalent to a pp field data of shape (4,5)" is that that is an example of an implementation of multidim_lazy_stack. I would consider renaming the variable though. Would |
||
| for index, val in np.ndenumerate(vals): | ||
| stack[index] = as_lazy_data(val*np.ones(stack_element_shape)) | ||
| result = multidim_lazy_stack(stack) | ||
| self.assertEqual(result.shape, stack_shape + stack_element_shape) | ||
|
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. I wonder if we could also check the actual values, to ensure that the ordering of elements is definitely correct ?
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.
Surely that's already achieved by using dimensions of different lengths? Not that I'm against adding extra assurance. |
||
| self.assertIsInstance(result, da.core.Array) | ||
|
|
||
| def test_0d_lazy_stack(self): | ||
| shape = () | ||
| result = self._check(shape) | ||
|
|
||
| def test_1d_lazy_stack(self): | ||
| shape = (2,) | ||
| result = self._check(shape) | ||
|
|
||
| def test_2d_lazy_stack(self): | ||
|
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. Is there any chance of a >2D test?
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. It seems a bit unnecessary? The 2d test is already testing the recursivity
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. Indeed, however part of testing is checking edge cases, and I don't know what would happen if I passed a 7D array to it. Evidently we don't need to test all possible dimensionalities (!), but it would be good to test something that's outside of the boundary of logical intent for the functionality being tested. This is just in case an unsafe assumption has been made about how the function will be used; "of course, no-one would ever use this for more than a 2D input".
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.
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. From a mockist / white-box-y point of view, the existing tests do already cover all 3 code branches. |
||
| shape = (3, 2) | ||
| result = self._check(shape) | ||
|
|
||
|
|
||
| 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.
I think it would be neater + clearer to iterate with a multidimensional index here, thus avoiding the
np.prod(shape)and the subsequentstack.reshape.Your magic friend for that is
np.ndindex(shape), which gives you indices to all elements of a multidimensional array, as used here for exampleI think that gets you something like ...