Public contiguity checking by corinnebosley · Pull Request #3144 · SciTools/iris

corinnebosley · 2018-08-15T15:46:10Z

This PR contains:

plot.py:

minor changes to function names, just for clarity
if a user tries to plot an unmasked discontiguous array, they now get a warning with advice on how to fix this rather than an error

test_2d_coords.py:

changed a class name to something more appropriate
moved some tests from Test_2d_coords_bound_mode to new class TestContiguityChecking
Added minimal set of tests to TestContiguityChecking

util.py:

new function find_discontiguity()
new_function mask_discontiguity()

Enjoy!

stickler-ci · 2018-08-15T15:46:34Z

+                                    'where the discontiguity occurs. The ' \
+                                    'mesh created for this plot may cause a ' \
+                                    'misrepresentation of the input data.  ' \
+                                    'Please use iris.contiguity_check(coord) ' \


E501 line too long (80 > 79 characters)

stickler-ci · 2018-08-15T15:46:34Z

        anchor.patch.set_boxstyle('round, pad=0, rounding_size=0.2')
        axes = axes if axes else figure.gca()
        axes.add_artist(anchor)
+


W391 blank line at end of file

stickler-ci · 2018-08-15T15:46:35Z

+        returned = find_discontiguities(coord)
+        self.assertEqual(expected, returned)
+
+        # TODO Make sure everything works when there are multiple discontiguities


E501 line too long (81 > 79 characters)

corinnebosley · 2018-08-15T15:47:55Z

I can already see that I could do with making these changes:

change test_2d_coords.py class name 'Test_2d_coords_bound_mode' to camel case
add test for multiple discontiguities (as stated in TODO which is left in there)

lbdreyer · 2018-08-16T11:42:41Z



-def _check_contiguity_and_bounds(coord, data, abs_tol=1e-4, transpose=False):
+def _check_contiguity_and_mask(coord, data, abs_tol=1e-4, transpose=False):


I agree with the inclusion of mask but by not mentioning bounds it isn't clear that that's what it is referring to.
How about _check_bounds_contiguity_and_mask?

Yeah that'll do. It's a bit of a mouthful but it's private anyway so it shouldn't matter.

lbdreyer · 2018-08-16T11:44:47Z

-                             ' Matplotlib'.format(coord.name()))
+            discontiguity_warning = 'The bounds of the {} coordinate are ' \
+                                    'not contiguous and data is not masked ' \
+                                    'where the discontiguity occurs. The ' \


Not a change you've made but now rereading through this intial sentence I see it doesn't make sense. We say discontiguous twice: "The bounds of the {} coordinate are not contiguous ... where the discontiguity occurs.".

Oh I see, this says "there is a discontiguity and the data is not masked where that discontiguity is (or are)"

corinnebosley · 2018-08-16T11:54:44Z

@lbdreyer The changes I made for the last commit prompted numpy to give me this warning:
"FutureWarning: Using a non-tuple sequence for multidimensional indexing is deprecated; use arr[tuple(seq)] instead of arr[seq]. In the future this will be interpreted as an array index, arr[np.array(seq)], which will result either in an error or a different result."

I'm not exactly sure what it's referring to, I have a horrible feeling it's something to do with the zip output, and that's been a really horrible nightmare to put into a user-friendly return format.

We will obviously have to address this at some point, but I could do with your help with it I think.

lbdreyer · 2018-08-16T11:55:43Z

 """

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


We are using these two lines as boilerplate for all our modules to help with Python 3 compatibility. (you'll find them at the top of all files.)
I think it would be best to keep all of them (even if they aren't being used) as otherwise we may forget to include them.
In fact that is what the #noqa tag is there for - to stop flake8 complaining about unused imports

I didn't even realise it wasn't there. I'll pop it in.

No idea how I removed that...

lbdreyer · 2018-08-16T12:46:46Z

+
+        (b) the :attr:`standard_name`, :attr:`long_name`, or
+        :attr:`var_name` of an instance of an instance of
+        :class:`iris.coords.Coord`.


If you are going to allow for a user to do this, you need to also add a line at the start that gets the coordinate.
This function starts by doing

_, diffs_x, diffs_y = iris.coords._discontiguity_in_2d_bounds(coord.bounds, abs_tol)

so if that needs to be a coord instance by then

Thing is, if we do allow that user to use a name instead of a coord object, then we have to pass the cube in as well so that we can get the coord. This seems a bit unnecessary, so it might make sense to only allow a coord object.

stickler-ci · 2018-08-16T13:02:28Z

                try:
-                    _check_contiguity_and_bounds(coord, data=cube.data,
-                                                 abs_tol=twodim_contig_atol)
+                    _check_contiguity_and_mask(coord, data=cube.data,


F821 undefined name '_check_contiguity_and_mask'

stickler-ci · 2018-08-16T13:02:28Z

-                    if _check_contiguity_and_bounds(coord, data=cube.data,
-                                                    abs_tol=twodim_contig_atol,
-                                                    transpose=True) is True:
+                    if _check_contiguity_and_mask(coord, data=cube.data,


F821 undefined name '_check_contiguity_and_mask'

stickler-ci · 2018-08-16T13:07:16Z

-                    _check_contiguity_and_bounds(coord, data=cube.data,
-                                                 abs_tol=twodim_contig_atol)
+                    _check_bounds_contiguity_and_mask(coord, data=cube.data,
+                                               abs_tol=twodim_contig_atol)


E128 continuation line under-indented for visual indent

stickler-ci · 2018-08-16T13:07:17Z

-                                                    abs_tol=twodim_contig_atol,
-                                                    transpose=True) is True:
+                    if _check_bounds_contiguity_and_mask(coord, data=cube.data,
+                                                  abs_tol=twodim_contig_atol,


E128 continuation line under-indented for visual indent

stickler-ci · 2018-08-16T13:07:17Z

-                                                    transpose=True) is True:
+                    if _check_bounds_contiguity_and_mask(coord, data=cube.data,
+                                                  abs_tol=twodim_contig_atol,
+                                                  transpose=True) is True:


E128 continuation line under-indented for visual indent

pp-mo · 2018-08-16T15:19:50Z

+        absolute tolerance in degrees
+
+    Returns:
+        Dictionary of values representing data array indices where


I'm a bit 👎 on this.
Would it not be much simpler to return an boolean array of ok/bad-point values ?
Likewise in the code, avoid use of np.where + just use boolean arrays
bad = (diffs_x > abs_tol) | (diffs_y > abs_tol)

Not really. Not simpler. You still have to move everything along by one point to the right for x-values and down for y-values because the bad point is after the discontiguity in the array. That's really where the complication arises.

Also I don't see how that's any more use to the user. If they have a particularly large dataset then they will have to search through it for places where it's True anyway, so what's the point?

And also, would we return the boolean array for the data or for the coord points? What are you expecting the user to do with a boolean array?

pp-mo · 2018-08-16T15:22:25Z

+        return None
+
+
+def mask_discontiguities(coord, data, abs_tol=1e-4):


I thought if we did have a simple boolean array from the previous, this routine would not be needed..
Something like

badpts = find_discontiguities(coord) points = ma.masked_array(coord.points) points[badpts] = ma.masked coord.points = points

A boolean array would be fine for this because we don't need anything to be human-readable; we are just doing operations. But I still disagree with creating a boolean array for the previous.

Also what do you mean by 'this routine would not be needed?'

This routine is pretty much as simple as it gets. The whole point of having the two separate routines is that one allows you to check which points are discontiguous (as a human) and look at them and think about them, and the other allows you to just mask them without thought. Those are both valid options and it seems stupid to just not offer one of them because that's simpler.

stickler-ci · 2018-08-20T13:39:22Z

+        # Construct an array the size of the points array filled with 'False'
+        # to represent an empty discontiguity array
+        self.empty_bool = np.zeros((self.lons.points.shape),
+                                    dtype=bool)


E127 continuation line over-indented for visual indent

corinnebosley · 2018-08-22T11:03:29Z

Changes following last review:

find_discontiguities now returns a boolean array, not a dictionary (although it was not simple, thank you very much)
make_bounds_discontiguous_at_point now broadcasts the discontiguity mask properly to handle cubes with more than 2 dimensions
mask_discontiguities also broadcasts the 2d mask to data arrays with more than 2 dimensions
temporary tests have been updated to check that the broadcasting works

corinnebosley · 2018-08-22T14:57:45Z

@lbdreyer All tests passed. What do you think?

lbdreyer

Looking good! I have a few comments, mostly renaming things more descriptive names.

The tests are in a bit of a jumble. Maybe keep the old tests where they are (as I have moved them in the PR I am working on, but feel free to do so in this PR instead), but the new tests that are introduced in this PR (e.g. for find_discontiguities and mask_discontiguities) should go in the appropriate places

lbdreyer · 2018-08-22T15:23:57Z


        # If discontinuity occurs but not masked, any grid will be created
-        #  incorrectly, so raise a warning
+        #  incorrectly, so raise a warning for the user


You don't really need this clarification as all warnings are for the user

lbdreyer · 2018-08-22T15:26:04Z

+                                    'Please use ' \
+                                    'iris.contiguity_check(coord) to find ' \
+                                    'and mask your data ' \
+                                    'appropriately'.format(coord.name())


Don't you mean iris.util.find_discontiguities and iris.util.mask_discontiguities instead of iris.contiguity_check?

Yep, this is leftover from before the full implementation.

Is this going to end up a bit long and rambly?

lbdreyer · 2018-08-22T15:26:47Z

+                                    'iris.contiguity_check(coord) to find ' \
+                                    'and mask your data ' \
+                                    'appropriately'.format(coord.name())
+            warnings.warn(discontiguity_warning)


We have chosen to raise this as an error but I make that change in the next PR so we'll leave this as it is in this PR

Ah. Good decision.

lbdreyer · 2018-08-22T15:30:49Z

+    try:
+        return bad_points_boolean
+    except ValueError:
+        return None


I assume you forgot to remove this? Otherwise it will catch all errors which would working out what is going wrong difficult.

Yes I did. I needed this for an earlier implementation but it will never return None now (unless it's broken), so you're quite right.

lbdreyer · 2018-08-22T16:02:19Z

+        # awkward but supported cube.
+        cube = self.latlon_2d
+        result = iplt._draw_2d_from_bounds('pcolormesh', cube)
+        self.assertTrue(result)


draw_2d_from_bounds is too complicated to unit test. I suggest you just remove this test.
I am in the process of adding an integration test that will test draw_2d_from_bounds (which will at the same time check that it is not raising an error)

Yeah, this was meant as a placeholder really, I think it was one of the first tests I put in, just to help me think. I'll take it out.

lbdreyer · 2018-08-22T16:21:31Z

+        Copy of the data array, masked at points where bounds are
+        discontiguous.
+    """
+    mask = find_discontiguities(coord, abs_tol=abs_tol)


mask isn't that clear what it means as it doesn't refer to the cube's original mask, it refers to the locations of the discontiguities and so the new places that should be masked.
How about something like discontiguous_locations or discontiguous or discontiguous_bounds_locations... I can't really think of a good name.

Maybe just new_mask or extra_mask

lbdreyer · 2018-08-22T16:40:30Z

+
+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
+            with self.assertRaises(UserWarning):


This really needs a assertRaisesRegexp to check it's getting the right warning`

lbdreyer · 2018-08-22T16:46:18Z

+        self.empty_bool = np.zeros(self.lons.points.shape,
+                                   dtype=bool)
+        self.masked = self.empty_bool.copy()
+        self.masked[3, 3] = True


Why not just use the mask of self.latlon_2d?

And unmask it first? I suppose so. Don't see that it makes a lot of difference but I'll pop it in anyway.

Oh no wait a minute, I'm not quite sure what I'm actually trying to achieve here, maybe I need to think about this a bit more and add some comments.

I think I will need the array of Falses that I construct here because that's the easiest way to do it, but there is no need to then mask the single point I have done here. I can just use the cube's mask instead of constructing self.masked.

lbdreyer · 2018-08-22T16:47:57Z

+    def setUp(self):
+        self.latlon_2d = full2d_global()
+        make_bounds_discontiguous_at_point(self.latlon_2d, 3, 2)
+        self.lons = self.latlon_2d.coord('longitude')


I'd prefer more descriptive names, it would make reading the test easier, so:
self.latlon_2d_cube and self.lons_coord

stickler-ci · 2018-08-23T13:56:45Z


+import numpy as np
 import numpy.ma as ma
+import warnings


F401 'warnings' imported but unused

stickler-ci · 2018-08-23T13:56:46Z

+import warnings

 import iris.coords as coords
+import iris.util


F401 'iris.util' imported but unused

stickler-ci · 2018-08-23T13:56:46Z

+#     #     with warnings.catch_warnings():
+#     #         warnings.simplefilter("error")
+#     #         with self.assertRaises(UserWarning):
+#     #             iris.plot._check_bounds_contiguity_and_mask(coord, cube.data)


E501 line too long (81 > 79 characters)

stickler-ci · 2018-08-23T13:56:46Z

+        # Construct an array the size of the points array filled with 'False'
+        # to represent a mask showing no discontiguities
+        empty_mask = np.zeros(self.longitude_coord.points.shape,
+                                   dtype=bool)


E127 continuation line over-indented for visual indent

stickler-ci · 2018-08-23T13:56:46Z

+
+    # Apply mask for x-direction discontiguities:
+    bad_points_boolean[:, :-1] = np.logical_or(bad_points_boolean[:, :-1],
+                                              gaps_x)


E128 continuation line under-indented for visual indent

stickler-ci · 2018-08-23T13:56:46Z

+
+    # apply mask for y-direction discontiguities:
+    bad_points_boolean[:-1, :] = np.logical_or(bad_points_boolean[:-1, :],
+                                              gaps_y)


E128 continuation line under-indented for visual indent

stickler-ci · 2018-08-23T14:15:04Z

+import iris.plot
 import iris.tests as tests

+import numpy as np


F401 'numpy as np' imported but unused

stickler-ci · 2018-08-23T14:15:04Z


 import iris.coords as coords
+import iris.util
+from iris.util import find_discontiguities_in_bounds


F401 'iris.util.find_discontiguities_in_bounds' imported but unused

stickler-ci · 2018-08-23T14:15:04Z

 import iris.coords as coords
+import iris.util
+from iris.util import find_discontiguities_in_bounds
+from iris.util import mask_data_at_discontiguities


F401 'iris.util.mask_data_at_discontiguities' imported but unused

stickler-ci · 2018-08-23T14:15:04Z

+from iris.util import mask_data_at_discontiguities
 from iris.tests.stock import simple_2d_w_multidim_coords as cube_2dcoords
 from iris.tests.stock import simple_3d_w_multidim_coords as cube3d_2dcoords
+from iris.tests.stock import simple_3d


F401 'iris.tests.stock.simple_3d' imported but unused

stickler-ci · 2018-08-23T14:15:05Z

+from iris.tests.stock import simple_3d
 from iris.tests.stock import sample_2d_latlons
 from iris.tests.stock import make_bounds_discontiguous_at_point
+from iris.tests.stock._stock_2d_latlons import grid_coords_2d_from_1d


F401 'iris.tests.stock._stock_2d_latlons.grid_coords_2d_from_1d' imported but unused

stickler-ci · 2018-08-23T14:15:05Z

+        # Construct an array the size of the points array filled with 'False'
+        # to represent a mask showing no discontiguities
+        empty_mask = np.zeros(self.longitude_coord.points.shape,
+                                   dtype=bool)


E127 continuation line over-indented for visual indent

corinnebosley · 2018-08-23T15:40:17Z

Had some trouble with make_bounds_discontiguous_at_point here. If you use full2d_global (which is just sample_2d_latlons(transformed=True), some of the points become miniscule and therefore the differences between bottom-left bound and bottom-right bound are also miniscule.

I have been finding that many coordinate points from full2d_global (such as (0, 0) and (3, 2)) are so small that the discontiguity created by make_bounds_discontiguous_at_point is well below the default absolute tolerance in find_discontiguities_in_bounds.

This is possibly a good test for a relative tolerance option though...

corinnebosley · 2018-08-23T15:47:34Z

@lbdreyer I'm slightly worried about bits still being a problem, particularly test data. But for now, I'm afraid this is the best you're gonna get. Thanks for all your help and good luck getting to the finish with this.

pp-mo · 2018-08-28T17:40:50Z

Hi @corinnebosley @lbdreyer
👍 for good stuff + progress made

However, discussing today with @lbdreyer I'm still not very sure about the API.
We could easily combine these two functions now.
Or, we could make the mask_data_at_discontiguities routine into a more general "apply_mask" operation. But in that case it should really work with a general mask input, for which purpose a boolean array is not good because the dimensions to apply it to are unknown.
So actually, ideally, I think that the main "find_discontiguities.." result should be wrapped as a cube containing coords, so that the 2d mask can be mapped to the dimensions it applies to.

It also seems to me that this should absolutely be checking both x- and y-coords of a given cube, or every use will be calling it twice ??

Rather more seriously ... I'm wondering now where + why we need this ??
If we haven't an immediate use within the collected test data, then I this this has to take low priority.
I'll go + have a look for any existing test data that needs it ...

pp-mo · 2018-09-05T14:58:56Z

@pp-mo Rather more seriously ... I'm wondering now where + why we need this

Forget that -- I found out why today !
Some data wouldn't plot. No easy means of working out why not, let alone fixing it.

DPeterK · 2018-10-03T09:48:52Z

... and consider #3105 while getting this fixed.

…orking pylint corrections, small corrections to zip nightmare and return format corrections for Laura reinstated imports of six that pycharm keeps secretly changing some changes on Laura's suggestions, more to come pylint changes more stickler requested changes (not pylint at all) some fixes and tweaks

pp-mo

Bunch of trivial points !!

PR moved on since suggested changes (some changes made, others discarded)

stickler-ci · 2018-10-10T10:17:06Z

+                                                          rtol * cell_size)
+
+                points_close_enough = (diffs_along_axis <= (atol +
+                                        rtol * (lower_bounds - upper_bounds)))


E128 continuation line under-indented for visual indent

corinnebosley · 2018-10-10T11:27:37Z

@pp-mo Still have to remove all references to discontiguity from iris.util.mask_cube. Sorry, forgot...

corinnebosley · 2018-10-10T12:54:39Z

@pp-mo I have now removed all references to discontiguity in iris.util.mask_cube, but I'd like you to check the words in the docstring for me please because I'm not sure if they're quite right.

Thanks!

pp-mo · 2018-10-10T14:49:15Z

iris.util.mask_cube ... I'd like you to check the words in the docstring

Looks fine to me 😃 !

Test discontiguity checking for a y-direction mismatch.

lbdreyer

I've been getting lots of notifications from this PR so I couldn't help but have a very very quick look, as I was interested in what the API would look like.

I have a couple quick comments about doc strings.

stickler-ci reviewed Aug 15, 2018

View reviewed changes

corinnebosley requested review from lbdreyer and pp-mo August 15, 2018 15:48

lbdreyer reviewed Aug 16, 2018

View reviewed changes

stickler-ci reviewed Aug 16, 2018

View reviewed changes

pp-mo reviewed Aug 16, 2018

View reviewed changes

stickler-ci reviewed Aug 20, 2018

View reviewed changes

corinnebosley added the Feature: 2D coords label Aug 22, 2018

lbdreyer previously requested changes Aug 22, 2018

View reviewed changes

stickler-ci reviewed Aug 23, 2018

View reviewed changes

DPeterK added this to the v2.2.0 milestone Oct 3, 2018

DPeterK changed the base branch from 2d_coords to master October 3, 2018 09:49

DPeterK assigned corinnebosley Oct 3, 2018

corinnebosley force-pushed the public_contiguity_checking branch from 582c164 to 29d0e95 Compare October 3, 2018 13:21

pp-mo requested changes Oct 9, 2018

View reviewed changes

suggested changes

e8b8621

stickler-ci reviewed Oct 10, 2018

View reviewed changes

Comment thread lib/iris/coords.py Outdated

corinnebosley added 2 commits October 10, 2018 10:10

sticklerbot correction

ae783c1

fixed some tests after tweaking function output formatÂ

df39a1a

stickler-ci reviewed Oct 10, 2018

View reviewed changes

made syntax consistent for sticklerbot

3a69551

removed all refs to discontiguity in mask_cube

a4321c2

corrected test name

47c3121

corinnebosley and others added 2 commits October 10, 2018 16:06

last tweaks to docstring and stuff

cc517be

Test discontiguity checking for a y-direction mismatch.

23ce813

pp-mo approved these changes Oct 10, 2018

View reviewed changes

Merge pull request #13 from pp-mo/public_contiguity_checking_extra

66f5a07

Test discontiguity checking for a y-direction mismatch.

DPeterK mentioned this pull request Oct 10, 2018

Release v2.2.0 #3193

Closed

13 tasks

lbdreyer reviewed Oct 10, 2018

View reviewed changes

Comment thread lib/iris/util.py

lbdreyer reviewed Oct 10, 2018

View reviewed changes

Comment thread lib/iris/util.py

lbdreyer reviewed Oct 10, 2018

View reviewed changes

pp-mo merged commit 70f57e4 into SciTools:master Oct 11, 2018

pp-mo mentioned this pull request Oct 11, 2018

Revert "Public contiguity checking" #3196

Closed

pp-mo mentioned this pull request Jan 31, 2019

Mis-named routine in docs for iris.util.find_discontiguities #3253

Closed

cpelley mentioned this pull request Jun 28, 2019

BUG: Assumptions made in _discontiguity_in_bounds #3345

Closed

rcomer mentioned this pull request Dec 9, 2020

iris.util.mask_cube wish list #3936

Closed

jonseddon mentioned this pull request Dec 29, 2021

Unusual comparison in coord contiguity check #4473

Closed



		def _check_contiguity_and_bounds(coord, data, abs_tol=1e-4, transpose=False):
		def _check_contiguity_and_mask(coord, data, abs_tol=1e-4, transpose=False):

		return None


		def mask_discontiguities(coord, data, abs_tol=1e-4):

Uh oh!

Conversation

corinnebosley commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corinnebosley commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbdreyer Aug 16, 2018 • 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!

lbdreyer Aug 16, 2018 • 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!

corinnebosley commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corinnebosley Aug 17, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corinnebosley Aug 17, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

corinnebosley commented Aug 22, 2018

Uh oh!

corinnebosley commented Aug 22, 2018

Uh oh!

lbdreyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbdreyer Aug 16, 2018 •

edited

Loading

lbdreyer Aug 16, 2018 •

edited

Loading

corinnebosley Aug 17, 2018 •

edited

Loading

corinnebosley Aug 17, 2018 •

edited

Loading