Fixes to _discontiguity_in_bounds (attempt 2)

[stephenworsley]

🚀 Pull Request

Description

Addresses #3401 and #3480, replaces #3404 which I couldn't reopen since it's targeting a deleted branch.

This adresses three problems at once:

• 2D discontiguity checking wasn't comparing enough bounds.
• The logic for determining discontiguity was somewhat garbled, I couldn't figure out exactly what it was trying to achieve so I replaced it with np.isclose like the np.allclose in 1D checking.
• It was unclear how the returned diffs ought to be represented as this used to be a boolean array for 2D coords and floats for 1D coords. Since this function is private, this is effectively a case of checking how these values are used. I believe that in each case they are used, they are being treated as an array of booleans so I have changed the behaviour of 1D coord checking and the description of the function to match this.

---

Consult Iris pull request check list

[rcomer]

I haven't looked closely, but I think this would also address #4473?

[pp-mo]

After taking simply ages to understand what has changed here and why, I think I finally grasped it !
So, iris.util.find_discontiguity now finds an extra cell to be invalid in each testcase from iris.tests.unit.util.test_find_discontiguities, and the make_bounds_discontiguous_at_point test routine now encodes this too : although it makes the same coord adjustments as before, the resulting points which will be judged 'discontiguous' as a result have changed.

I have confirmed that the changes to the data mask in make_bounds_discontiguous_at_point correspond to those locations that now fail according to the utility routine find_discontiguities : Thefind_discontiguities code has not changed, but it relies on Coord._discontiguity_in_bounds, which of course has.

Most importantly, I have also confirmed that the new results from iris.util.find_discontiguities continue to perform as expected : The points which it marks are exactly those which are required to be masked to make iris.plot.pcolormesh succeed for the cubes with bounds discontiguitites generated by the iris.tests.unit.util.test_find_discontiguities tests.

So in effect, the results from iris.util.find_discontiguities have changed. But (a) the test code has been changed to match, and (b) the result still matches the requirement for "making mesh plots work".

I hope you agree with this analysis -- it has taken me a long time to convince myself that this is all correct + OK !

I think we can get away with this change, even though it breaks previous behaviour.
What is still missing is a whatsnew -- it should make clear that the results of "Coord.is_contiguous" and "util.find_discontiguity" have changed, but that we consider this to be a bugfix rather than a "breaking change".

I also have a slight qualm that the existing make_bounds_discontiguous_at_point routine always modifies two corners at once.
I think that may have made sense before, when we only compared bounds 0+1' in X and 3+0' in Y, but now it would make more sense to have independent testcases that check the "new comparisons" -- i.e. 2+3' in X and 2+1' in Y.
So, I suggest you should enhance iris.tests.stock._stock_2d_latlons.make_bounds_discontiguous_at_point to do these, and add suitable testcases to the existing iris.tests.unit.util.test_find_discontiguities.Test.test_find_discontiguities and iris.tests.unit.util.test_find_discontiguities.Test.test_find_discontiguities_in_y.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (333a3a9) 89.27% compared to head (19fa7dc) 89.26%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4975      +/-   ##
==========================================
- Coverage   89.27%   89.26%   -0.01%     
==========================================
  Files          88       88              
  Lines       22261    22264       +3     
  Branches     4873     4874       +1     
==========================================
+ Hits        19874    19875       +1     
- Misses       1641     1642       +1     
- Partials      746      747       +1     

Impacted Files	Coverage Δ
lib/iris/util.py	89.79% <ø> (-0.35%)	⬇️
lib/iris/coords.py	93.18% <100.00%> (-0.02%)	⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pp-mo]

i think there are still a few more minor changes to make.
the only really "wrong" thing is the docstring, but I think it does need fixing.

[pp-mo]

(sorry, additional point just spotted after I submitted the previous review)