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

Update the interface and ginga implementation to make some changes of the marker API

Author: mwcraig

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
 

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)