Coord nan equal by pp-mo · Pull Request #3283 · SciTools/iris

pp-mo · 2019-02-26T15:53:20Z

Following the discovery #3282,
found that saving regridded cubes would fail because when a regridded dependency coordinate (orography in this case), coord contains NaNs, then "coord != coord".

Changes here implement a nan-tolerant comparison, so a coord containing NaNs does compare equal to an identical one (or itself).

Example of resulting save error :

>>> import iris
>>> import iris.tests.stock as istk
>>> cube = istk.realistic_4d()
>>> orog = cube.coord('surface_altitude').points
>>> orog[10,10] = np.nan
>>> cube.coord('surface_altitude').points = orog
>>> iris.save(cube, 'tmp.nc')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/io/__init__.py", line 408, in save
    saver(source, target, **kwargs)
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/fileformats/netcdf.py", line 2352, in save
    fill_value=fill_value)
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/fileformats/netcdf.py", line 1001, in write
    self._add_aux_factories(cube, cf_var_cube, dimension_names)
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/fileformats/netcdf.py", line 1248, in _add_aux_factories
    key, coord in six.iteritems(factory.dependencies)}
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/fileformats/netcdf.py", line 1248, in <dictcomp>
    key, coord in six.iteritems(factory.dependencies)}
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/fileformats/netcdf.py", line 320, in name
    raise KeyError(msg)
KeyError: "Coordinate is not mapped, AuxCoord(array([[413.93686, 417.8168 , ...
 . . .
>>>

lbdreyer · 2019-02-26T16:27:18Z

docs/iris/src/whatsnew/contributions_2.3.0/newfeature_2019-Feb-25_compare_nan.txt

@@ -0,0 +1,2 @@
+* :func:`iris.util.array_equal` now has a 'withnans' keyword, which provides
+  a NaN-tolerant array comparison.


You've targetted this at the v2.2.x branch but included a whats new entry to the 2.3.0 whats new?

Addressed .. ?

pp-mo · 2019-02-26T19:18:15Z

NOTE:

This might call into question whether cube-comparison should also be made nan-tolerant ??

It's harder, as

cube comparison involves a toleranced array difference (though really not a very solid one)
We might further extend array_equal to do toleranced comparison, but that seems over-extension,
so it probably needs to go "somewhere else"

I think it's also less likely to cause internal Iris errors, like this one
- because Iris doesn't deal in collections of cubes very much (but collections of Coords, quite a lot)

So, I'm proposing that we ignore this problem for now.

lbdreyer · 2019-02-28T17:32:48Z

lib/iris/tests/unit/util/test_array_equal.py

@@ -1,4 +1,4 @@
-# (C) British Crown Copyright 2014 - 2015, Met Office
+# (C) British Crown Copyright 2014 - 2019, Met Office


Did you want to follow your suggested approach and remove the year

The template for the new file headers is here:
https://github.com/SciTools/scitools.org.uk/blob/master/documents/scitools_licensing.md#source-preamble

I think I'd rather do it separately.

My reason is, I just realised, we are going to have to modify the licence-header check in test_coding_standards.py first ... see #3285

Yep, that makes sense 👍

lbdreyer · 2019-02-28T17:43:09Z

docs/iris/src/whatsnew/2.2.rst


+* :func:`iris.util.array_equal` now has a 'withnans' keyword, which provides
+  a NaN-tolerant array comparison.
+


I'm torn with this.
Part of me wants it in under a sub-heading, similar to the "Bugs fixed in 1.7.3" sub headings in the 1.7 what's new but I'm not sure...

Hmm. It's awkward because bugfix releases aren't supposed to contain new features !

We could treat the new keyword as a "private matter" for now, and then announce it with enormous fanfare in the nest minor release ??
I think that means putting the whatsnew contribution back into a textfile in a contributions_2.3.0 directory. The actual code changes (+docstring) can stay though,

Would you favour that solution @lbdreyer ?

I believe the outcome of the offline discussion was to keep what you have already done.
Admittedly the reasons for that decision are a little fuzzy we discussed this over a week ago, but I think I personally wanted to not keep it private, but not add confusion by adding a "features added in 2.2.1" section

So basically, I'm happy with this change as it is.

lbdreyer · 2019-03-13T17:07:44Z

@pp-mo It looks like this PR is ready to go!

Do you mind rebasing to resolve the conflict and then it can be merged?

pp-mo · 2019-03-21T18:05:16Z

Rebased !

lbdreyer · 2019-05-10T11:33:29Z

I am going to restarted all the travis ci jobs now that #3311, which fixes the tests, is in.

lbdreyer · 2019-05-10T11:53:25Z

Tests all pass 🎉
I'm going to merge this in (finally!)

rcomer · 2019-05-10T11:56:16Z

@pp-mo

cube comparison involves a toleranced array difference (though really not a very solid one)

Not sure if it's relevant to your point, but the line in question will change if/when #3250 is merged.

* Allow coords with NaNs in to compare equal. * Tests for AuxCoord.__eq__ and util.array_equal(withnans=True). * Move whatsnew contributions into existing whatsnew document. * Better logic in util.array_equal.

pp-mo requested review from bjlittle and lbdreyer February 26, 2019 15:53

lbdreyer reviewed Feb 26, 2019

View reviewed changes

lbdreyer added this to the v2.2.1 milestone Feb 27, 2019

lbdreyer reviewed Feb 28, 2019

View reviewed changes

pp-mo added 4 commits March 21, 2019 17:54

Allow coords with NaNs in to compare equal.

9c2c7c2

Tests for AuxCoord.__eq__ and util.array_equal(withnans=True).

8dc38d0

Move whatsnew contributions into existing whatsnew document.

5db955d

Better logic in util.array_equal.

4e02a0f

pp-mo force-pushed the coord_nan_equal branch from c708742 to 4e02a0f Compare March 21, 2019 18:04

lbdreyer merged commit 09297fe into SciTools:v2.2.x May 10, 2019

pp-mo deleted the coord_nan_equal branch December 18, 2020 19:14

trexfeathers mentioned this pull request Apr 6, 2022

Unexpected behaviour when merging cubelist with constant NaN-valued scalar coord #4681

Open

lbdreyer mentioned this pull request Apr 21, 2022

Change Cell equality to equate nans (fixes #4681) #4701

Merged

		@@ -0,0 +1,2 @@
		* :func:`iris.util.array_equal` now has a 'withnans' keyword, which provides
		a NaN-tolerant array comparison.

		@@ -1,4 +1,4 @@
		# (C) British Crown Copyright 2014 - 2015, Met Office
		# (C) British Crown Copyright 2014 - 2019, Met Office

Conversation

pp-mo commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pp-mo commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Feb 28, 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 Feb 28, 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!

Choose a reason for hiding this comment

Uh oh!

lbdreyer commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pp-mo commented Mar 21, 2019

Uh oh!

lbdreyer commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbdreyer commented May 10, 2019

Uh oh!

rcomer commented May 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pp-mo commented Feb 26, 2019 •

edited

Loading

pp-mo commented Feb 26, 2019 •

edited

Loading

lbdreyer Feb 28, 2019 •

edited

Loading

pp-mo Feb 28, 2019 •

edited

Loading

lbdreyer commented Mar 13, 2019 •

edited

Loading

lbdreyer commented May 10, 2019 •

edited

Loading