Spatial-Spectral highlighting (#1528)

* extend ShadowMixin to shadow multiple marks simultaneously
* implement ShadowSpatialSpectral mark which shadows the spectrum of a spatial subset but the mask and styling of a spectral subset
* automatically create/delete ShadowSpatialSpectral marks
* generalize code with viewer property to access custom marks
* improve test coverage for spatial-spectral marks

CHANGES.rst

@@ -79,6 +79,8 @@ Cubeviz
 
 - Increased spectral slider performance considerably. [#1550]
 
+- Fixed the spectral subset highlighting of spatial subsets in the profile viewer. [#1528]
+
 Imviz
 ^^^^^
 

jdaviz/configs/cubeviz/helper.py

@@ -80,9 +80,7 @@ def select_wavelength(self, wavelength):
         if not isinstance(wavelength, (int, float)):
             raise TypeError("wavelength must be a float or int")
         # Retrieve the x slices from the spectrum viewer's marks
-        x_all = [m for m in self.app.get_viewer('spectrum-viewer').figure.marks
-                 if m.__class__.__name__ in ['Lines', 'LinesGL']
-                 ][0].x
+        x_all = self.app.get_viewer('spectrum-viewer').native_marks[0].x
         index = np.argmin(abs(x_all - wavelength))
         return self.select_slice(int(index))
 

jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py

@@ -2,7 +2,7 @@
 
 
 @pytest.mark.filterwarnings('ignore:No observer defined on WCS')
-def test_moment_calculation(cubeviz_helper, spectrum1d_cube, tmpdir):
+def test_data_selection(cubeviz_helper, spectrum1d_cube, tmpdir):
     app = cubeviz_helper.app
     # NOTE: these are the same underlying data.  This works fine for the current scope
     # of the tests (to make sure checking/unchecking operations change the data exposed

jdaviz/configs/cubeviz/plugins/viewers.py

@@ -1,9 +1,10 @@
 import numpy as np
 from glue.core import BaseData
+from glue.core.subset import RoiSubsetState, RangeSubsetState
 from glue_jupyter.bqplot.image import BqplotImageView
 
 from jdaviz.core.registries import viewer_registry
-from jdaviz.core.marks import SliceIndicatorMarks
+from jdaviz.core.marks import SliceIndicatorMarks, ShadowSpatialSpectral
 from jdaviz.configs.default.plugins.viewers import JdavizViewerMixin
 from jdaviz.configs.cubeviz.helper import layer_is_cube_image_data
 from jdaviz.configs.imviz.helper import data_has_valid_wcs
@@ -197,6 +198,63 @@ def __init__(self, *args, **kwargs):
         # default_tool_priority=['jdaviz:selectslice']
         super().__init__(*args, **kwargs)
 
+    def _get_spatial_subset_layers(self):
+        return [ls for ls in self.state.layers
+                if isinstance(getattr(ls.layer, 'subset_state', None), RoiSubsetState)]
+
+    def _get_spectral_subset_layers(self):
+        return [ls for ls in self.state.layers
+                if isinstance(getattr(ls.layer, 'subset_state', None), RangeSubsetState)]
+
+    def _get_marks_for_layers(self, layers):
+        layers_list = list(self.state.layers)
+        # here we'll assume that all custom marks are subclasses of Lines/GL but don't directly
+        # use Lines/LinesGL (so an isinstance check won't be sufficient here)
+        layer_marks = self.native_marks
+        # and now we'll assume that the marks are in the same order as the layers, this should
+        # be the case as long as the order isn't manually resorted
+        return [layer_marks[layers_list.index(layer)] for layer in layers]
+
+    def _on_subset_delete(self, msg):
+        # delete any ShadowSpatialSpectral mark for which either of the spectral or spatial marks
+        # no longer exists
+        spectral_marks = self._get_marks_for_layers(self._get_spectral_subset_layers())
+        spatial_marks = self._get_marks_for_layers(self._get_spatial_subset_layers())
+        self.figure.marks = [m for m in self.figure.marks
+                             if not (isinstance(m, ShadowSpatialSpectral) and
+                                     (m.spatial_spectrum_mark in spatial_marks or
+                                      m.spectral_subset_mark in spectral_marks))]
+
+    def _expected_subset_layer_default(self, layer_state):
+        """
+        This gets called whenever the layer of an expected new subset is added, we want to set the
+        default for the linewidth depending on whether it is spatial or spectral, and handle
+        creating any necessary marks for spatial-spectral subset intersections.
+        """
+        super()._expected_subset_layer_default(layer_state)
+
+        this_mark = self._get_marks_for_layers([layer_state])[0]
+        new_marks = []
+
+        if isinstance(layer_state.layer.subset_state, RoiSubsetState):
+            layer_state.linewidth = 1
+
+            # need to add marks for every intersection between THIS spatial subset and ALL spectral
+            spectral_marks = self._get_marks_for_layers(self._get_spectral_subset_layers())
+            for spectral_mark in spectral_marks:
+                new_marks += [ShadowSpatialSpectral(this_mark, spectral_mark)]
+
+        else:
+            layer_state.linewidth = 3
+
+            # need to add marks for every intersection between THIS spectral subset and ALL spatial
+            spatial_marks = self._get_marks_for_layers(self._get_spatial_subset_layers())
+            for spatial_mark in spatial_marks:
+                new_marks += [ShadowSpatialSpectral(spatial_mark, this_mark)]
+
+        # NOTE: += or append won't pick up on change
+        self.figure.marks = self.figure.marks + new_marks
+
     @property
     def slice_indicator(self):
         for mark in self.figure.marks:

jdaviz/configs/default/plugins/viewers.py

@@ -25,6 +25,20 @@ def __init__(self, *args, **kwargs):
         # NOTE: anything here most likely won't be called by viewers because of inheritance order
         super().__init__(*args, **kwargs)
 
+    @property
+    def native_marks(self):
+        """
+        Return all marks that are Lines/LinesGL objects (and not subclasses)
+        """
+        return [m for m in self.figure.marks if m.__class__.__name__ in ['Lines', 'LinesGL']]
+
+    @property
+    def custom_marks(self):
+        """
+        Return all marks that are not Lines/LinesGL objects (but can be subclasses)
+        """
+        return [m for m in self.figure.marks if m.__class__.__name__ not in ['Lines', 'LinesGL']]
+
     def _subscribe_to_layers_update(self):
         # subscribe to new layers
         self._expected_subset_layers = []

jdaviz/configs/specviz/plugins/viewers.py

@@ -2,7 +2,7 @@
 import warnings
 
 from glue.core import BaseData
-from glue.core.subset import Subset, RoiSubsetState
+from glue.core.subset import Subset
 from glue.config import data_translator
 from glue_jupyter.bqplot.profile import BqplotProfileView
 from glue.core.exceptions import IncompatibleAttribute
@@ -64,12 +64,10 @@ def __init__(self, *args, **kwargs):
         # Change collapse function to sum
         self.state.function = 'sum'
 
-    def _expected_subset_layer_default(self, layer):
-        super()._expected_subset_layer_default(layer)
-        if isinstance(layer.layer.subset_state, RoiSubsetState):
-            layer.linewidth = 1
-        else:
-            layer.linewidth = 3
+    def _expected_subset_layer_default(self, layer_state):
+        super()._expected_subset_layer_default(layer_state)
+
+        layer_state.linewidth = 3
 
     def data(self, cls=None):
         # Grab the user's chosen statistic for collapsing data

jdaviz/core/helpers.py

@@ -12,7 +12,7 @@
 import numpy as np
 import astropy.units as u
 from glue.core import HubListener
-from glue.core.message import SubsetCreateMessage
+from glue.core.message import SubsetCreateMessage, SubsetDeleteMessage
 
 from jdaviz.app import Application
 
@@ -59,6 +59,8 @@ def __init__(self, app=None, verbosity='warning', history_verbosity='info'):
 
         self.app.hub.subscribe(self, SubsetCreateMessage,
                                handler=lambda msg: self._propagate_callback_to_viewers('_on_subset_create', msg)) # noqa
+        self.app.hub.subscribe(self, SubsetDeleteMessage,
+                               handler=lambda msg: self._propagate_callback_to_viewers('_on_subset_delete', msg)) # noqa
 
     def _propagate_callback_to_viewers(self, method, msg):
         # viewers don't have access to the app/hub to subscribe to messages, so we'll

jdaviz/core/marks.py

@@ -3,6 +3,7 @@
 from astropy import units as u
 from bqplot import LinearScale
 from bqplot.marks import Lines, Label, Scatter
+from copy import deepcopy
 from glue.core import HubListener
 from specutils import Spectrum1D
 
@@ -326,19 +327,32 @@ class ShadowMixin:
 
     Can manually override ``_on_shadowing_changed`` for more advanced logic cases.
     """
+    def _get_id(self, mark):
+        return getattr(mark, '_model_id', None)
+
     def _setup_shadowing(self, shadowing, sync_traits=[], other_traits=[]):
-        self._shadowing = shadowing
-        self._sync_traits = sync_traits
+        """
+        sync_traits: traits to set now, and mirror any changes to shadowing in the future
+        other_trait: traits to set now, but not mirror in the future
+        """
+        if not hasattr(self, '_shadowing'):
+            self._shadowing = {}
+            self._sync_traits = {}
+        shadowing_id = self._get_id(shadowing)
+        self._shadowing[shadowing_id] = shadowing
+        self._sync_traits[shadowing_id] = sync_traits
 
         # sync initial values
         for attr in sync_traits + other_traits:
-            self._on_shadowing_changed({'name': attr, 'new': getattr(shadowing, attr)})
+            self._on_shadowing_changed({'name': attr,
+                                        'new': getattr(shadowing, attr),
+                                        'owner': shadowing})
 
         # subscribe to future changes
         shadowing.observe(self._on_shadowing_changed)
 
     def _on_shadowing_changed(self, change):
-        if change['name'] in self._sync_traits:
+        if change['name'] in self._sync_traits.get(self._get_id(change.get('owner')), []):
             setattr(self, change['name'], change['new'])
         return
 
@@ -359,13 +373,48 @@ def __init__(self, shadowing, shadow_width=1, **kwargs):
                               ['scales', 'x', 'y', 'visible', 'line_style', 'marker'],
                               ['stroke_width', 'marker_size'])
 
+
+class ShadowSpatialSpectral(Lines, HubListener, ShadowMixin):
+    """
+    Shadow the mark of a spatial subset collapsed spectrum, with the mask from a spectral subset,
+    and the styling from the spatial subset.
+    """
+    def __init__(self, spatial_spectrum_mark, spectral_subset_mark):
+        # spatial_spectrum_mark: Lines mark corresponding to the spatially-collapsed spectrum
+        # from a spatial subset
+        # spectral_subset_mark: Lines mark on the FULL cube corresponding to the glue-highlight
+        # of the spectral subset
+        super().__init__(scales=spatial_spectrum_mark.scales, marker=None)
+
+        self._spatial_mark_id = self._get_id(spatial_spectrum_mark)
+        self._setup_shadowing(spatial_spectrum_mark,
+                              ['scales', 'y', 'visible', 'line_style'],
+                              ['x'])
+
+        self._spectral_mark_id = self._get_id(spectral_subset_mark)
+        self._setup_shadowing(spectral_subset_mark,
+                              ['stroke_width', 'x', 'y', 'visible', 'opacities', 'colors'])
+
+    @property
+    def spatial_spectrum_mark(self):
+        return self._shadowing[self._spatial_mark_id]
+
+    @property
+    def spectral_subset_mark(self):
+        return self._shadowing[self._spectral_mark_id]
+
     def _on_shadowing_changed(self, change):
-        super()._on_shadowing_changed(change)
-        if change['name'] in ['stroke_width', 'marker_size']:
-            # apply the same, but increased by the shadow width
-            setattr(self, change['name'],
-                    change['new'] + self._shadow_width if change['new'] else 0)
-        return
+        if hasattr(self, '_spectral_mark_id'):
+            if change['name'] == 'y':
+                # force a copy or else we'll overwrite the mask to the spatial mark!
+                change['new'] = deepcopy(self.spatial_spectrum_mark.y)
+                change['new'][np.isnan(self.spectral_subset_mark.y)] = np.nan
+
+            elif change['name'] == 'visible':
+                # only show if BOTH shadowing marks are set to visible
+                change['new'] = self.spectral_subset_mark.visible and self.spatial_spectrum_mark.visible  # noqa
+
+        return super()._on_shadowing_changed(change)
 
 
 class ShadowLabelFixedY(Label, ShadowMixin):

jdaviz/tests/test_subsets.py

@@ -8,6 +8,8 @@
 from regions import EllipsePixelRegion, RectanglePixelRegion
 from specutils import SpectralRegion
 
+from jdaviz.core.marks import ShadowSpatialSpectral
+
 
 def test_region_from_subset_2d(cubeviz_helper):
     data = Data(flux=np.ones((128, 128)), label='Test 2D Flux')
@@ -162,13 +164,19 @@ def test_region_spectral_spatial(cubeviz_helper, spectral_cube_wcs):
     cubeviz_helper.app.add_data_to_viewer('spectrum-viewer', 'Test Flux')
     cubeviz_helper.app.add_data_to_viewer('flux-viewer', 'Test Flux')
 
-    cubeviz_helper.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(5, 15.5))
+    spectrum_viewer = cubeviz_helper.app.get_viewer("spectrum-viewer")
+    spectrum_viewer.apply_roi(XRangeROI(5, 15.5))
+
+    # should be no spatial-spectral intersection marks yet
+    assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 0  # noqa
 
     flux_viewer = cubeviz_helper.app.get_viewer("flux-viewer")
     # We set the active tool here to trigger a reset of the Subset state to "Create New"
     flux_viewer.toolbar.active_tool = flux_viewer.toolbar.tools['bqplot:rectangle']
     flux_viewer.apply_roi(RectangularROI(1, 3.5, -0.2, 3.3))
 
+    assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 1  # noqa
+
     subsets = cubeviz_helper.app.get_subsets_from_viewer('spectrum-viewer', subset_type='spectral')
     reg = subsets.get('Subset 1')
 
@@ -188,6 +196,14 @@ def test_region_spectral_spatial(cubeviz_helper, spectral_cube_wcs):
     assert_allclose(reg.width, 2.5)
     assert_allclose(reg.height, 3.5)
 
+    # add another spectral subset to ensure the spatial-spectral intersection marks are created as
+    # expected
+    # reset the tool to force a new selection instead of the default "replace"
+    spectrum_viewer.toolbar.active_tool = spectrum_viewer.toolbar.tools['jdaviz:panzoom']
+    spectrum_viewer.toolbar.active_tool = spectrum_viewer.toolbar.tools['bqplot:xrange']
+    spectrum_viewer.apply_roi(XRangeROI(3, 16))
+    assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 2  # noqa
+
 
 def test_disjoint_spectral_subset(cubeviz_helper, spectral_cube_wcs):
     subset_plugin = cubeviz_helper.app.get_tray_item_from_name('g-subset-plugin')