Finished markers MVP

but still need tests. [ci skip]

jdaviz/configs/imviz/helper.py

@@ -28,6 +28,8 @@ def __init__(self, *args, **kwargs):
         # Markers
         self._marktags = set()
         self._default_mark_tag_name = 'default-marker-name'
+        # TODO: {'marker': 's', 'markersize': 10} does not work. How to fix?
+        self.marker = {'color': 'red', 'alpha': 1.0}
 
     def load_data(self, data, parser_reference=None, **kwargs):
         """Load data into Imviz.
@@ -117,7 +119,7 @@ def center_on(self, point):
         if isinstance(point, SkyCoord):
             i_top = get_top_layer_index(viewer)
             image = viewer.layers[i_top].layer
-            if hasattr(image, 'coords') and isinstance(image.coords, BaseHighLevelWCS):
+            if data_has_valid_wcs(image):
                 try:
                     pix = image.coords.world_to_pixel(point)  # 0-indexed X, Y
                 except NoConvergence as e:  # pragma: no cover
@@ -132,7 +134,7 @@ def center_on(self, point):
 
         # Disallow centering outside of display.
         if (pix[0] < viewer.state.x_min or pix[0] > viewer.state.x_max
-                or pix[1] < viewer.state.y_min or pix[1] > viewer.state.y_max):
+                or pix[1] < viewer.state.y_min or pix[1] > viewer.state.y_max):  # pragma: no cover
             self.app.hub.broadcast(SnackbarMessage(
                 f'{pix} is out of bounds', color="warning", sender=self.app))
             return
@@ -171,7 +173,7 @@ def offset_to(self, dx, dy, skycoord_offset=False):
         if skycoord_offset:
             i_top = get_top_layer_index(viewer)
             image = viewer.layers[i_top].layer
-            if hasattr(image, 'coords') and isinstance(image.coords, BaseHighLevelWCS):
+            if data_has_valid_wcs(image):
                 # To avoid distortion headache, assume offset is relative to
                 # displayed center.
                 x_cen = viewer.state.x_min + (width * 0.5)
@@ -193,6 +195,28 @@ def offset_to(self, dx, dy, skycoord_offset=False):
                 viewer.state.x_max = viewer.state.x_min + width
                 viewer.state.y_max = viewer.state.y_min + height
 
+    @property
+    def marker(self):
+        """Marker to use.
+
+        Marker can be set as follows; e.g.::
+
+            {'color': 'red', 'alpha': 1.0}
+            {'color': '#ff0000', 'alpha': 0.5}
+            {'color': (1, 0, 0), 'alpha': 0.75}
+            {'color': '0.0', 'alpha': 0.1}
+
+        Also see: https://docs.glueviz.org/en/stable/api/glue.core.visual.VisualAttributes.html
+
+        """
+        return self._marker_dict
+
+    @marker.setter
+    def marker(self, val):
+        # TODO: Add glupyter specific validation.
+        # Only set this once we have successfully created a marker
+        self._marker_dict = val
+
     def _validate_marker_name(self, marker_name):
         """Raise an error if the marker_name is not allowed."""
         if marker_name in RESERVED_MARKER_SET_NAMES:
@@ -203,7 +227,8 @@ def _validate_marker_name(self, marker_name):
     def add_markers(self, table, x_colname='x', y_colname='y',
                     skycoord_colname='coord', use_skycoord=False,
                     marker_name=None):
-        """Creates markers in the image at given points.
+        """Creates markers w.r.t. the reference image at given points
+        in the table.
 
         Parameters
         ----------
@@ -215,7 +240,7 @@ def add_markers(self, table, x_colname='x', y_colname='y',
             Coordinates must be 0-indexed.
 
         skycoord_colname : str
-            Column name with ``SkyCoord`` objects.
+            Column name with `~astropy.coordinates.SkyCoord` objects.
 
         use_skycoord : bool
             If `True`, use ``skycoord_colname`` to mark.
@@ -225,27 +250,82 @@ def add_markers(self, table, x_colname='x', y_colname='y',
             Name to assign the markers in the table. Providing a name
             allows markers to be removed by name at a later time.
 
+        Raises
+        ------
+        AttributeError
+            Sky coordinates are given but reference image does not have a valid WCS.
+
+        ValueError
+            Invalid marker name.
+
         """
         if marker_name is None:
             marker_name = self._default_mark_tag_name
 
         self._validate_marker_name(marker_name)
-        self._marktags.add(marker_name)
         viewer = self.app.get_viewer("viewer-1")
+        jglue = self.app.session.application
 
-        # TODO: Is this really the correct Data to use?
+        # TODO: How to link to invidual images separately? add_link in loop does not work.
+        # Link markers to reference image data.
         image = viewer.state.reference_data
 
+        # TODO: Is Glue smart enough to no-op if link already there?
         if use_skycoord:
-            raise NotImplementedError
+            if not data_has_valid_wcs(image):
+                raise AttributeError(f'{image.label} does not have a valid WCS')
+            sky = table[skycoord_colname]
+            t_glue = Data(marker_name, ra=sky.ra.deg, dec=sky.dec.deg)
+            jglue.data_collection[marker_name] = t_glue
+            jglue.add_link(t_glue, 'ra', image, 'Right Ascension')
+            jglue.add_link(t_glue, 'dec', image, 'Declination')
         else:
-            # TODO: Doing it this way is not compatible with blink
             t_glue = Data(marker_name, **table[x_colname, y_colname])
-            jglue = self.app.session.application
             jglue.data_collection[marker_name] = t_glue
             jglue.add_link(t_glue, x_colname, image, image.pixel_component_ids[1].label)
             jglue.add_link(t_glue, y_colname, image, image.pixel_component_ids[0].label)
-            viewer.add_data(t_glue)
+
+        try:
+            viewer.add_data(t_glue, **self.marker)
+        except Exception as e:  # pragma: no cover
+            self.app.hub.broadcast(SnackbarMessage(
+                f"Failed to add markers '{marker_name}': {repr(e)}",
+                color="warning", sender=self.app))
+        else:
+            self._marktags.add(marker_name)
+
+    def remove_markers(self, marker_name=None):
+        """Remove some but not all of the markers by name used when
+        adding the markers.
+
+        Parameters
+        ----------
+        marker_name : str
+            Name used when the markers were added.
+            If not given, will delete markers added under default name.
+
+        """
+        if marker_name is None:
+            marker_name = self._default_mark_tag_name
+
+        try:
+            i = self.app.data_collection.labels.index(marker_name)
+        except ValueError as e:  # pragma: no cover
+            self.app.hub.broadcast(SnackbarMessage(
+                f"Failed to remove markers '{marker_name}': {repr(e)}",
+                color="warning", sender=self.app))
+            return
+
+        data = self.app.data_collection[i]
+        self.app.data_collection.remove(data)
+        self._marktags.remove(marker_name)
+
+    def reset_markers(self):
+        """Delete all markers."""
+        # Grab the entire list of marker names before iterating
+        # otherwise what we are iterating over changes.
+        for marker_name in list(self._marktags):
+            self.remove_markers(marker_name=marker_name)
 
 
 def split_filename_with_fits_ext(filename):
@@ -295,10 +375,18 @@ def split_filename_with_fits_ext(filename):
     return filepath, ext, data_label
 
 
+def data_has_valid_wcs(data):
+    return hasattr(data, 'coords') and isinstance(data.coords, BaseHighLevelWCS)
+
+
+def layer_is_image_data(layer):
+    return isinstance(layer, BaseData) and layer.ndim == 2
+
+
 def get_top_layer_index(viewer):
-    """Get index of the top visible layer in Imviz.
+    """Get index of the top visible image layer in Imviz.
     This is because when blinked, first layer might not be top visible layer.
 
     """
     return [i for i, lyr in enumerate(viewer.layers)
-            if lyr.visible and isinstance(lyr.layer, BaseData)][-1]
+            if lyr.visible and layer_is_image_data(lyr.layer)][-1]

jdaviz/configs/imviz/plugins/viewers.py

@@ -1,10 +1,8 @@
 import numpy as np
 
-from astropy.wcs.wcsapi import BaseHighLevelWCS
-
-from glue.core import BaseData
 from glue_jupyter.bqplot.image import BqplotImageView
 
+from jdaviz.configs.imviz.helper import data_has_valid_wcs, layer_is_image_data
 from jdaviz.core.events import SnackbarMessage
 from jdaviz.core.registries import viewer_registry
 
@@ -74,7 +72,7 @@ def on_mouse_or_key_event(self, data):
 
             self.label_mouseover.pixel = (fmt.format(x, y))
 
-            if hasattr(image, 'coords') and isinstance(image.coords, BaseHighLevelWCS):
+            if data_has_valid_wcs(image):
                 # Convert these to a SkyCoord via WCS - note that for other datasets
                 # we aren't actually guaranteed to get a SkyCoord out, just for images
                 # with valid celestial WCS
@@ -116,7 +114,7 @@ def blink_once(self):
 
         # Exclude Subsets: They are global
         valid = [ilayer for ilayer, layer in enumerate(self.state.layers)
-                 if isinstance(layer.layer, BaseData)]
+                 if layer_is_image_data(layer.layer)]
         n_layers = len(valid)
 
         if n_layers == 1:
@@ -156,4 +154,4 @@ def data(self, cls=None):
         return [layer_state.layer  # .get_object(cls=cls or self.default_class)
                 for layer_state in self.state.layers
                 if hasattr(layer_state, 'layer') and
-                isinstance(layer_state.layer, BaseData)]
+                layer_is_image_data(layer_state.layer)]