Perceptual image hashing #2206

bjlittle · 2016-10-24T02:36:20Z

This PR provides a more stable and scalable approach to the new graphic testing framework, replacing cryptographic sha based image hashes with perceptual image hashes.

It relies on the support of the new perceptual image hash repository test-iris-imagehash.

Perceptual image hashes allows checking for the degree of similarity between images through measuring the hamming distance difference (a count of bit differences) between image hashes.

The major benefit of this perceptual image hash approach is that it is simple, scalable (already almost halved the number of required reference images) and makes us less sensitive to scientific software stack dependency changes.

This PR also addresses the issues raised in #2195. Normally, I would have pushed those changes in a separate PR, but there is clearly a major overlap here with regards to graphical testing. In short, the problem simply relates to iris plotting not being thread-safe, and our travis-ci testing uses nose, which highlighted this problem in a typically dreaded, threaded sporadic way.

Closes #2195

bjlittle · 2016-10-24T02:39:03Z

docs/iris/example_tests/extest_util.py

@@ -69,7 +68,7 @@ def show_replaced_by_check_graphic(test_case, tol=_DEFAULT_IMAGE_TOLERANCE):
    """
    def replacement_show():
        # form a closure on test_case and tolerance
-        test_case.check_graphic(tol=tol)
+        test_case.check_graphic()


In the hope of refraining from a culture of tweaking graphical testing tolerances, I've closed the door to passing a tol through to check_graphic.

bjlittle · 2016-10-24T02:40:51Z

docs/iris/example_tests/test_atlantic_profiles.py

@@ -33,7 +33,7 @@ def test_atlantic_profiles(self):
        with fail_any_deprecation_warnings():
            with add_examples_to_path():
                import atlantic_profiles
-            with show_replaced_by_check_graphic(self, tol=14.0):


This covers up an issue in the use of twiny ... changing the tolerance of an individual test is not healthy.

bjlittle · 2016-10-24T02:42:13Z

lib/iris/plot.py

+            result = func(*args, **kwargs)
+        return result
+    return decorated_func
+


A simple decorator to facilitate the use of a re-entrant lock to provide thread-safe plotting. A re-entrant lock is required here as iris.plot.contourf calls iris.plot.contour ...

bjlittle · 2016-10-24T02:42:49Z

lib/iris/plot.py

@@ -665,6 +686,7 @@ def _map_common(draw_method_name, arg_func, mode, cube, plot_defn,
    return plotfn(*new_args, **kwargs)


+@_locker
 def contour(cube, *args, **kwargs):


Use the lock for all public api plotting functions ...

I wonder whether this will have any unexpected behavioural impacts in normal usage of iris.plot? I must admit that I expected that the lock would be applied within the scope of the tests only.

I'm inclined to agree... it feels like this should be done when testing. We are mostly wrapping matplotlib calls, which I guess aren't thread safe either, so it doesn't seem consistent to lock our wrappers.

@dkillick @ajdawson I see your point, and I was in two minds whether to lock at this level. To be honest, I expected that discussion to have happen during the review 😉 ...

Regardless of testing, iris plotting is not thread safe for users thanks to mpl, hence why I locked the wrappers at the plotting level. The problem lies deep within iris plotting regarding calls to iris.plot._replace_axes_with_cartopy_axes

Locking at the graphics test level completely makes sense as it totally ensures that graphics tests are atomic (as I've already implemented), so at a minimum we definitely require to keep that level of locking. The plotting level of locking may help the user in a threaded context, but I'm less convinced of this ... So we may be able to compromise and drop the plotting level of locking ... I'll take a look and see how it behaves. But I'd be more than happy to do that.

bjlittle · 2016-10-24T02:44:40Z

lib/iris/tests/__init__.py

@@ -143,7 +147,8 @@
    plt.switch_backend('tkagg')
    _DISPLAY_FIGURES = True

-_DEFAULT_IMAGE_TOLERANCE = 10.0
+# Threading non re-entrant blocking lock to ensure thread-safe plotting.
+_lock = threading.Lock()


A non re-entrant lock is required here to ensure that there is no cross-pollination between graphic tests using the wrong plot figures or axes.

bjlittle · 2016-10-24T02:48:30Z

lib/iris/tests/__init__.py

-                with open(result_fname, 'rb') as fi:
-                    sha1 = hashlib.sha1(fi.read())
-                return sha1
-


This hot-fix didn't work and needs to be purged.

At this point the plot was already wrong i.e. another thread had already clobbered either the plot title or axes title/s, so re-saving the clobbered plot image was never going to yield the correct sha.

See iris.plot._replace_axes_with_cartopy_axes.

bjlittle · 2016-10-24T02:50:02Z

lib/iris/tests/__init__.py

+                    h = hexstr[i*2:i*2+2]
+                    v = int("0x" + h, 16)
+                    l.append([v & 2**i > 0 for i in range(8)])
+                return imagehash.ImageHash(np.array(l))


This fix has already been merged to imagehash but the fix has not yet been pushed to pypi ... we can remove this function when the fix is available via pypi .... or even conda-forge!

bjlittle · 2016-10-24T02:51:44Z

lib/iris/tests/__init__.py

+            buffer = io.BytesIO()
+            figure = plt.gcf()
+            figure.savefig(buffer, format='png')
+            buffer.seek(0)


Memory buffers are our friend ... only save to disk on failure, rather than save to disk and delete on success.

bjlittle · 2016-10-24T02:52:39Z

lib/iris/tests/__init__.py


-                if sha1.hexdigest() not in expected:
+                if np.all([hd > tol for hd in distances]):


The result image fails to meet expectation iff it is not similar to all our registered expected images.

bjlittle · 2016-10-24T02:54:19Z

lib/iris/tests/experimental/test_animate.py

@@ -40,6 +40,7 @@
 @tests.skip_plot
 class IntegrationTest(tests.GraphicsTest):
    def setUp(self):
+        super(IntegrationTest, self).setUp()


We need to make sure that the GraphicsTest.setUp is called, as it aquires the non re-entrant lock that protects plot figures for a test.

@bjlittle do we need to follow this pattern for all instances of test classes inheriting from tests.GraphicsTest? If so, you'll also need to update the following:

In lib/iris/tests:

test_analysis.TestRotatedPole

test_coordsystem.Test_LambertConformal

test_mapping.TestBasic

In testPlot: Test1dPlotMultiArgs, TestMissingCoord, TestMissingCS, TestAttributePositive, TestPlotOtherCoordSystems

All affected test classes in lib/iris/tests/integration/test_grib_load.py

Test class in lib/iris/tests/integration/test_netcdftime.py

@dkillick Thanks. Yes, we do indeed need to follow a pattern here ...

A GraphicsTest subclass must call the GraphicsTest.setUp method (in order to acquire the non-reentrant lock) if the subclass in question overrides the inherited setUp method. If the subclass doesn't specialise the setUp method then, thanks to inheritance, the right thing happens, in that the unittest framework calls the (inherited) GraphicsTest.setUp for the subclass (this behaviour also applies to GraphicsTest.tearDown btw to release the lock).

In all of the test cases that you identified above, there is no need to explicitly call GraphicsTest.setUp, simply because the subclass does not specialise the setUp method.

Note that, you incorrectly mentioned test_mapping.TestBasic:

class TestBasic(tests.GraphicsTest): def setUp(self): super(TestBasic, self).setUp() self.cube = iris.tests.stock.realistic_4d()

which does apply the pattern, and so does test_plot.Test1dPlotMultiArgs:

class Test1dPlotMultiArgs(tests.GraphicsTest): # tests for iris.plot using multi-argument calling convention def setUp(self): super(Test1dPlotMultiArgs, self).setUp() self.cube1d = _load_4d_testcube()[0, :, 0, 0] self.draw_method = iplt.plot

So, I'd argue that this is a non-issue, unless I'm missing something ...

I'd argue that this is a non-issue

Agreed. I was sure there must be a pattern to it, but I couldn't work out what the pattern was! Good ol' inheritance 😉

bjlittle · 2016-10-24T02:54:49Z

lib/iris/tests/idiff.py

@@ -29,7 +29,6 @@
 import codecs
 import contextlib
 from glob import glob
-import hashlib


hashlib is dead ...

bjlittle · 2016-10-24T02:55:16Z

lib/iris/tests/idiff.py

@@ -38,10 +37,12 @@

 from PIL import Image
 import filelock
+import imagehash


... long live imagehash! 😉

bjlittle · 2016-10-24T02:55:38Z

lib/iris/tests/idiff.py

+        h = hexstr[i*2:i*2+2]
+        v = int("0x" + h, 16)
+        l.append([v & 2**i > 0 for i in range(8)])
+    return imagehash.ImageHash(np.array(l))


Again this will disappear ...

bjlittle · 2016-10-24T02:57:48Z

lib/iris/tests/runner/_runner.py


-    for expected, actual, diff in _failed_images_iter():
+    for expected, actual, diff in step_over_diffs(rdir, 'similar', False):


This fixes the failed test runner --print-failed-images capability, so that we can see graphical failures that occurred remotely on travis again 😄

bjlittle · 2016-10-24T03:01:33Z

Whoop! All the tests passed first time! 🍻

marqh · 2016-10-24T08:46:13Z

Whoop! All the tests passed first time! 🍻

🍻 indeed

marqh clinks glasses with @bjlittle

DPeterK

It's possible this PR was merged a little hastily. Here are my extra review comments, which I'll reference in a new issue to increase the chance they're acted upon.

DPeterK · 2016-10-24T09:26:16Z

.travis.yml

@@ -60,6 +60,10 @@ install:
      fi
    fi

+  # Perceptual image hashing (TBD: push recipe to conda-forge!)
+  - conda install pip


FWIW you don't need to install pip - it's already available on Travis.

As part of the conda environment (and installing to it), though?

pip is installed by default when you create a new conda environment (e.g. https://travis-ci.org/SciTools/iris/jobs/170017955#L253) so it doesn't need to be done separately. The result of this line is a no-op (https://travis-ci.org/SciTools/iris/jobs/170017955#L464).

DPeterK · 2016-10-24T09:29:21Z

lib/iris/plot.py

@@ -665,6 +686,7 @@ def _map_common(draw_method_name, arg_func, mode, cube, plot_defn,
    return plotfn(*new_args, **kwargs)


+@_locker
 def contour(cube, *args, **kwargs):


I wonder whether this will have any unexpected behavioural impacts in normal usage of iris.plot? I must admit that I expected that the lock would be applied within the scope of the tests only.

DPeterK · 2016-10-24T10:38:42Z

lib/iris/tests/experimental/test_animate.py

@@ -40,6 +40,7 @@
 @tests.skip_plot
 class IntegrationTest(tests.GraphicsTest):
    def setUp(self):
+        super(IntegrationTest, self).setUp()


@bjlittle do we need to follow this pattern for all instances of test classes inheriting from tests.GraphicsTest? If so, you'll also need to update the following:

In lib/iris/tests:

test_analysis.TestRotatedPole

test_coordsystem.Test_LambertConformal

test_mapping.TestBasic

In testPlot: Test1dPlotMultiArgs, TestMissingCoord, TestMissingCS, TestAttributePositive, TestPlotOtherCoordSystems

All affected test classes in lib/iris/tests/integration/test_grib_load.py

Test class in lib/iris/tests/integration/test_netcdftime.py

bjlittle added 2 commits October 24, 2016 03:02

Perceptual image hashing.

eb4581e

Ensure thread-safe plotting.

3bc2a1f

bjlittle added the Status: Work in Progress label Oct 24, 2016

bjlittle assigned marqh Oct 24, 2016

bjlittle added this to the v1.11 milestone Oct 24, 2016

bjlittle commented Oct 24, 2016

View reviewed changes

marqh merged commit d52aaec into SciTools:master Oct 24, 2016

marqh removed the Status: Work in Progress label Oct 24, 2016

DPeterK reviewed Oct 24, 2016

View reviewed changes

DPeterK mentioned this pull request Oct 24, 2016

Outstanding review comments from #2206 #2210

Closed

This was referenced Oct 26, 2016

Remove plot locking. #2217

Closed

Remove plot api locking. #2218

Merged

bjlittle deleted the perceptual-image-hashing branch October 29, 2019 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perceptual image hashing #2206

Perceptual image hashing #2206

bjlittle commented Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016

DPeterK Oct 24, 2016

ajdawson Oct 24, 2016 •

edited

Loading

bjlittle Oct 26, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016

bjlittle Oct 24, 2016

bjlittle Oct 24, 2016

bjlittle Oct 24, 2016

DPeterK Oct 24, 2016

bjlittle Oct 26, 2016 •

edited

Loading

DPeterK Oct 31, 2016

bjlittle Oct 24, 2016

bjlittle Oct 24, 2016

bjlittle Oct 24, 2016

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle commented Oct 24, 2016

marqh commented Oct 24, 2016

DPeterK left a comment

DPeterK Oct 24, 2016

QuLogic Oct 24, 2016

ajdawson Oct 24, 2016

bjlittle Oct 26, 2016

DPeterK Oct 24, 2016

DPeterK Oct 24, 2016


		if sha1.hexdigest() not in expected:
		if np.all([hd > tol for hd in distances]):


		for expected, actual, diff in _failed_images_iter():
		for expected, actual, diff in step_over_diffs(rdir, 'similar', False):

Perceptual image hashing #2206

Perceptual image hashing #2206

Conversation

bjlittle commented Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

bjlittle Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

bjlittle Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajdawson Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

bjlittle Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

bjlittle Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

bjlittle Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjlittle Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjlittle Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

bjlittle commented Oct 24, 2016

marqh commented Oct 24, 2016

DPeterK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjlittle commented Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading

ajdawson Oct 24, 2016 •

edited

Loading

bjlittle Oct 26, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading

bjlittle Oct 26, 2016 •

edited

Loading

bjlittle Oct 24, 2016 •

edited

Loading