Merge pull request #98 from mwcraig/empty-marker-fix

Fix some edge cases in using named marker sets

astrowidgets/core.py

@@ -530,6 +530,10 @@ def get_markers(self, x_colname='x', y_colname='y',
                                          y_colname=y_colname,
                                          skycoord_colname=skycoord_colname,
                                          marker_name=name)
+                if table is None:
+                    # No markers by this name, skip it
+                    continue
+
                 try:
                     coordinates.extend(c for c in table[skycoord_colname])
                 except KeyError:
@@ -545,11 +549,19 @@ def get_markers(self, x_colname='x', y_colname='y',
 
             return stacked
 
+        # We should always allow the default name. The case
+        # where that table is empty will be handled in a moment.
+        if (marker_name not in self._marktags
+                and marker_name != self._default_mark_tag_name):
+            raise ValueError(f"No markers named '{marker_name}' found.")
+
         try:
             c_mark = self._viewer.canvas.get_object_by_tag(marker_name)
-        except Exception as e:  # No markers
-            self.logger.warning(str(e))
-            return
+        except Exception:
+            # No markers in this table. Issue a warning and continue
+            warnings.warn(f"Marker set named '{marker_name}' is empty",
+                          category=UserWarning)
+            return None
 
         image = self._viewer.get_image()
         xy_col = []

astrowidgets/tests/test_api.py

@@ -150,8 +150,10 @@ def test_reset_markers():
     image.add_markers(table, x_colname='x', y_colname='y',
                       skycoord_colname='coord', marker_name='test2')
     image.reset_markers()
-    assert image.get_markers(marker_name='test') is None
-    assert image.get_markers(marker_name='test2') is None
+    with pytest.raises(ValueError):
+        image.get_markers(marker_name='test')
+    with pytest.raises(ValueError):
+        image.get_markers(marker_name='test2')
 
 
 def test_remove_markers():

astrowidgets/tests/test_image_widget.py

@@ -10,6 +10,38 @@
 from ..core import ImageWidget, RESERVED_MARKER_SET_NAMES
 
 
+def _make_fake_ccd(with_wcs=True):
+    """
+    Generate a CCDData object for use with ImageWidget tests.
+
+    Parameters
+    ----------
+
+    with_wcs : bool, optional
+        If ``True`` the image will have a WCS attached to it,
+        which is useful for some of the marker tests.
+
+    Returns
+    -------
+
+    `astropy.nddata.CCDData`
+        CCD image
+    """
+    npix_side = 100
+    fake_image = np.random.randn(npix_side, npix_side)
+    if with_wcs:
+        wcs = WCS(naxis=2)
+        wcs.wcs.crpix = (fake_image.shape[0] / 2, fake_image.shape[1] / 2)
+        wcs.wcs.ctype = ('RA---TAN', 'DEC--TAN')
+        wcs.wcs.crval = (314.275419158, 31.6662781301)
+        wcs.wcs.pc = [[0.000153051015113, -3.20700931602e-05],
+                      [3.20704370872e-05, 0.000153072382405]]
+    else:
+        wcs = None
+
+    return CCDData(data=fake_image, wcs=wcs, unit='adu')
+
+
 def test_setting_image_width_height():
     image = ImageWidget()
     width = 200
@@ -40,15 +72,9 @@ def test_adding_markers_as_world_recovers_with_get_markers():
     Make sure that our internal conversion from world to pixel
     coordinates doesn't mess anything up.
     """
-    npix_side = 100
-    fake_image = np.random.randn(npix_side, npix_side)
-    wcs = WCS(naxis=2)
-    wcs.wcs.crpix = (fake_image.shape[0] / 2, fake_image.shape[1] / 2)
-    wcs.wcs.ctype = ('RA---TAN', 'DEC--TAN')
-    wcs.wcs.crval = (314.275419158, 31.6662781301)
-    wcs.wcs.pc = [[0.000153051015113, -3.20700931602e-05],
-                  [3.20704370872e-05, 0.000153072382405]]
-    fake_ccd = CCDData(data=fake_image, wcs=wcs, unit='adu')
+    fake_ccd = _make_fake_ccd(with_wcs=True)
+    npix_side = fake_ccd.shape[0]
+    wcs = fake_ccd.wcs
     iw = ImageWidget(pixel_coords_offset=0)
     iw.load_nddata(fake_ccd)
     # Get me 100 positions please, not right at the edge
@@ -215,3 +241,61 @@ def test_get_marker_with_names():
 
     assert (expected['x'] == all_marks['x']).all()
     assert (expected['y'] == all_marks['y']).all()
+
+
+def test_unknown_marker_name_error():
+    """
+    Regression test for https://github.com/astropy/astrowidgets/issues/97
+
+    This particular test checks that getting a marker name that
+    does not exist raises an error.
+    """
+    iw = ImageWidget()
+    bad_name = 'not a real marker name'
+    with pytest.raises(ValueError) as e:
+        iw.get_markers(marker_name=bad_name)
+
+    assert f"No markers named '{bad_name}'" in str(e.value)
+
+
+def test_marker_name_has_no_marks_warning():
+    """
+    Regression test for https://github.com/astropy/astrowidgets/issues/97
+
+    This particular test checks that getting an empty table gives a
+    useful warning message.
+    """
+    iw = ImageWidget()
+    bad_name = 'empty marker set'
+    iw.start_marking(marker_name=bad_name)
+
+    with pytest.warns(UserWarning) as record:
+        iw.get_markers(marker_name=bad_name)
+
+    assert f"Marker set named '{bad_name}' is empty" in str(record[0].message)
+
+
+def test_empty_marker_name_works_with_all():
+    """
+    Regression test for https://github.com/astropy/astrowidgets/issues/97
+
+    This particular test checks that an empty table doesn't break
+    marker_name='all'. The bug only comes up if there is a coordinate
+    column, so use a fake image a WCS.
+    """
+    iw = ImageWidget()
+    fake_ccd = _make_fake_ccd(with_wcs=True)
+    iw.load_nddata(fake_ccd)
+
+    x = np.array([20, 30, 40])
+    y = np.array([40, 80, 100])
+    input_markers = Table(data=[x, y], names=['x', 'y'])
+    # Add some markers with our own name
+    iw.add_markers(input_markers, marker_name='nonsense')
+
+    # Start marking to create a new marker set that is empty
+    iw.start_marking(marker_name='empty')
+
+    marks = iw.get_markers(marker_name='all')
+    assert len(marks) == len(x)
+    assert 'empty' not in marks['marker name']

setup.cfg

@@ -24,7 +24,8 @@ offline = True
 
 [flake8]
 # E501: line too long
-ignore = E501
+# W503: line break before binary operator
+ignore = E501,W503
 exclude = setup_package.py,conftest.py,__init__.py
 
 [metadata]