Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 45 additions & 13 deletions astrowidgets/ginga.py
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Expand Down Expand Up @@ -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__()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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')

Expand All @@ -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 = []
Expand Down Expand Up @@ -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
Expand All @@ -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.'
Expand Down
30 changes: 21 additions & 9 deletions astrowidgets/interface_definition.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
]


Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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.

Expand All @@ -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

Expand Down
16 changes: 0 additions & 16 deletions astrowidgets/tests/test_image_widget.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import re

import pytest
import numpy as np
from astropy.table import Table, vstack
from astropy.wcs import WCS
Expand Down Expand Up @@ -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
Expand Down
84 changes: 64 additions & 20 deletions astrowidgets/tests/widget_api_test.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# TODO: How to enable switching out backend and still run the same tests?
import warnings

import pytest

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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")
Comment on lines +149 to +150
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, won't setting pytest to turn all warnings into exceptions take care of this?

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()
Expand All @@ -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
Expand All @@ -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',
Expand Down Expand Up @@ -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)
Expand Down