Merge pull request #2041 from astrofrog/fix-scatter-bugs

Fix several bugs with scatter viewer

CHANGES.md

@@ -6,11 +6,20 @@ v0.16.0 (unreleased)
 
 * No changes yet.
 
+v0.15.5 (unreleased)
+--------------------
+
+* Fixed bug with density map visibility when using color-coding. [#2041]
+
+* Fixed bug with incompatible subsets and density maps. [#2041]
+
+* Make sure that lines/errors/vectors are disabled when in density map mode. [#2041]
+
 v0.15.4 (unreleased)
 --------------------
 
 * Fixed bug that occurred when trying to add a subset to a new table
-  viewer (without the parent data).
+  viewer (without the parent data). [#2038]
 
 v0.15.3 (2019-06-27)
 --------------------

glue/viewers/scatter/layer_artist.py

@@ -157,7 +157,7 @@ def __init__(self, axes, viewer_state, layer_state=None, layer=None):
                                                        vmin=self.density_auto_limits.min,
                                                        vmax=self.density_auto_limits.max,
                                                        update_while_panning=False,
-                                                       histogram2d_func=self.state.compute_density_map)
+                                                       histogram2d_func=self.compute_density_map)
         self.axes.add_artist(self.density_artist)
 
         self.mpl_artists = [self.scatter_artist, self.plot_artist,
@@ -172,6 +172,17 @@ def __init__(self, axes, viewer_state, layer_state=None, layer=None):
         # See also https://github.com/matplotlib/matplotlib/issues/13799
         self._errorbar_keep = None
 
+    def compute_density_map(self, *args, **kwargs):
+        try:
+            density_map = self.state.compute_density_map(*args, **kwargs)
+        except IncompatibleAttribute:
+            self.disable_invalid_attributes(self._viewer_state.x_att,
+                                            self._viewer_state.y_att)
+            return np.array([[np.nan]])
+        else:
+            self.enable()
+        return density_map
+
     @defer_draw
     def _update_data(self):
 
@@ -463,7 +474,15 @@ def _update_visual_attributes(self, changed, force=False):
                 artist.set_zorder(self.state.zorder)
 
             if force or 'visible' in changed:
-                artist.set_visible(self.state.visible)
+                # We need to hide the density artist if it is not needed because
+                # otherwise it might still show even if there is no data as the
+                # neutral/zero color might not be white.
+                if artist is self.density_artist:
+                    artist.set_visible(self.state.visible and
+                                       self.state.density_map and
+                                       self.state.markers_visible)
+                else:
+                    artist.set_visible(self.state.visible)
 
         self.redraw()
 

glue/viewers/scatter/qt/layer_style_editor.py

@@ -45,6 +45,7 @@ def __init__(self, layer, parent=None):
 
         self.layer_state.add_callback('density_map', self._update_size_mode)
         self.layer_state.add_callback('density_map', self._update_warnings)
+        self.layer_state.add_callback('density_map', self._update_checkboxes)
 
         self.layer_state.add_callback('layer', self._update_warnings)
 
@@ -58,6 +59,8 @@ def __init__(self, layer, parent=None):
         self._update_vector_mode()
         self._update_cmap_mode()
 
+        self._update_checkboxes()
+
         self._update_warnings()
 
     def _update_warnings(self, *args):
@@ -137,6 +140,16 @@ def _update_markers_visible(self, *args):
         self.ui.combosel_points_mode.setEnabled(self.layer_state.markers_visible)
         self.ui.value_density_contrast.setEnabled(self.layer_state.markers_visible)
 
+    def _update_checkboxes(self, *args):
+        for checkbox in [self.ui.bool_line_visible, self.ui.bool_xerr_visible,
+                         self.ui.bool_yerr_visible, self.ui.bool_vector_visible]:
+            if self.layer_state.density_map:
+                checkbox.setEnabled(False)
+                checkbox.setToolTip('Not available with density map')
+            else:
+                checkbox.setEnabled(True)
+                checkbox.setToolTip('')
+
     def _update_line_visible(self, *args):
         self.ui.value_linewidth.setEnabled(self.layer_state.line_visible)
         self.ui.combosel_linestyle.setEnabled(self.layer_state.line_visible)

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

@@ -598,3 +598,48 @@ def test_datetime64_support(self, tmpdir):
         assert options.valuetext_y_max.text() == '3.512'
 
         ga.close()
+
+    def test_density_map_incompatible_subset(self, capsys):
+
+        # Regression test for a bug that caused the scatter viewer to crash
+        # if subset for density map was incompatible.
+
+        data2 = Data(label='d1', x=[3.4, 2.3, -1.1, 0.3], y=[3.2, 3.3, 3.4, 3.5], z=['a', 'b', 'c', 'a'])
+
+        self.data_collection.append(data2)
+
+        self.viewer.add_data(self.data)
+        self.viewer.add_data(data2)
+
+        self.data_collection.new_subset_group('test', self.data.id['x'] > 1)
+
+        for layer in self.viewer.state.layers:
+            layer.density_map = True
+
+        self.viewer.figure.canvas.draw()
+        process_events()
+
+        assert self.viewer.layers[0].enabled
+        assert not self.viewer.layers[1].enabled
+        assert self.viewer.layers[2].enabled
+        assert not self.viewer.layers[3].enabled
+
+    def test_density_map_line_error_vector(self, capsys):
+
+        # Make sure that we don't allow/show lines/errors/vectors
+        # if in density map mode.
+
+        self.viewer.add_data(self.data)
+
+        self.viewer.state.layers[0].line_visible = True
+        self.viewer.state.layers[0].xerr_visible = True
+        self.viewer.state.layers[0].yerr_visible = True
+        self.viewer.state.layers[0].vector_visible = True
+
+        # Setting density_map to True resets the visibility of
+        # lines/errors/vectors.
+        self.viewer.state.layers[0].density_map = True
+        assert not self.viewer.state.layers[0].line_visible
+        assert not self.viewer.state.layers[0].xerr_visible
+        assert not self.viewer.state.layers[0].yerr_visible
+        assert not self.viewer.state.layers[0].vector_visible

glue/viewers/scatter/qt/tests/test_python_export.py

@@ -177,3 +177,13 @@ def test_cmap_mode_change(self, tmpdir):
         self.viewer.state.layers[0].cmap_mode = 'Linear'
         self.viewer.state.layers[0].cmap_mode = 'Fixed'
         self.assert_same(tmpdir)
+
+    def test_density_map_change(self, tmpdir):
+        # Regression test for a bug that caused the density map to still
+        # be visible if using color-coding with the density map then
+        # switching to markers.
+        self.viewer.state.layers[0].density_map = True
+        self.viewer.state.layers[0].cmap_mode = 'Linear'
+        self.viewer.state.layers[0].cmap = plt.cm.BuGn
+        self.viewer.state.layers[0].density_map = False
+        self.assert_same(tmpdir)

glue/viewers/scatter/state.py

@@ -263,6 +263,7 @@ def __init__(self, viewer_state=None, layer=None, **kwargs):
         ScatterLayerState.points_mode.set_display_func(self, points_mode_display.get)
 
         self.add_callback('points_mode', self._update_density_map_mode)
+        self.add_callback('density_map', self._on_density_map_change, priority=10000)
 
         ScatterLayerState.cmap_mode.set_choices(self, ['Fixed', 'Linear'])
         ScatterLayerState.size_mode.set_choices(self, ['Fixed', 'Linear'])
@@ -342,6 +343,21 @@ def _update_density_map_mode(self, *args):
         else:
             self.density_map = False
 
+    def _on_density_map_change(self, *args):
+        # If the density map mode is used, we should disable the lines/errors/vectors
+        if self.density_map:
+            with delay_callback(self,
+                                'line_visible', 'xerr_visible',
+                                'yerr_visible', 'vector_visible'):
+                if self.line_visible:
+                    self.line_visible = False
+                if self.xerr_visible:
+                    self.xerr_visible = False
+                if self.yerr_visible:
+                    self.yerr_visible = False
+                if self.vector_visible:
+                    self.vector_visible = False
+
     def flip_cmap(self):
         """
         Flip the cmap_vmin/cmap_vmax limits.