From 18dfd3582fd3d604862b51667c7c65c3081da97b Mon Sep 17 00:00:00 2001
From: Pey Lian Lim <2090236+pllim@users.noreply.github.com>
Date: Wed, 23 Jun 2021 18:10:57 -0400
Subject: [PATCH 1/7] WIP: add_markers initial implementation

---
 jdaviz/configs/imviz/helper.py | 64 +++++++++++++++++++++++++++++++++-
 notebooks/ImvizExample.ipynb   | 23 ++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/jdaviz/configs/imviz/helper.py b/jdaviz/configs/imviz/helper.py
index a699d7f872..332c7e4e8f 100644
--- a/jdaviz/configs/imviz/helper.py
+++ b/jdaviz/configs/imviz/helper.py
@@ -7,7 +7,7 @@
 from astropy.wcs import NoConvergence
 from astropy.wcs.wcsapi import BaseHighLevelWCS
 from echo import delay_callback
-from glue.core import BaseData
+from glue.core import BaseData, Data
 
 from jdaviz.core.events import SnackbarMessage
 from jdaviz.core.helpers import ConfigHelper
@@ -15,12 +15,20 @@
 __all__ = ['Imviz']
 
 ASTROPY_LT_4_3 = not minversion('astropy', '4.3')
+RESERVED_MARKER_SET_NAMES = ['all']
 
 
 class Imviz(ConfigHelper):
     """Imviz Helper class"""
     _default_configuration = 'imviz'
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        # Markers
+        self._marktags = set()
+        self._default_mark_tag_name = 'default-marker-name'
+
     def load_data(self, data, parser_reference=None, **kwargs):
         """Load data into Imviz.
 
@@ -185,6 +193,60 @@ 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
 
+    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:
+            raise ValueError(
+                f"The marker name {marker_name} is not allowed. Any name is "
+                f"allowed except these: {', '.join(RESERVED_MARKER_SET_NAMES)}")
+
+    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.
+
+        Parameters
+        ----------
+        table : `~astropy.table.Table`
+            Table containing marker locations.
+
+        x_colname, y_colname : str
+            Column names for X and Y.
+            Coordinates must be 0-indexed.
+
+        skycoord_colname : str
+            Column name with ``SkyCoord`` objects.
+
+        use_skycoord : bool
+            If `True`, use ``skycoord_colname`` to mark.
+            Otherwise, use ``x_colname`` and ``y_colname``.
+
+        marker_name : str, optional
+            Name to assign the markers in the table. Providing a name
+            allows markers to be removed by name at a later time.
+
+        """
+        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")
+
+        # TODO: Is this really the correct Data to use?
+        image = viewer.state.reference_data
+
+        if use_skycoord:
+            raise NotImplementedError
+        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)
+
 
 def split_filename_with_fits_ext(filename):
     """Split a ``filename[ext]`` input into filename and FITS extension.
diff --git a/notebooks/ImvizExample.ipynb b/notebooks/ImvizExample.ipynb
index c536191d2c..6a83b4f62f 100644
--- a/notebooks/ImvizExample.ipynb
+++ b/notebooks/ImvizExample.ipynb
@@ -314,6 +314,29 @@
     "imviz.offset_to(0.5 * u.arcsec, -1.5 * u.arcsec, skycoord_offset=True)"
    ]
   },
+  {
+   "cell_type": "markdown",
+   "id": "11fab067-7428-4ce4-bc9b-f5462fe52e2a",
+   "metadata": {},
+   "source": [
+    "It is also possible to programmatically add non-interactive markers to the image."
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "270a6dd2-ce21-457f-845e-19cf5405fee7",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "import numpy as np\n",
+    "from astropy.table import Table\n",
+    "\n",
+    "t = Table({'x': np.random.randint(0, 500, 100),\n",
+    "           'y': np.random.randint(0, 500, 100)})\n",
+    "imviz.add_markers(t)"
+   ]
+  },
   {
    "cell_type": "code",
    "execution_count": null,

From 52dc2ef3b3ee5549dba69ac341a25dfeca4115c4 Mon Sep 17 00:00:00 2001
From: Pey Lian Lim <2090236+pllim@users.noreply.github.com>
Date: Tue, 29 Jun 2021 15:25:58 -0400
Subject: [PATCH 2/7] Finished markers MVP

but still need tests. [ci skip]
---
 jdaviz/configs/imviz/helper.py          | 114 ++++++++++++++++++---
 jdaviz/configs/imviz/plugins/viewers.py |  10 +-
 notebooks/ImvizExample.ipynb            | 130 ++++++++++++++++++++----
 3 files changed, 217 insertions(+), 37 deletions(-)

diff --git a/jdaviz/configs/imviz/helper.py b/jdaviz/configs/imviz/helper.py
index 332c7e4e8f..e9b9b43fb5 100644
--- a/jdaviz/configs/imviz/helper.py
+++ b/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]
diff --git a/jdaviz/configs/imviz/plugins/viewers.py b/jdaviz/configs/imviz/plugins/viewers.py
index ed9f832ba9..8c408c8193 100644
--- a/jdaviz/configs/imviz/plugins/viewers.py
+++ b/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)]
diff --git a/notebooks/ImvizExample.ipynb b/notebooks/ImvizExample.ipynb
index 6a83b4f62f..e3f157bf60 100644
--- a/notebooks/ImvizExample.ipynb
+++ b/notebooks/ImvizExample.ipynb
@@ -45,15 +45,47 @@
     "%matplotlib inline"
    ]
   },
+  {
+   "cell_type": "markdown",
+   "id": "7c73155f-c062-461a-8ca7-0891ce142901",
+   "metadata": {},
+   "source": [
+    "Import modules needed for this notebook."
+   ]
+  },
   {
    "cell_type": "code",
    "execution_count": null,
-   "id": "bccca7f4",
+   "id": "0915bde6-e3cc-4038-aca1-c2ae852f7a69",
    "metadata": {},
    "outputs": [],
    "source": [
+    "import matplotlib.pyplot as plt\n",
+    "import numpy as np\n",
+    "from astropy import units as u\n",
+    "from astropy.coordinates import SkyCoord\n",
+    "from astropy.table import Table\n",
     "from astropy.utils.data import download_file\n",
+    "from glue.plugins.wcs_autolinking.wcs_autolinking import wcs_autolink, WCSLink\n",
     "\n",
+    "from jdaviz import Imviz"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "id": "60eb5782-264f-4af4-bb6e-ae583398277d",
+   "metadata": {},
+   "source": [
+    "Download some data. In this example, we use two 47 Tuc observations from HST/ACS."
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "bccca7f4",
+   "metadata": {},
+   "outputs": [],
+   "source": [
     "acs_47tuc_1 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03gjq_flc.fits', cache=True)\n",
     "acs_47tuc_2 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03h1q_flc.fits', cache=True)"
    ]
@@ -73,11 +105,6 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "import matplotlib.pyplot as plt\n",
-    "from glue.plugins.wcs_autolinking.wcs_autolinking import wcs_autolink, WCSLink\n",
-    "\n",
-    "from jdaviz import Imviz\n",
-    "\n",
     "imviz = Imviz()\n",
     "imviz.load_data(acs_47tuc_1, data_label='acs_47tuc_1')\n",
     "imviz.load_data(acs_47tuc_2, data_label='acs_47tuc_2')\n",
@@ -161,7 +188,10 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "viewer.state.layers[0].percentile = 95"
+    "viewer.state.layers[0].percentile = 95\n",
+    "\n",
+    "# Do this for the second image too.\n",
+    "viewer.state.layers[1].percentile = 95"
    ]
   },
   {
@@ -295,8 +325,6 @@
    "outputs": [],
    "source": [
     "# Center the image on given sky coordinates.\n",
-    "from astropy.coordinates import SkyCoord\n",
-    "\n",
     "sky = SkyCoord('00h24m07.33s -71d52m50.71s')\n",
     "imviz.center_on(sky)"
    ]
@@ -309,8 +337,6 @@
    "outputs": [],
    "source": [
     "# Move the image with the given sky offsets.\n",
-    "from astropy import units as u\n",
-    "\n",
     "imviz.offset_to(0.5 * u.arcsec, -1.5 * u.arcsec, skycoord_offset=True)"
    ]
   },
@@ -329,12 +355,19 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "import numpy as np\n",
-    "from astropy.table import Table\n",
-    "\n",
-    "t = Table({'x': np.random.randint(0, 500, 100),\n",
-    "           'y': np.random.randint(0, 500, 100)})\n",
-    "imviz.add_markers(t)"
+    "# Add 100 randomly generated X,Y (0-indexed) w.r.t. reference image\n",
+    "# using default marker properties.\n",
+    "t_xy = Table({'x': np.random.randint(0, 4096, 100),\n",
+    "              'y': np.random.randint(0, 2048, 100)})\n",
+    "imviz.add_markers(t_xy)"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "id": "f1c1aa0d-46b7-42f6-b6d7-395305c15f91",
+   "metadata": {},
+   "source": [
+    "You could customize marker color and alpha with values that Glue supports."
    ]
   },
   {
@@ -343,6 +376,67 @@
    "id": "07088bc0-3963-4230-8eb7-40109e10123c",
    "metadata": {},
    "outputs": [],
+   "source": [
+    "imviz.marker = {'color': '#4b0082', 'alpha': 0.8}"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "9ecfd0f9-da9e-419b-99a8-cc6efc20568f",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "# Mark some sky coordinates using updated marker properties.\n",
+    "t_sky = Table({'coord': [SkyCoord('00h24m07.33s -71d52m50.71s'),\n",
+    "                         SkyCoord('00h24m01.57s -71d53m17.77s'),\n",
+    "                         SkyCoord('00h24m11.70s -71d52m29.21s'),\n",
+    "                         SkyCoord('00h24m22.29s -71d53m28.04s')]})\n",
+    "imviz.add_markers(t_sky, use_skycoord=True, marker_name='my_sky')"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "id": "bf02dfc5-088b-4745-8dbc-2de1106c53ea",
+   "metadata": {},
+   "source": [
+    "When you do not need the markers anymore, you could remove them by name."
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "776c3b75-a1a9-4141-9d6d-8aa766c58d21",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "imviz.remove_markers(marker_name='my_sky')"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "id": "594e8b67-6725-416b-814e-8d49055821f2",
+   "metadata": {},
+   "source": [
+    "You can also remove all the markers."
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "046641de-b173-4377-92d3-344bd4ba48f3",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "imviz.reset_markers()"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "ebd78f30-7d53-4d66-85c2-502c207bedf9",
+   "metadata": {},
+   "outputs": [],
    "source": []
   }
  ],
@@ -362,7 +456,7 @@
    "name": "python",
    "nbconvert_exporter": "python",
    "pygments_lexer": "ipython3",
-   "version": "3.9.5"
+   "version": "3.8.3"
   }
  },
  "nbformat": 4,

From db4e8b0c8ac7cffc90dafc515b632da5b8aef78e Mon Sep 17 00:00:00 2001
From: Pey Lian Lim <2090236+pllim@users.noreply.github.com>
Date: Wed, 30 Jun 2021 18:24:05 -0400
Subject: [PATCH 3/7] Add markers tests

---
 jdaviz/configs/imviz/helper.py                |  48 ++++++--
 .../imviz/tests/test_astrowidgets_api.py      | 113 +++++++++++++-----
 jdaviz/configs/imviz/tests/utils.py           |  38 ++++++
 notebooks/ImvizExample.ipynb                  |   8 +-
 4 files changed, 157 insertions(+), 50 deletions(-)
 create mode 100644 jdaviz/configs/imviz/tests/utils.py

diff --git a/jdaviz/configs/imviz/helper.py b/jdaviz/configs/imviz/helper.py
index e9b9b43fb5..41ce42110d 100644
--- a/jdaviz/configs/imviz/helper.py
+++ b/jdaviz/configs/imviz/helper.py
@@ -28,8 +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}
+        # marker shape not settable: https://github.com/glue-viz/glue/issues/2202
+        self.marker = {'color': 'red', 'alpha': 1.0, 'markersize': 5}
 
     def load_data(self, data, parser_reference=None, **kwargs):
         """Load data into Imviz.
@@ -128,7 +128,7 @@ def center_on(self, point):
                         color="warning", sender=self.app))
                     return
             else:
-                raise AttributeError(f'{image.label} does not have a valid WCS')
+                raise AttributeError(f'{getattr(image, "label", None)} does not have a valid WCS')
         else:
             pix = point
 
@@ -187,7 +187,7 @@ def offset_to(self, dx, dy, skycoord_offset=False):
                     new_sky_cen = sky_cen.spherical_offsets_by(dx, dy)
                 self.center_on(new_sky_cen)
             else:
-                raise AttributeError(f'{image.label} does not have a valid WCS')
+                raise AttributeError(f'{getattr(image, "label", None)} does not have a valid WCS')
         else:
             with delay_callback(viewer.state, 'x_min', 'x_max', 'y_min', 'y_max'):
                 viewer.state.x_min += dx
@@ -201,10 +201,9 @@ def marker(self):
 
         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}
+            {'color': 'red', 'alpha': 1.0, 'markersize': 3}
+            {'color': '#ff0000', 'alpha': 0.5, 'markersize': 10}
+            {'color': (1, 0, 0)}
 
         Also see: https://docs.glueviz.org/en/stable/api/glue.core.visual.VisualAttributes.html
 
@@ -213,8 +212,26 @@ def marker(self):
 
     @marker.setter
     def marker(self, val):
-        # TODO: Add glupyter specific validation.
-        # Only set this once we have successfully created a marker
+        # Validation: Ideally Glue should do this but we have to due to
+        # https://github.com/glue-viz/glue/issues/2203
+        given = set(val.keys())
+        allowed = set(('color', 'alpha', 'markersize'))
+        if not given.issubset(allowed):
+            raise KeyError(f'Invalid attribute(s): {given - allowed}')
+        if 'color' in val:
+            from matplotlib.colors import ColorConverter
+            ColorConverter().to_rgb(val['color'])  # ValueError: Invalid RGBA argument
+        if 'alpha' in val:
+            alpha = val['alpha']
+            if not isinstance(alpha, (int, float)) or alpha < 0 or alpha > 1:
+                raise ValueError(f'Invalid alpha: {alpha}')
+        if 'markersize' in val:
+            size = val['markersize']
+            if not isinstance(size, (int, float)):
+                raise ValueError(f'Invalid marker size: {size}')
+
+        # Only set this once we have successfully validated a marker.
+        # Those not set here use Glue defaults.
         self._marker_dict = val
 
     def _validate_marker_name(self, marker_name):
@@ -266,14 +283,14 @@ def add_markers(self, table, x_colname='x', y_colname='y',
         viewer = self.app.get_viewer("viewer-1")
         jglue = self.app.session.application
 
-        # TODO: How to link to invidual images separately? add_link in loop does not work.
+        # TODO: How to link to invidual images separately for X/Y? 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:
             if not data_has_valid_wcs(image):
-                raise AttributeError(f'{image.label} does not have a valid WCS')
+                raise AttributeError(f'{getattr(image, "label", None)} 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
@@ -286,12 +303,17 @@ def add_markers(self, table, x_colname='x', y_colname='y',
             jglue.add_link(t_glue, y_colname, image, image.pixel_component_ids[0].label)
 
         try:
-            viewer.add_data(t_glue, **self.marker)
+            viewer.add_data(t_glue)
         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:
+            # Only can set alpha and color using viewer.add_data(), so brute force here instead.
+            # https://github.com/glue-viz/glue/issues/2201
+            for key, val in self.marker.items():
+                setattr(self.app.data_collection[self.app.data_collection.labels.index(marker_name)].style, key, val)  # noqa
+
             self._marktags.add(marker_name)
 
     def remove_markers(self, marker_name=None):
diff --git a/jdaviz/configs/imviz/tests/test_astrowidgets_api.py b/jdaviz/configs/imviz/tests/test_astrowidgets_api.py
index 4d31b0c0ba..b487758206 100644
--- a/jdaviz/configs/imviz/tests/test_astrowidgets_api.py
+++ b/jdaviz/configs/imviz/tests/test_astrowidgets_api.py
@@ -1,41 +1,12 @@
-import numpy as np
 import pytest
 from astropy import units as u
-from astropy.io import fits
-from astropy.wcs import WCS
+from astropy.table import Table
 from numpy.testing import assert_allclose
 
+from jdaviz.configs.imviz.tests.utils import BaseImviz_WCS_NoWCS
 
-class TestAstrowidgetsAPI:
-    @pytest.fixture(autouse=True)
-    def setup_class(self, imviz_app):
-        hdu = fits.ImageHDU(np.zeros((10, 10)), name='SCI')
-
-        # Apply some celestial WCS from
-        # https://learn.astropy.org/rst-tutorials/celestial_coords1.html
-        hdu.header.update({'CTYPE1': 'RA---TAN',
-                           'CUNIT1': 'deg',
-                           'CDELT1': -0.0002777777778,
-                           'CRPIX1': 1,
-                           'CRVAL1': 337.5202808,
-                           'NAXIS1': 10,
-                           'CTYPE2': 'DEC--TAN',
-                           'CUNIT2': 'deg',
-                           'CDELT2': 0.0002777777778,
-                           'CRPIX2': 1,
-                           'CRVAL2': -20.833333059999998,
-                           'NAXIS2': 10})
-
-        # Data with WCS
-        imviz_app.load_data(hdu, data_label='has_wcs')
-
-        # Data without WCS
-        imviz_app.load_data(hdu, data_label='no_wcs')
-        imviz_app.app.data_collection[1].coords = None
-
-        self.wcs = WCS(hdu.header)
-        self.imviz = imviz_app
-        self.viewer = imviz_app.app.get_viewer('viewer-1')
+
+class TestCenterOffset(BaseImviz_WCS_NoWCS):
 
     def test_center_offset_pixel(self):
         self.imviz.center_on((0, 1))
@@ -84,3 +55,79 @@ def test_center_offset_sky(self):
 
         with pytest.raises(AttributeError, match='does not have a valid WCS'):
             self.imviz.offset_to(dsky, dsky, skycoord_offset=True)
+
+
+class TestMarkers(BaseImviz_WCS_NoWCS):
+
+    def test_invalid_markers(self):
+        with pytest.raises(KeyError, match='Invalid attribute'):
+            self.imviz.marker = {'foo': 'bar', 'alpha': 0.8}
+        with pytest.raises(ValueError, match='Invalid RGBA argument'):
+            self.imviz.marker = {'color': 'greenfishbluefish'}
+        with pytest.raises(ValueError, match='Invalid alpha'):
+            self.imviz.marker = {'alpha': '1'}
+        with pytest.raises(ValueError, match='Invalid alpha'):
+            self.imviz.marker = {'alpha': 42}
+        with pytest.raises(ValueError, match='Invalid marker size'):
+            self.imviz.marker = {'markersize': '1'}
+
+    def test_mvp_markers(self):
+        x_pix = (0, 0)
+        y_pix = (0, 1)
+        sky = self.wcs.pixel_to_world(x_pix, y_pix)
+        tbl = Table({'x': x_pix, 'y': y_pix, 'coord': sky})
+
+        self.imviz.add_markers(tbl)
+        data = self.imviz.app.data_collection[2]
+        assert data.label == 'default-marker-name'
+        assert data.style.color == 'red'
+        assert data.style.marker == 'o'
+        assert_allclose(data.style.markersize, 5)
+        assert_allclose(data.style.alpha, 1)
+        assert_allclose(data.get_component('x').data, x_pix)
+        assert_allclose(data.get_component('y').data, y_pix)
+
+        # Table with only sky coordinates but no use_skycoord=True
+        with pytest.raises(KeyError):
+            self.imviz.add_markers(tbl[('coord', )])
+
+        # Cannot use reserved marker name
+        with pytest.raises(ValueError, match='not allowed'):
+            self.imviz.add_markers(tbl, use_skycoord=True, marker_name='all')
+
+        self.imviz.marker = {'color': (0, 1, 0), 'alpha': 0.8}
+
+        self.imviz.add_markers(tbl, use_skycoord=True, marker_name='my_sky')
+        data = self.imviz.app.data_collection[3]
+        assert data.label == 'my_sky'
+        assert data.style.color == (0, 1, 0)  # green
+        assert data.style.marker == 'o'
+        assert_allclose(data.style.markersize, 3)  # Glue default
+        assert_allclose(data.style.alpha, 0.8)
+        assert_allclose(data.get_component('ra').data, sky.ra.deg)
+        assert_allclose(data.get_component('dec').data, sky.dec.deg)
+
+        # TODO: How to check imviz.app.data_collection.links is correct?
+        assert len(self.imviz.app.data_collection.links) == 12
+
+        # Remove markers with default name.
+        self.imviz.remove_markers()
+        assert self.imviz.app.data_collection.labels == [
+            'has_wcs[SCI,1]', 'no_wcs[SCI,1]', 'my_sky']
+
+        # Reset markers (runs remove_markers with marker_name set)
+        self.imviz.reset_markers()
+        assert self.imviz.app.data_collection.labels == [
+            'has_wcs[SCI,1]', 'no_wcs[SCI,1]']
+
+        assert len(self.imviz.app.data_collection.links) == 8
+
+        # NOTE: This changes the state of self.imviz for this test class!
+
+        self.imviz.app.data_collection.remove(self.imviz.app.data_collection[0])
+        with pytest.raises(AttributeError, match='does not have a valid WCS'):
+            self.imviz.add_markers(tbl, use_skycoord=True, marker_name='my_sky')
+
+        self.imviz.app.data_collection.clear()
+        with pytest.raises(AttributeError, match='does not have a valid WCS'):
+            self.imviz.add_markers(tbl, use_skycoord=True, marker_name='my_sky')
diff --git a/jdaviz/configs/imviz/tests/utils.py b/jdaviz/configs/imviz/tests/utils.py
new file mode 100644
index 0000000000..28cf0531ad
--- /dev/null
+++ b/jdaviz/configs/imviz/tests/utils.py
@@ -0,0 +1,38 @@
+import numpy as np
+import pytest
+from astropy.io import fits
+from astropy.wcs import WCS
+
+__all__ = ['BaseImviz_WCS_NoWCS']
+
+
+class BaseImviz_WCS_NoWCS:
+    @pytest.fixture(autouse=True)
+    def setup_class(self, imviz_app):
+        hdu = fits.ImageHDU(np.zeros((10, 10)), name='SCI')
+
+        # Apply some celestial WCS from
+        # https://learn.astropy.org/rst-tutorials/celestial_coords1.html
+        hdu.header.update({'CTYPE1': 'RA---TAN',
+                           'CUNIT1': 'deg',
+                           'CDELT1': -0.0002777777778,
+                           'CRPIX1': 1,
+                           'CRVAL1': 337.5202808,
+                           'NAXIS1': 10,
+                           'CTYPE2': 'DEC--TAN',
+                           'CUNIT2': 'deg',
+                           'CDELT2': 0.0002777777778,
+                           'CRPIX2': 1,
+                           'CRVAL2': -20.833333059999998,
+                           'NAXIS2': 10})
+
+        # Data with WCS
+        imviz_app.load_data(hdu, data_label='has_wcs')
+
+        # Data without WCS
+        imviz_app.load_data(hdu, data_label='no_wcs')
+        imviz_app.app.data_collection[1].coords = None
+
+        self.wcs = WCS(hdu.header)
+        self.imviz = imviz_app
+        self.viewer = imviz_app.app.get_viewer('viewer-1')
diff --git a/notebooks/ImvizExample.ipynb b/notebooks/ImvizExample.ipynb
index e3f157bf60..fadfe3cd2d 100644
--- a/notebooks/ImvizExample.ipynb
+++ b/notebooks/ImvizExample.ipynb
@@ -367,17 +367,17 @@
    "id": "f1c1aa0d-46b7-42f6-b6d7-395305c15f91",
    "metadata": {},
    "source": [
-    "You could customize marker color and alpha with values that Glue supports."
+    "You could customize marker color, alpha, and size with values that Glue supports."
    ]
   },
   {
    "cell_type": "code",
    "execution_count": null,
-   "id": "07088bc0-3963-4230-8eb7-40109e10123c",
+   "id": "7c8edb6f-c928-4aee-8e64-73a34ab020d0",
    "metadata": {},
    "outputs": [],
    "source": [
-    "imviz.marker = {'color': '#4b0082', 'alpha': 0.8}"
+    "imviz.marker = {'color': 'green', 'alpha': 0.8, 'markersize': 50}"
    ]
   },
   {
@@ -434,7 +434,7 @@
   {
    "cell_type": "code",
    "execution_count": null,
-   "id": "ebd78f30-7d53-4d66-85c2-502c207bedf9",
+   "id": "ea66062b-a546-4d77-8119-b3f59d3c2104",
    "metadata": {},
    "outputs": [],
    "source": []

From 344bc2066c604f0a8a2e64de9cc48c2dccfacfa6 Mon Sep 17 00:00:00 2001
From: Pey Lian Lim <2090236+pllim@users.noreply.github.com>
Date: Fri, 2 Jul 2021 11:05:57 -0400
Subject: [PATCH 4/7] add_markers known issue

---
 docs/known_bugs.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/docs/known_bugs.rst b/docs/known_bugs.rst
index 4e62f68921..be1bc24631 100644
--- a/docs/known_bugs.rst
+++ b/docs/known_bugs.rst
@@ -83,6 +83,15 @@ has the unintended consequence of changing the contrast of the image displayed
 in the Cubeviz cube viewer.
 
 
+Imviz add_markers may not show markers
+--------------------------------------
+
+In some OS/browser combo, ``imviz.add_markers(...)`` might take a few tries
+to show the markers, or not at all. This is a known bug reported in
+https://github.com/glue-viz/glue-jupyter/issues/243 . If you encounter this,
+try a different OS/browser combo.
+
+
 Reporting a bug
 ---------------
 

From 308f0f18bc0d44ba8a0619b865debdc209325203 Mon Sep 17 00:00:00 2001
From: "P. L. Lim" <2090236+pllim@users.noreply.github.com>
Date: Fri, 2 Jul 2021 15:56:38 -0400
Subject: [PATCH 5/7] Clarify marker in docstring

---
 jdaviz/configs/imviz/helper.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/jdaviz/configs/imviz/helper.py b/jdaviz/configs/imviz/helper.py
index 41ce42110d..37a1c66763 100644
--- a/jdaviz/configs/imviz/helper.py
+++ b/jdaviz/configs/imviz/helper.py
@@ -247,6 +247,8 @@ def add_markers(self, table, x_colname='x', y_colname='y',
         """Creates markers w.r.t. the reference image at given points
         in the table.
 
+        .. note:: Use `marker` to change marker appearance.
+
         Parameters
         ----------
         table : `~astropy.table.Table`

From 31fdd7d481fe24f8ea2283947a45023bff937849 Mon Sep 17 00:00:00 2001
From: "P. L. Lim" <2090236+pllim@users.noreply.github.com>
Date: Fri, 2 Jul 2021 15:59:49 -0400
Subject: [PATCH 6/7] Apply suggestions from eteq

Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com>
---
 docs/known_bugs.rst            | 2 +-
 jdaviz/configs/imviz/helper.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/known_bugs.rst b/docs/known_bugs.rst
index be1bc24631..9660bece8e 100644
--- a/docs/known_bugs.rst
+++ b/docs/known_bugs.rst
@@ -86,7 +86,7 @@ in the Cubeviz cube viewer.
 Imviz add_markers may not show markers
 --------------------------------------
 
-In some OS/browser combo, ``imviz.add_markers(...)`` might take a few tries
+In some OS/browser combinations, ``imviz.add_markers(...)`` might take a few tries
 to show the markers, or not at all. This is a known bug reported in
 https://github.com/glue-viz/glue-jupyter/issues/243 . If you encounter this,
 try a different OS/browser combo.
diff --git a/jdaviz/configs/imviz/helper.py b/jdaviz/configs/imviz/helper.py
index 37a1c66763..2322d6dfea 100644
--- a/jdaviz/configs/imviz/helper.py
+++ b/jdaviz/configs/imviz/helper.py
@@ -205,7 +205,7 @@ def marker(self):
             {'color': '#ff0000', 'alpha': 0.5, 'markersize': 10}
             {'color': (1, 0, 0)}
 
-        Also see: https://docs.glueviz.org/en/stable/api/glue.core.visual.VisualAttributes.html
+        The valid properties for markers in imviz are listed at  https://docs.glueviz.org/en/stable/api/glue.core.visual.VisualAttributes.html
 
         """
         return self._marker_dict

From f706ac0a4ca16154f083c0f73fc7a1f8c5f4180f Mon Sep 17 00:00:00 2001
From: "P. L. Lim" <2090236+pllim@users.noreply.github.com>
Date: Fri, 2 Jul 2021 16:01:14 -0400
Subject: [PATCH 7/7] Fix PEP 8

---
 jdaviz/configs/imviz/helper.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/jdaviz/configs/imviz/helper.py b/jdaviz/configs/imviz/helper.py
index 2322d6dfea..2582e32619 100644
--- a/jdaviz/configs/imviz/helper.py
+++ b/jdaviz/configs/imviz/helper.py
@@ -205,7 +205,8 @@ def marker(self):
             {'color': '#ff0000', 'alpha': 0.5, 'markersize': 10}
             {'color': (1, 0, 0)}
 
-        The valid properties for markers in imviz are listed at  https://docs.glueviz.org/en/stable/api/glue.core.visual.VisualAttributes.html
+        The valid properties for markers in imviz are listed at
+        https://docs.glueviz.org/en/stable/api/glue.core.visual.VisualAttributes.html
 
         """
         return self._marker_dict