enforce non-masked dimension coordinate points/bounds #3427

bjlittle · 2019-09-30T14:17:20Z

This PR addresses #3419.

bjlittle · 2019-09-30T14:17:40Z

Happy to add more tests as a review action...

bjlittle · 2019-09-30T14:18:41Z

lib/iris/coords.py

            # NB. Returns the *same object* if result.ndim >= ndmin
-            result = np.array(result, ndmin=ndmin, copy=False)
+            func = ma.array if ma.isMaskedArray(result) else np.array
+            result = func(result, ndmin=ndmin, copy=False)


As @rcomer points out, this is indeed the nub of the issue 👍

This bit will applies to AuxCoords rather than DimCoords, which was the point of the original issue.
I'll try and remember to edit the squashed commit for that.

bjlittle · 2019-09-30T14:20:24Z

lib/iris/coords.py

+            raise ValueError(emsg.format(self.name(), self.__class__.__name__))
+        if ma.isMaskedArray(points) and ma.count_masked(points):
+            emsg = 'A {!r} {} points array must not be masked.'
+            raise TypeError(emsg.format(self.name(), self.__class__.__name__))


Raise a TypeError exceptions here to explicitly catch those masked points. Hence the need for the change here.

bjlittle · 2019-09-30T14:27:55Z

docs/iris/src/userguide/interpolation_and_regridding.rst

-   [       nan  494.44452  588.8889   683.33325  777.77783  872.2222
-     966.66675 1061.1111  1155.5554         nan]
+   [-- 494.44451904296875 588.888916015625 683.333251953125 777.77783203125
+    872.2222290039062 966.666748046875 1061.111083984375 1155.555419921875 --]


This really makes sense to me now for coordinate behaviour...

We've asked for an extrapolation_mode=mask, so now we get a masked array for the altitude derived coordinate for the extrapolated points, and not just an array with nans injected.

If the extrapolation_mode=nan, then we (quite rightly) instead get:

[ nan 494.44452 588.8889 683.33325 777.77783 872.2222 966.66675 1061.1111 1155.5554 nan]

bjlittle · 2019-09-30T15:07:28Z

@pp-mo Yup, I definitely want to add some extra test coverage here. I think that our masked array test coverage (in gerneral) is pretty poor (at least an after thought).

I just want to convince myself that I've not broken anything here for DimCoords and AuxCoords... as it's pretty fundamental to the core of iris 😟

pp-mo · 2019-09-30T15:16:43Z

lib/iris/coords.py

-            raise ValueError('The points array must be numeric.')
+            emsg = 'The {!r} {} points array must be numeric.'
+            raise ValueError(emsg.format(self.name(), self.__class__.__name__))
+        if ma.isMaskedArray(points) and ma.count_masked(points):


Should probably be using 'is_masked' for this : https://docs.scipy.org/doc/numpy/reference/generated/numpy.ma.is_masked.html

Yup, thanks @pp-mo ... I always forget that pattern, and it gives us the same behaviour that we want, namely:

>>> import numpy as np >>> import numpy.ma as ma >>> a = np.arange(5) >>> b = ma.arange(5) >>> c = ma.arange(5) >>> c[0] = ma.masked >>> ma.is_masked(a) False >>> ma.is_masked(b) False >>> ma.is_masked(c) True

@pp-mo Done 👍

pp-mo · 2019-09-30T15:17:15Z

lib/iris/coords.py

+            emsg = 'The {!r} {} bounds array must be numeric.'
+            raise ValueError(emsg.format(self.name(), self.__class__.__name__))
+        # Check not masked.
+        if ma.isMaskedArray(bounds) and ma.count_masked(bounds):


As above, use 'is_masked'

@pp-mo Done 👍

rcomer · 2019-09-30T15:22:26Z

One thought that springs to mind: do methods such as cube.extract and cube.aggregated_by behave sensibly if presented with masked points?

pp-mo

All makes good sense, but clearly we have no testing for these behaviours : e.g.

errors for bad creation / assignment
successful AuxCoord creation with masked data
error making DimCoord with masked points
success making DimCoord from masked array with no masked points.

Maybe we should address these, just as regression checks ?

pp-mo · 2019-09-30T16:03:12Z

lib/iris/coords.py

            # NB. Returns the *same object* if result.ndim >= ndmin
-            result = np.array(result, ndmin=ndmin, copy=False)
+            func = ma.array if ma.isMaskedArray(result) else np.array
+            result = func(result, ndmin=ndmin, copy=False)


This bit will applies to AuxCoords rather than DimCoords, which was the point of the original issue.
I'll try and remember to edit the squashed commit for that.

pp-mo · 2019-09-30T16:36:23Z

One thought that springs to mind: do methods such as cube.extract and cube.aggregated_by behave sensibly if presented with masked points?

I think we have considered masking in most operations now, but it is not all tested.
Specifically, extract should be fine, aggregation does work (I just checked), but it is not tested.

stickler-ci · 2019-10-01T09:48:48Z

lib/iris/tests/unit/coords/test_AuxCoord.py


 from __future__ import (absolute_import, division, print_function)
 from six.moves import (filter, input, map, range, zip)  # noqa
+import six


F401 'six' imported but unused

bjlittle · 2019-10-01T09:57:18Z

All makes good sense, but clearly we have no testing for these behaviours : e.g.
* errors for bad creation / assignment

* successful AuxCoord creation with masked data

* error making DimCoord with masked points

* success making DimCoord from masked array with no masked points.
Maybe we should address these, just as regression checks ?

@pp-mo After pushing commits 9f9a553 and 91ceae9, I'm pretty satified that we've got reasonable test coverage for both DimCoords and AuxCoords given the areas you highlighted. But this is just for coordinate creation.

Did we want to extend this further?

Pragmatically, perhaps we create an issue with areas of the code base that require masking test coverage outside this PR? Kinda feels like slow, long burner...

bjlittle · 2019-10-01T10:19:25Z

This need a whatsnew entry...

pp-mo

Some silly little points, but generally all GTG, OWOK, LGTM ...

.../iris/src/whatsnew/contributions_2.3.0/bugfix_2019-Oct-01_masked_dimcoords_and_auxcoords.txt

lib/iris/tests/unit/coords/__init__.py

lib/iris/tests/unit/coords/test_AuxCoord.py

lib/iris/tests/unit/coords/test_DimCoord.py

bjlittle · 2019-10-02T11:57:55Z

@pp-mo Over to you 👍

pp-mo · 2019-10-02T14:14:09Z

Nice @bjlittle
👍 for minimal fuss over this one !

enforce non-masked dimension coordinate points/bounds

c14057e

bjlittle added Type: Bug Release: Minor labels Sep 30, 2019

bjlittle added this to the v2.3.0 milestone Sep 30, 2019

bjlittle commented Sep 30, 2019

View reviewed changes

make _sanitise_array a static method

9f828b2

pp-mo self-assigned this Sep 30, 2019

pp-mo self-requested a review September 30, 2019 14:49

pp-mo reviewed Sep 30, 2019

View reviewed changes

Review actions + masked DimCoord test coverage

9f9a553

stickler-ci reviewed Oct 1, 2019

View reviewed changes

masked AuxCoord test coverage

91ceae9

bjlittle force-pushed the fix-sanitise-array branch from 3ea2f1f to 91ceae9 Compare October 1, 2019 09:50

update license header year

6281082

Add an associated whatsnew entry

6d06e60

pp-mo reviewed Oct 1, 2019

View reviewed changes

review actions

8f0bc03

pp-mo merged commit 3a6a2b9 into SciTools:master Oct 2, 2019

rcomer mentioned this pull request Oct 2, 2019

Coord masked points #3419

Closed

bjlittle added a commit to bjlittle/iris that referenced this pull request Oct 16, 2019

enforce non-masked dimension coordinate points/bounds (SciTools#3427)

57090e0

bjlittle deleted the fix-sanitise-array branch October 29, 2019 10:24

enforce non-masked dimension coordinate points/bounds #3427

enforce non-masked dimension coordinate points/bounds #3427

Uh oh!

Conversation

bjlittle commented Sep 30, 2019

Uh oh!

bjlittle commented Sep 30, 2019

Uh oh!

bjlittle Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

bjlittle Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pp-mo Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

bjlittle Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

pp-mo Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

bjlittle Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

rcomer commented Sep 30, 2019

Uh oh!

pp-mo left a comment

Choose a reason for hiding this comment

Uh oh!

pp-mo Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Sep 30, 2019

Uh oh!

stickler-ci Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

bjlittle commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjlittle commented Oct 1, 2019

Uh oh!

pp-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bjlittle commented Oct 2, 2019

Uh oh!

pp-mo commented Oct 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bjlittle Sep 30, 2019 •

edited

Loading

bjlittle Sep 30, 2019 •

edited

Loading

bjlittle Sep 30, 2019 •

edited

Loading

bjlittle commented Sep 30, 2019 •

edited

Loading

bjlittle Oct 1, 2019 •

edited

Loading

bjlittle commented Oct 1, 2019 •

edited

Loading