Merge pull request #1390 from astrofrog/fix-references-modest-image

Fix crashes in image viewer when removing subsets

CHANGES.md

@@ -4,7 +4,8 @@ Full changelog
 v0.11.1 (unreleased)
 --------------------
 
-* No changes yet
+* Fixed bug that caused ModestImage references to not be properly deleted, in
+  turn leading to issues/crashes when removing subsets from image viewers. [#1390]
 
 v0.11.0 (2017-08-22)
 --------------------

glue/app/qt/application.py

@@ -973,6 +973,9 @@ def dropEvent(self, event):
 
     def closeEvent(self, event):
         """Emit a message to hub before closing."""
+        for tab in self.viewers:
+            for viewer in tab:
+                viewer.close(warn=False)
         self._log.close()
         self._hub.broadcast(ApplicationClosedMessage(None))
         event.accept()

glue/app/qt/tests/test_preferences.py

@@ -377,3 +377,5 @@ def test_foreground_background_settings():
         assert_axes_foreground(histogram2.axes, GREEN)
         assert_axes_foreground(dendrogram2.axes, GREEN)
         assert_axes_foreground(custom2.axes, GREEN)
+
+    app.close()

glue/core/application_base.py

@@ -341,6 +341,9 @@ def __init__(self, session):
         self._hub = None
         self._layer_artist_container = self._layer_artist_container_cls()
 
+    def close(self):
+        self._layer_artist_container.clear()
+
     def register_to_hub(self, hub):
         self._hub = hub
 

glue/core/layer_artist.py

@@ -185,9 +185,19 @@ def update(self, view=None):
 
     @abstractmethod
     def clear(self):
-        """Clear the visulaization for this layer"""
+        """Clear the visualization for this layer"""
         raise NotImplementedError()
 
+    def remove(self):
+        """
+        Remove the visualization for this layer.
+
+        This is called when the layer artist is removed for good from the
+        viewer. It defaults to calling clear, but can be overriden in cases
+        where clear and remove should be different.
+        """
+        self.clear()
+
     def force_update(self, *args, **kwargs):
         """
         Sets the _changed flag to true, and calls update.
@@ -325,20 +335,17 @@ def remove(self, artist):
         :param artist: The artist to remove
         :type artist: :class:`MatplotlibLayerArtist`
         """
-        try:
+        if artist in self.artists:
             self.artists.remove(artist)
-            artist.clear()
-        except ValueError:
-            pass
-
-        self._notify()
+            artist.remove()
+            self._notify()
 
     def clear(self):
         """
         Remove all layer artists from this collection
         """
         for artist in self.artists:
-            artist.clear()
+            artist.remove()
         if six.PY2:
             self.artists[:] = []
         else:

glue/core/qt/layer_artist_model.py

@@ -372,14 +372,12 @@ def append(self, artist):
         self._notify()
 
     def remove(self, artist):
-        try:
+        if artist in self.artists:
             index = self.artists.index(artist)
-        except ValueError:
-            return
-        self.model.removeRow(index)
-        assert artist not in self.artists
-
-        self._notify()
+            self.model.removeRow(index)
+            assert artist not in self.artists
+            artist.remove()
+            self._notify()
 
     def __nonzero__(self):
         return True

glue/external/modest_image.py

@@ -247,7 +247,11 @@ def imshow(axes, X, cmap=None, norm=None, aspect=None,
         im.set_extent(extent)
 
     axes.images.append(im)
-    im._remove_method = lambda h: axes.images.remove(h)
+
+    def remove(h):
+        axes.images.remove(h)
+
+    im._remove_method = remove
 
     return im
 

glue/plugins/tools/pv_slicer/qt/tests/test_pv_slicer.py

@@ -62,6 +62,9 @@ def setup_method(self, method):
         im = np.random.random((3, 3))
         self.w = StandaloneImageViewer(im)
 
+    def teardown_method(self, method):
+        self.w.close()
+
     def test_set_cmap(self):
         cm_mode = self.w.toolbar.tools['image:colormap']
         act = cm_mode.menu_actions()[1]
@@ -92,5 +95,8 @@ def setup_method(self, method):
         self.image = MockImageViewer(self.slc, self.d)
         self.w = PVSliceWidget(image=np.zeros((3, 4)), wcs=None, image_viewer=self.image)
 
+    def teardown_method(self, method):
+        self.w.close()
+
     def test_basic(self):
         pass

glue/plugins/tools/spectrum_tool/qt/tests/test_spectrum_tool.py

@@ -49,6 +49,9 @@ def setup_method(self, method):
         self.tool = self.mode._tool
         self.tool.show = lambda *args: None
 
+    def teardown_method(self, method):
+        self.image.close()
+
 
 class TestSpectrumTool(BaseTestSpectrumTool):
 

glue/tests/test_session_back_compat.py

@@ -101,6 +101,7 @@ def test_load_link_helpers_04():
     state = GlueUnSerializer.loads(content)
 
     ga = state.object('__main__')
+    ga.close()
 
 
 @requires_qt
@@ -145,6 +146,8 @@ def test_load_viewers_04():
     assert s.state.x_log
     assert not s.state.y_log
 
+    ga.close()
+
 
 @requires_qt
 def test_load_pixel_components_07():
@@ -164,6 +167,8 @@ def test_load_pixel_components_07():
     assert isinstance(ga.data_collection[0].pixel_component_ids[0], PixelComponentID)
     assert isinstance(ga.data_collection[0].pixel_component_ids[1], PixelComponentID)
 
+    ga.close()
+
 
 @requires_qt
 def test_table_widget_010():

glue/viewers/common/qt/data_viewer.py

@@ -116,7 +116,8 @@ def mousePressEvent(self, event):
 
     def close(self, warn=True):
         self._warn_close = warn
-        super(DataViewer, self).close()
+        QtWidgets.QMainWindow.close(self)
+        ViewerBase.close(self)
         self._warn_close = True
 
     def mdi_wrap(self):

glue/viewers/common/qt/data_viewer_with_state.py

@@ -62,7 +62,6 @@ def _sync_layer_artist_container(self):
         for layer_artist in self._layer_artist_container:
             if layer_artist.layer not in layer_states:
                 self._layer_artist_container.remove(layer_artist)
-                layer_artist.remove()
 
     def add_data(self, data):
 

glue/viewers/common/qt/tests/test_data_viewer.py

@@ -55,6 +55,7 @@ def test_single_draw_call_on_create(self):
             assert len(set(selfs)) == len(selfs)
         finally:
             MplCanvas.draw = draw
+        app.close()
 
     def test_close_on_last_layer_remove(self):
         # regression test for 391
@@ -69,6 +70,7 @@ def test_close_on_last_layer_remove(self):
             dc.remove(d1)
             dc.remove(d2)
         assert close.call_count >= 1
+        app.close()
 
     def test_viewer_size(self, tmpdir):
 

glue/viewers/image/qt/data_viewer.py

@@ -66,6 +66,12 @@ def __init__(self, session, parent=None, state=None):
                                             origin='lower', interpolation='nearest')
         self._set_wcs()
 
+    def close(self, **kwargs):
+        super(ImageViewer, self).close(**kwargs)
+        if self.axes._composite_image is not None:
+            self.axes._composite_image.remove()
+            self.axes._composite_image = None
+
     def _update_axes(self, *args):
 
         if self.state.x_att_world is not None:

glue/viewers/image/qt/standalone_image_viewer.py

@@ -132,6 +132,9 @@ def mdi_wrap(self):
         return sub
 
     def closeEvent(self, event):
+        if self._im is not None:
+            self._im.remove()
+            self._im = None
         self.window_closed.emit()
         return super(StandaloneImageViewer, self).closeEvent(event)
 

glue/viewers/image/qt/tests/test_data_viewer.py

@@ -3,6 +3,7 @@
 from __future__ import absolute_import, division, print_function
 
 import os
+import gc
 from collections import Counter
 
 import pytest
@@ -12,6 +13,7 @@
 import numpy as np
 from numpy.testing import assert_allclose
 
+from glue.external.modest_image import ModestImage
 from glue.core.coordinates import Coordinates, WCSCoordinates
 from glue.core.message import SubsetUpdateMessage
 from glue.core import HubListener, Data
@@ -38,18 +40,6 @@ def init_data(self):
 
     viewer_cls = ImageViewer
 
-    def setup_method(self, method):
-        # Some of the tests rely on seeing whether the viewer updates if we
-        # change the color of the data, so we temporarily set it so that
-        # global_sync is True in each layer (otherwise changing the color of)
-        # a dataset has no effect on the image viewer specifically. We change
-        # it back in teardown_method.
-        ImageLayerState.global_sync._default = True
-        return super(TestImageCommon, self).setup_method(method)
-
-    def teardown_method(self, method):
-        ImageLayerState.global_sync._default = False
-
     @pytest.mark.skip()
     def test_double_add_ignored(self):
         pass
@@ -484,6 +474,48 @@ def test_scatter_overlay(self):
         self.viewer.add_data(self.image1)
         self.viewer.add_data(self.catalog)
 
+    def test_removed_subset(self):
+
+        # Regression test for a bug in v0.11.0 that meant that if a subset
+        # was removed, the image viewer would then crash when changing view
+        # (e.g. zooming in). The bug was caused by undeleted references to
+        # ModestImage due to circular references. We therefore check in this
+        # test how many ModestImage objects exist.
+
+        def get_modest_images():
+            mi = []
+            gc.collect()
+            for obj in gc.get_objects():
+                try:
+                    if isinstance(obj, ModestImage):
+                        mi.append(obj)
+                except ReferenceError:
+                    pass
+            return mi
+
+        # The viewer starts off with one ModestImage. This is also a good test
+        # that other ModestImages in other tests have been removed.
+        assert len(get_modest_images()) == 1
+
+        large_image = Data(x=np.random.random((2048, 2048)))
+        self.data_collection.append(large_image)
+
+        # The subset group can be made from any dataset
+        subset_group = self.data_collection.new_subset_group(subset_state=self.image1.id['x'] > 1, label='A')
+
+        self.viewer.add_data(large_image)
+
+        # Since the dataset added has a subset, and each subset has its own
+        # ModestImage, this increases the count.
+        assert len(get_modest_images()) == 2
+
+        assert len(self.viewer.layers) == 2
+
+        self.data_collection.remove_subset_group(subset_group)
+
+        # Removing the subset should bring the count back to 1 again
+        assert len(get_modest_images()) == 1
+
 
 class TestSessions(object):
 
@@ -576,6 +608,8 @@ def test_session_back_compat(self, protocol):
         layer_state = viewer3.state.layers[1]
         assert layer_state.visible
 
+        ga.close()
+
     @pytest.mark.parametrize('protocol', [0, 1])
     def test_session_cube_back_compat(self, protocol):
 
@@ -602,6 +636,8 @@ def test_session_cube_back_compat(self, protocol):
         assert viewer1.state.y_att_world is dc[0].id['World 1']
         assert viewer1.state.slices == [2, 0, 0, 1]
 
+        ga.close()
+
     @pytest.mark.parametrize('protocol', [0, 1])
     def test_session_rgb_back_compat(self, protocol):
 
@@ -639,3 +675,5 @@ def test_session_rgb_back_compat(self, protocol):
         assert layer_state.visible
         assert layer_state.attribute.label == 'b'
         assert layer_state.color == 'b'
+
+        ga.close()

glue/viewers/matplotlib/layer_artist.py

@@ -53,6 +53,7 @@ def remove(self):
                 pass
             except AttributeError:  # can happen for Matplotlib 1.4
                 pass
+        self.mpl_artists[:] = []
 
     def get_layer_color(self):
         return self.state.color

glue/viewers/matplotlib/qt/tests/test_data_viewer.py

@@ -451,6 +451,8 @@ def test_session_round_trip(self, tmpdir):
         assert viewer2.layers[0].layer is data2
         assert viewer2.layers[1].layer is data2.subsets[0]
 
+        ga2.close()
+
     def test_apply_roi_undo(self):
 
         self.data_collection.append(self.data)