-
Notifications
You must be signed in to change notification settings - Fork 316
Replace dask 'compute()' usage with a common realisation call. (#2) #2447
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 2 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 |
|---|---|---|
|
|
@@ -33,7 +33,8 @@ | |
| import numpy as np | ||
| import numpy.ma as ma | ||
|
|
||
| from iris._lazy_data import as_lazy_data, is_lazy_data, multidim_lazy_stack | ||
| from iris._lazy_data import (as_lazy_data, as_concrete_data, is_lazy_data, | ||
| multidim_lazy_stack) | ||
| import iris.cube | ||
| import iris.coords | ||
| import iris.exceptions | ||
|
|
@@ -1217,10 +1218,11 @@ def merge(self, unique=True): | |
| if all_have_data: | ||
| # All inputs were concrete, so turn the result back into a | ||
| # normal array. | ||
| merged_data = merged_data.compute() | ||
| # Unmask the array only if it is filled. | ||
| merged_data = as_concrete_data(merged_data, | ||
| nans_replacement=ma.masked) | ||
| # Unmask the array if it has no masked points. | ||
| if (ma.isMaskedArray(merged_data) and | ||
| ma.count_masked(merged_data) == 0): | ||
| not ma.is_masked(merged_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. Nice spot 😉 |
||
| merged_data = merged_data.data | ||
| merged_cube = self._get_cube(merged_data) | ||
| merged_cubes.append(merged_cube) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,11 +32,11 @@ | |
| import warnings | ||
| import zlib | ||
|
|
||
| from iris._lazy_data import is_lazy_data | ||
| import dask.array as da | ||
| import netcdftime | ||
| import numpy as np | ||
|
|
||
| from iris._lazy_data import as_concrete_data, is_lazy_data | ||
| import iris.aux_factory | ||
| import iris.exceptions | ||
| import iris.time | ||
|
|
@@ -1611,7 +1611,11 @@ def _sanitise_array(self, src, ndmin): | |
| def points(self): | ||
| """Property containing the points values as a numpy array""" | ||
| if is_lazy_data(self._points): | ||
| self._points = self._points.compute() | ||
| self._points = as_concrete_data(self._points, | ||
| nans_replacement=np.ma.masked) | ||
| # NOTE: we probably don't have full support for masked aux-coords. | ||
| # We certainly *don't* handle a _FillValue attribute (and possibly | ||
| # the loader will throw one away ?) | ||
|
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. @lbdreyer We should raise a ticket to consider how we deal with masked integral data on coordinates and cell measures. At the moment we don't keep the result dtype ... this is lost in translation, which is bad.
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. @lbdreyer Did you create an issue to cover this?
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. No, but I'll do that now
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. |
||
| return self._points.view() | ||
|
|
||
| @points.setter | ||
|
|
@@ -1649,7 +1653,11 @@ def bounds(self): | |
| if self._bounds is not None: | ||
| bounds = self._bounds | ||
| if is_lazy_data(bounds): | ||
| bounds = bounds.compute() | ||
| bounds = as_concrete_data(bounds, | ||
| nans_replacement=np.ma.masked) | ||
| # NOTE: we probably don't fully support for masked aux-coords. | ||
| # We certainly *don't* handle a _FillValue attribute (and | ||
| # possibly the loader will throw one away ?) | ||
| self._bounds = bounds | ||
| bounds = bounds.view() | ||
| else: | ||
|
|
@@ -1740,9 +1748,11 @@ def measure(self): | |
| @property | ||
| def data(self): | ||
| """Property containing the data values as a numpy array""" | ||
| data = self._data | ||
| if is_lazy_data(self._data): | ||
| self._data = self._data.compute() | ||
| self._data = as_concrete_data(self._data, | ||
| nans_replacement=np.ma.masked) | ||
| # NOTE: like AuxCoords, we probably don't fully support masks, and | ||
| # we certainly don't handle any _FillValue attribute. | ||
| return self._data.view() | ||
|
|
||
| @data.setter | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| # (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.as_concrete_data`.""" | ||
|
|
||
| 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 numpy.ma as ma | ||
| import dask.array as da | ||
|
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. @lbdreyer Sorry, but could you fix this import order ...
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. Ah man! I didn't see this one! |
||
|
|
||
| from iris._lazy_data import as_concrete_data, as_lazy_data, is_lazy_data | ||
| from iris.tests import mock | ||
|
|
||
|
|
||
| class Test_as_concrete_data(tests.IrisTest): | ||
| def test_concrete_input_data(self): | ||
| data = np.arange(24).reshape((4, 6)) | ||
| result = as_concrete_data(data) | ||
| self.assertIs(data, result) | ||
| self.assertFalse(is_lazy_data(result)) | ||
|
|
||
| def test_concrete_masked_input_data(self): | ||
| data = ma.masked_array([10, 12, 8, 2], mask=[True, True, False, True]) | ||
| result = as_concrete_data(data) | ||
| self.assertIs(data, result) | ||
| self.assertFalse(is_lazy_data(result)) | ||
|
|
||
| def test_lazy_data(self): | ||
| # Minimal testing as as_concrete_data is a wrapper to | ||
| # convert_nans_array | ||
| data = np.arange(24).reshape((2, 12)) | ||
| lazy_array = as_lazy_data(data) | ||
| sentinel = mock.sentinel.data | ||
| with mock.patch('iris._lazy_data.convert_nans_array') as conv_nans: | ||
| conv_nans.return_value = sentinel | ||
| result = as_concrete_data(lazy_array) | ||
| self.assertEqual(sentinel, result) | ||
|
|
||
| # Check call to convert_nans_array | ||
| conv_nans.assert_called_once() | ||
| args, kwargs = conv_nans.call_args | ||
| arg, = args | ||
| self.assertFalse(is_lazy_data(arg)) | ||
| self.assertArrayEqual(arg, data) | ||
| self.assertEqual(kwargs, {'result_dtype': None, | ||
|
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
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. Doesn't work when one of the args is a numpy array
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. It probably would if you also mocked the array.
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. Also, in this case, neither of them are NumPy arrays... |
||
| 'nans_replacement': None}) | ||
|
|
||
| def test_lazy_data_pass_thru_kwargs(self): | ||
| # Minimal testing as as_concrete_data is a wrapper to | ||
| # convert_nans_array | ||
| data = np.arange(24).reshape((2, 12)) | ||
| lazy_array = as_lazy_data(data) | ||
| sentinel = mock.sentinel.data | ||
| with mock.patch('iris._lazy_data.convert_nans_array') as conv_nans: | ||
| conv_nans.return_value = sentinel | ||
| result = as_concrete_data(lazy_array, nans_replacement=7, | ||
| result_dtype=np.int16) | ||
| self.assertEqual(sentinel, result) | ||
|
|
||
| # Check call to convert_nans_array | ||
| conv_nans.assert_called_once() | ||
|
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 reckon you can do
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 I don't think that @lbdreyer Although, you could combine this test and the above |
||
| args, kwargs = conv_nans.call_args | ||
| arg, = args | ||
| self.assertFalse(is_lazy_data(arg)) | ||
| self.assertArrayEqual(arg, data) | ||
| self.assertEqual(kwargs, {'nans_replacement': 7, | ||
| 'result_dtype': np.int16, }) | ||
|
|
||
|
|
||
| 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.
Typo: "there" --> "their".
Alternatively you could change the signature to
as the
kwargsare not used at all by this function and are just passed straight toconvert_nans_array. At that point you could replace this description of thekwargswith something like"Kwargs are passed straight to :func:
~iris._lazy_data.convert_nans_array."It is a more typical way of doing such things in Python but also less readable...
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 "See there for usage" was meant as in "See over there for usage".
I like your idea of
**kwargsThere 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.
Ah yes, on a re-reading you're quite right!