Merge pull request #173 from mwcraig/update-marker-access

mwcraig · web-flow · commit 50f60cfb7dc2 · 2025-04-22T07:55:19.000-07:00
Update the interface and ginga implementation to make some changes of the marker API
diff --git a/astrowidgets/ginga.py b/astrowidgets/ginga.py
@@ -24,7 +24,9 @@
 
 from astrowidgets.interface_definition import (
     ALLOWED_CURSOR_LOCATIONS,
-    RESERVED_MARKER_SET_NAMES
+    RESERVED_MARKER_SET_NAMES,
+    DEFAULT_MARKER_NAME,
+    DEFAULT_INTERACTIVE_MARKER_NAME
 )
 
 __all__ = ['ImageWidget']
@@ -61,6 +63,12 @@ class ImageWidget(ipyw.VBox):
     # List of marker names that are for internal use only
     RESERVED_MARKER_SET_NAMES = RESERVED_MARKER_SET_NAMES
 
+    # Default marker name for marking via API
+    DEFAULT_MARKER_NAME: str = DEFAULT_MARKER_NAME
+
+    # Default marker name for interactive marking
+    DEFAULT_INTERACTIVE_MARKER_NAME: str = DEFAULT_INTERACTIVE_MARKER_NAME
+
     def __init__(self, logger=None, image_width=500, image_height=500,
                  pixel_coords_offset=0, **kwargs):
         super().__init__()
@@ -133,8 +141,8 @@ def __init__(self, logger=None, image_width=500, image_height=500,
         # duplicate names.
         self._marktags = set()
         # Let's have a default name for the tag too:
-        self._default_mark_tag_name = 'default-marker-name'
-        self._interactive_marker_set_name_default = 'interactive-markers'
+        self._default_mark_tag_name = DEFAULT_MARKER_NAME
+        self._interactive_marker_set_name_default = DEFAULT_INTERACTIVE_MARKER_NAME
         self._interactive_marker_set_name = self._interactive_marker_set_name_default
 
         # coordinates display
@@ -542,9 +550,21 @@ def get_markers(self, x_colname='x', y_colname='y',
             Table of markers, if any, or ``None``.
 
         """
+        default_column_names = [x_colname, y_colname, skycoord_colname, 'marker name']
+
+        empty_table = Table(names=default_column_names)
+
         if marker_name is None:
             marker_name = self._default_mark_tag_name
 
+        # If it isn't a string assume it is a list of strings
+        if not isinstance(marker_name, str):
+            return vstack([self.get_markers(x_colname=x_colname,
+                                            y_colname=y_colname,
+                                            skycoord_colname=skycoord_colname,
+                                            marker_name=name)
+                           for name in marker_name])
+
         if marker_name == 'all':
             # If it wasn't for the fact that SKyCoord columns can't
             # be stacked this would all fit nicely into a list
@@ -558,7 +578,7 @@ 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:
+                if len(table) == 0:
                     # No markers by this name, skip it
                     continue
 
@@ -571,7 +591,8 @@ def get_markers(self, x_colname='x', y_colname='y',
                 tables.append(table)
 
             if len(tables) == 0:
-                return None
+                # No markers at all, return an empty table
+                return empty_table
 
             stacked = vstack(tables, join_type='exact')
 
@@ -584,15 +605,13 @@ def get_markers(self, x_colname='x', y_colname='y',
         # 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.")
+            return empty_table
 
         try:
             c_mark = self._viewer.canvas.get_object_by_tag(marker_name)
-        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
+        except Exception:  # Keep this broad -- unclear what exceptions can be raised
+            # No markers in this table.
+            return empty_table
 
         image = self._viewer.get_image()
         xy_col = []
@@ -764,8 +783,9 @@ def remove_markers(self, marker_name=None):
         Parameters
         ----------
 
-        marker_name : str, optional
-            Name used when the markers were added.
+        marker_name : str, or list of str, optional
+            Name used when the markers were added. The name "all" will
+            remove all markers.
         """
         # TODO:
         #   arr : ``SkyCoord`` or array-like
@@ -777,6 +797,18 @@ def remove_markers(self, marker_name=None):
         if marker_name is None:
             marker_name = self._default_mark_tag_name
 
+        if marker_name == "all":
+            all_markers = self._marktags.copy()
+            for marker in all_markers:
+                self.remove_markers(marker_name=marker)
+            return
+
+        # If not a string assume, marker_name is a list
+        if not isinstance(marker_name, str):
+            for name in marker_name:
+                self.remove_markers(marker_name=name)
+            return
+
         if marker_name not in self._marktags:
             # This shouldn't have happened, raise an error
             raise ValueError('Marker name {} not found in current markers.'
diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py
@@ -12,13 +12,18 @@
 # Allowed locations for cursor display
 ALLOWED_CURSOR_LOCATIONS = ('top', 'bottom', None)
 
+DEFAULT_MARKER_NAME = 'default-marker-name'
+DEFAULT_INTERACTIVE_MARKER_NAME = 'interactive-markers'
+
 # List of marker names that are for internal use only
 RESERVED_MARKER_SET_NAMES = ('all',)
 
 __all__ = [
     'ImageViewerInterface',
     'ALLOWED_CURSOR_LOCATIONS',
-    'RESERVED_MARKER_SET_NAMES'
+    'RESERVED_MARKER_SET_NAMES',
+    'DEFAULT_MARKER_NAME',
+    'DEFAULT_INTERACTIVE_MARKER_NAME'
 ]
 
 
@@ -48,6 +53,12 @@ class ImageViewerInterface(Protocol):
     # List of marker names that are for internal use only
     RESERVED_MARKER_SET_NAMES: tuple = RESERVED_MARKER_SET_NAMES
 
+    # Default marker name for marking via API
+    DEFAULT_MARKER_NAME: str = DEFAULT_MARKER_NAME
+
+    # Default marker name for interactive marking
+    DEFAULT_INTERACTIVE_MARKER_NAME: str = DEFAULT_INTERACTIVE_MARKER_NAME
+
     # The methods, grouped loosely by purpose
 
     # Methods for loading data
@@ -179,15 +190,15 @@ def reset_markers(self) -> None:
     #     raise NotImplementedError
 
     @abstractmethod
-    def remove_markers(self, marker_name: str | None = None) -> None:
+    def remove_markers(self, marker_name: str | list[str] | None = None) -> None:
         """
         Remove markers from the image.
 
         Parameters
         ----------
         marker_name : str, optional
-            The name of the marker set to remove. If not given, all
-            markers will be removed.
+            The name of the marker set to remove. If the value is ``"all"``,
+            then all markers will be removed.
         """
         raise NotImplementedError
 
@@ -198,7 +209,7 @@ def remove_markers(self, marker_name: str | None = None) -> None:
     @abstractmethod
     def get_markers(self, x_colname: str = 'x', y_colname: str = 'y',
                     skycoord_colname: str = 'coord',
-                    marker_name: str | None = None) -> Table:
+                    marker_name: str | list[str] | None = None) -> Table:
         """
         Get the marker positions.
 
@@ -213,14 +224,15 @@ def get_markers(self, x_colname: str = 'x', y_colname: str = 'y',
         skycoord_colname : str, optional
             The name of the column containing the sky coordinates. Default
             is ``'coord'``.
-        marker_name : str, optional
-            The name of the marker set to use. If not given, all
-            markers will be returned.
+        marker_name : str or list of str, optional
+            The name of the marker set to use. If that value is ``"all"``,
+            then all markers will be returned.
 
         Returns
         -------
         table : `astropy.table.Table`
-            The table containing the marker positions.
+            The table containing the marker positions. If no markers match the
+            ``marker_name`` parameter, an empty table is returned.
         """
         raise NotImplementedError
 
diff --git a/astrowidgets/tests/test_image_widget.py b/astrowidgets/tests/test_image_widget.py
@@ -1,6 +1,5 @@
 import re
 
-import pytest
 import numpy as np
 from astropy.table import Table, vstack
 from astropy.wcs import WCS
@@ -119,21 +118,6 @@ def test_get_marker_with_names():
     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_empty_marker_name_works_with_all():
     """
     Regression test for https://github.com/astropy/astrowidgets/issues/97
diff --git a/astrowidgets/tests/widget_api_test.py b/astrowidgets/tests/widget_api_test.py
@@ -1,4 +1,5 @@
 # TODO: How to enable switching out backend and still run the same tests?
+import warnings
 
 import pytest
 
@@ -46,6 +47,11 @@ class variable does the trick.
         """
         self.image = self.image_widget_class(image_width=250, image_height=100)
 
+    def _check_marker_table_return_properties(self, table):
+        assert isinstance(table, Table)
+        assert len(table) == 0
+        assert sorted(table.colnames) == sorted(['x', 'y', 'coord', 'marker name'])
+
     def test_width_height(self):
         assert self.image.image_width == 250
         assert self.image.image_height == 100
@@ -103,7 +109,7 @@ def test_zoom(self):
 
     def test_marking_operations(self):
         marks = self.image.get_markers(marker_name="all")
-        assert marks is None
+        self._check_marker_table_return_properties(marks)
         assert not self.image.is_marking
 
         # Ensure you cannot set it like this.
@@ -138,12 +144,13 @@ def test_marking_operations(self):
         assert not self.image.click_drag
         assert not self.image.scroll_pan
 
-        # Regression test for GitHub Issue 97:
-        # Marker name with no markers should give warning.
-        with pytest.warns(UserWarning, match='is empty') as warning_lines:
+        # Make sure no warning is issued when trying to retrieve markers
+        # with a name that does not exist.
+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
             t = self.image.get_markers(marker_name='markymark')
-        assert t is None
-        assert len(warning_lines) == 1
+
+        self._check_marker_table_return_properties(t)
 
         self.image.click_drag = True
         self.image.start_marking()
@@ -160,7 +167,7 @@ def test_marking_operations(self):
         self.image.stop_marking(clear_markers=True)
 
         assert self.image.is_marking is False
-        assert self.image.get_markers(marker_name="all") is None
+        self._check_marker_table_return_properties(self.image.get_markers(marker_name="all"))
 
         # Hate this, should add to public API
         marknames = self.image._marktags
@@ -170,8 +177,7 @@ def test_marking_operations(self):
         assert self.image.click_drag
 
     def test_add_markers(self):
-        rng = np.random.default_rng(1234)
-        data = rng.integers(0, 100, (5, 2))
+        data = np.arange(10).reshape(5, 2)
         orig_tab = Table(data=data, names=['x', 'y'], dtype=('float', 'float'))
         tab = Table(data=data, names=['x', 'y'], dtype=('float', 'float'))
         self.image.add_markers(tab, x_colname='x', y_colname='y',
@@ -234,35 +240,73 @@ def test_add_markers(self):
         self.image.reset_markers()
         marknames = self.image._marktags
         assert len(marknames) == 0
-        assert self.image.get_markers(marker_name="all") is None
-        with pytest.warns(UserWarning, match='is empty'):
-            assert self.image.get_markers(marker_name=self.image._default_mark_tag_name) is None
+        self._check_marker_table_return_properties(self.image.get_markers(marker_name="all"))
+        # Check that no markers remain after clearing
+        tab = self.image.get_markers(marker_name=self.image._default_mark_tag_name)
+        self._check_marker_table_return_properties(tab)
+
+        # Check that retrieving a marker set that doesn't exist returns
+        # an empty table with the right columns
+        tab = self.image.get_markers(marker_name='test1')
+        self._check_marker_table_return_properties(tab)
+
+    def test_get_markers_accepts_list_of_names(self):
+        # Check that the get_markers method accepts a list of marker names
+        # and returns a table with all the markers from all the named sets.
+        data = np.arange(10).reshape((5, 2))
+        tab = Table(data=data, names=['x', 'y'])
+        self.image.add_markers(tab, marker_name='test1')
+        self.image.add_markers(tab, marker_name='test2')
 
-        with pytest.raises(ValueError, match="No markers named 'test1'"):
-            self.image.get_markers(marker_name='test1')
-        with pytest.raises(ValueError, match="No markers named 'test2'"):
-            self.image.get_markers(marker_name='test2')
+        # No guarantee markers will come back in the same order, so sort them.
+        t1 = self.image.get_markers(marker_name=['test1', 'test2'])
+        # Sort before comparing
+        t1.sort('x')
+        expected = vstack([tab, tab], join_type='exact')
+        expected.sort('x')
+        np.testing.assert_array_equal(t1['x'], expected['x'])
+        np.testing.assert_array_equal(t1['y'], expected['y'])
 
     def test_remove_markers(self):
         with pytest.raises(ValueError, match='arf'):
             self.image.remove_markers(marker_name='arf')
 
+    def test_remove_markers_name_all(self):
+        data = np.arange(10).reshape(5, 2)
+        tab = Table(data=data, names=['x', 'y'])
+        self.image.add_markers(tab, marker_name='test1')
+        self.image.add_markers(tab, marker_name='test2')
+
+        self.image.remove_markers(marker_name='all')
+        self._check_marker_table_return_properties(self.image.get_markers(marker_name='all'))
+
+    def test_remove_marker_accepts_list(self):
+        data = np.arange(10).reshape(5, 2)
+        tab = Table(data=data, names=['x', 'y'])
+        self.image.add_markers(tab, marker_name='test1')
+        self.image.add_markers(tab, marker_name='test2')
+
+        self.image.remove_markers(marker_name=['test1', 'test2'])
+        marks = self.image.get_markers(marker_name='all')
+        self._check_marker_table_return_properties(marks)
+
     def test_adding_markers_as_world(self, data, wcs):
         ndd = NDData(data=data, wcs=wcs)
         self.image.load_nddata(ndd)
 
         # Add markers using world coordinates
-        rng = np.random.default_rng(9435)
-
-        pixels = rng.integers(0, 100, (5, 2))
+        pixels = np.linspace(0, 100, num=10).reshape(5, 2)
         marks_pix = Table(data=pixels, names=['x', 'y'], dtype=('float', 'float'))
         marks_world = wcs.pixel_to_world(marks_pix['x'], marks_pix['y'])
         marks_coords = SkyCoord(marks_world, unit='degree')
         mark_coord_table = Table(data=[marks_coords], names=['coord'])
         self.image.add_markers(mark_coord_table, use_skycoord=True)
         result = self.image.get_markers()
         # Check the x, y positions as long as we are testing things...
-        np.testing.assert_allclose(result['x'], marks_pix['x'])
+        # The first test had one entry that was zero, so any check
+        # based on rtol will will. Added a small atol to make sure
+        # the test passes.
+        np.testing.assert_allclose(result['x'], marks_pix['x'], atol=1e-9)
         np.testing.assert_allclose(result['y'], marks_pix['y'])
         np.testing.assert_allclose(result['coord'].ra.deg,
                                    mark_coord_table['coord'].ra.deg)
