From 62f6b97af37d780bf588827602a2f3ee4b3ea906 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 12:27:07 -0500 Subject: [PATCH 01/14] Minimally disruptive change to Protocol --- astrowidgets/__init__.py | 2 +- astrowidgets/{core.py => ginga.py} | 0 astrowidgets/interface_definition.py | 93 +++++ astrowidgets/tests/test_api.py | 52 +-- astrowidgets/tests/test_image_widget.py | 16 +- astrowidgets/tests/test_widget_api_ginga.py | 16 + astrowidgets/tests/widget_api_test.py | 356 ++++++++++++++++++++ setup.cfg | 1 + 8 files changed, 501 insertions(+), 35 deletions(-) rename astrowidgets/{core.py => ginga.py} (100%) create mode 100644 astrowidgets/interface_definition.py create mode 100644 astrowidgets/tests/test_widget_api_ginga.py create mode 100644 astrowidgets/tests/widget_api_test.py diff --git a/astrowidgets/__init__.py b/astrowidgets/__init__.py index 30fa134..bd7beee 100644 --- a/astrowidgets/__init__.py +++ b/astrowidgets/__init__.py @@ -6,4 +6,4 @@ from ._astropy_init import * # noqa # ---------------------------------------------------------------------------- -from .core import * # noqa +from .ginga import * # noqa diff --git a/astrowidgets/core.py b/astrowidgets/ginga.py similarity index 100% rename from astrowidgets/core.py rename to astrowidgets/ginga.py diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py new file mode 100644 index 0000000..987f2ce --- /dev/null +++ b/astrowidgets/interface_definition.py @@ -0,0 +1,93 @@ +from typing import Protocol, runtime_checkable, Any +from abc import abstractmethod + +# Allowed locations for cursor display +ALLOWED_CURSOR_LOCATIONS = ['top', 'bottom', None] + +# List of marker names that are for internal use only +RESERVED_MARKER_SET_NAMES = ['all'] + +__all__ = [ + 'ImageViewerInterface', + 'ALLOWED_CURSOR_LOCATIONS', + 'RESERVED_MARKER_SET_NAMES' +] + + +@runtime_checkable +class ImageViewerInterface(Protocol): + # This are attributes, not methods. The type annotations are there + # to make sure Protocol knows they are attributes. Python does not + # do any checking at all of these types. + click_center: bool + click_drag: bool + scroll_pan: bool + image_width: int + image_height: int + zoom_level: float + marker: Any + cuts: Any + stretch: str + # viewer: Any + + # The methods, grouped loosely by purpose + + # Methods for loading data + @abstractmethod + def load_fits(self, file): + raise NotImplementedError + + @abstractmethod + def load_array(self, array): + raise NotImplementedError + + @abstractmethod + def load_nddata(self, data): + raise NotImplementedError + + # Saving contents of the view and accessing the view + @abstractmethod + def save(self, filename): + raise NotImplementedError + + # Marker-related methods + @abstractmethod + def start_marking(self): + raise NotImplementedError + + @abstractmethod + def stop_marking(self): + raise NotImplementedError + + @abstractmethod + def add_markers(self): + raise NotImplementedError + + @abstractmethod + def remove_all_markers(self): + raise NotImplementedError + + @abstractmethod + def remove_markers_by_name(self, marker_name=None): + raise NotImplementedError + + @abstractmethod + def get_all_markers(self): + raise NotImplementedError + + @abstractmethod + def get_markers_by_name(self, marker_name=None): + raise NotImplementedError + + # Methods that modify the view + @abstractmethod + def center_on(self): + raise NotImplementedError + + @abstractmethod + def offset_by(self): + raise NotImplementedError + + @abstractmethod + def zoom(self): + raise NotImplementedError diff --git a/astrowidgets/tests/test_api.py b/astrowidgets/tests/test_api.py index 37113e5..706a502 100644 --- a/astrowidgets/tests/test_api.py +++ b/astrowidgets/tests/test_api.py @@ -10,30 +10,30 @@ from ginga.ColorDist import ColorDistBase -from ..core import ImageWidget, ALLOWED_CURSOR_LOCATIONS +from ..ginga import ImageWidget, ALLOWED_CURSOR_LOCATIONS -def test_load_fits(): +def test_load_fits(): # yep image = ImageWidget() data = np.random.random([100, 100]) hdu = fits.PrimaryHDU(data=data) image.load_fits(hdu) -def test_load_nddata(): +def test_load_nddata(): # yep image = ImageWidget() data = np.random.random([100, 100]) nddata = NDData(data) image.load_nddata(nddata) -def test_load_array(): +def test_load_array(): # yep image = ImageWidget() data = np.random.random([100, 100]) image.load_array(data) -def test_center_on(): +def test_center_on(): # yep image = ImageWidget() x = 10 y = 10 @@ -48,7 +48,7 @@ def test_offset_to(): image.offset_to(dx, dy) -def test_offset_by(): +def test_offset_by(): # yep image = ImageWidget() # Pixels @@ -66,13 +66,13 @@ def test_offset_by(): image.offset_by(1 * u.arcsec, 1) -def test_zoom_level(): +def test_zoom_level(): # yep image = ImageWidget() image.zoom_level = 5 assert image.zoom_level == 5 -def test_zoom(): +def test_zoom(): # yep image = ImageWidget() image.zoom_level = 3 val = 2 @@ -80,19 +80,19 @@ def test_zoom(): assert image.zoom_level == 6 -@pytest.mark.xfail(reason='Not implemented yet') +@pytest.mark.xfail(reason='Not implemented yet') # Nope, note even sure what this is def test_select_points(): image = ImageWidget() image.select_points() -def test_get_selection(): +def test_get_selection(): # Um, how is this testing a selection?!? image = ImageWidget() marks = image.get_markers() assert isinstance(marks, Table) or marks is None -def test_stop_marking(): +def test_stop_marking(): # In test_marking_options image = ImageWidget() # This is not much of a test... image.stop_marking(clear_markers=True) @@ -100,14 +100,14 @@ def test_stop_marking(): assert image.is_marking is False -def test_is_marking(): +def test_is_marking(): # In test_marking_options image = ImageWidget() assert image.is_marking in [True, False] with pytest.raises(AttributeError): image.is_marking = True -def test_start_marking(): +def test_start_marking(): # In test_marking_options image = ImageWidget() # Setting these to check that start_marking affects them. @@ -142,7 +142,7 @@ def test_start_marking(): assert image.click_drag -def test_add_markers(): +def test_add_markers(): # In test_marking_options image = ImageWidget() table = Table(data=np.random.randint(0, 100, [5, 2]), names=['x', 'y'], dtype=('int', 'int')) @@ -150,7 +150,7 @@ def test_add_markers(): skycoord_colname='coord') -def test_set_markers(): +def test_set_markers(): # In test_marking_options image = ImageWidget() image.marker = {'color': 'yellow', 'radius': 10, 'type': 'cross'} assert 'cross' in str(image.marker) @@ -158,7 +158,7 @@ def test_set_markers(): assert '10' in str(image.marker) -def test_reset_markers(): +def test_reset_markers(): # In test_marking_options image = ImageWidget() # First test: this shouldn't raise any errors # (it also doesn't *do* anything...) @@ -177,7 +177,7 @@ def test_reset_markers(): image.get_markers(marker_name='test2') -def test_remove_markers(): +def test_remove_markers(): # In test_marking_options image = ImageWidget() # Add a tag name... image._marktags.add(image._default_mark_tag_name) @@ -186,7 +186,7 @@ def test_remove_markers(): assert 'arf' in str(e.value) -def test_stretch(): +def test_stretch(): # yep image = ImageWidget() with pytest.raises(ValueError) as e: image.stretch = 'not a valid value' @@ -196,7 +196,7 @@ def test_stretch(): assert isinstance(image.stretch, (ColorDistBase)) -def test_cuts(): +def test_cuts(): # yep image = ImageWidget() # An invalid string should raise an error @@ -219,7 +219,7 @@ def test_cuts(): assert image.cuts == (10, 100) -def test_colormap(): +def test_colormap(): # yep image = ImageWidget() cmap_desired = 'gray' cmap_list = image.colormap_options @@ -228,7 +228,7 @@ def test_colormap(): image.set_colormap(cmap_desired) -def test_cursor(): +def test_cursor(): # yep image = ImageWidget() assert image.cursor in ALLOWED_CURSOR_LOCATIONS with pytest.raises(ValueError): @@ -237,7 +237,7 @@ def test_cursor(): assert image.cursor == 'bottom' -def test_click_drag(): +def test_click_drag(): # yep image = ImageWidget() # Set this to ensure that click_drag turns it off image._click_center = True @@ -260,7 +260,7 @@ def test_click_drag(): assert 'Interactive marking' in str(e.value) -def test_click_center(): +def test_click_center(): # yep image = ImageWidget() assert (image.click_center is True) or (image.click_center is False) @@ -283,7 +283,7 @@ def test_click_center(): image.click_center = False -def test_scroll_pan(): +def test_scroll_pan(): # yep image = ImageWidget() # Make sure scroll_pan is actually settable @@ -292,13 +292,13 @@ def test_scroll_pan(): assert image.scroll_pan is val -def test_save(tmp_path): +def test_save(tmp_path): # yep image = ImageWidget() filename = 'woot.png' image.save(tmp_path / filename) -def test_width_height(): +def test_width_height(): # yep image = ImageWidget(image_width=250, image_height=100) assert image.image_width == 250 assert image.image_height == 100 diff --git a/astrowidgets/tests/test_image_widget.py b/astrowidgets/tests/test_image_widget.py index 4f4f142..c2398a5 100644 --- a/astrowidgets/tests/test_image_widget.py +++ b/astrowidgets/tests/test_image_widget.py @@ -7,7 +7,7 @@ from astropy.nddata import CCDData from astropy.coordinates import SkyCoord -from ..core import ImageWidget, RESERVED_MARKER_SET_NAMES +from ..ginga import ImageWidget, RESERVED_MARKER_SET_NAMES def _make_fake_ccd(with_wcs=True): @@ -42,7 +42,7 @@ def _make_fake_ccd(with_wcs=True): return CCDData(data=fake_image, wcs=wcs, unit='adu') -def test_setting_image_width_height(): +def test_setting_image_width_height(): # yep image = ImageWidget() width = 200 height = 300 @@ -51,7 +51,7 @@ def test_setting_image_width_height(): assert image._viewer.get_window_size() == (width, height) -def test_add_marker_does_not_modify_input_table(): +def test_add_marker_does_not_modify_input_table(): # in test_marking_operations # Regression test for #45 # Adding markers should not modify the input data table image = ImageWidget(image_width=300, image_height=300, @@ -67,7 +67,7 @@ def test_add_marker_does_not_modify_input_table(): assert (in_table == orig_table).all() -def test_adding_markers_as_world_recovers_with_get_markers(): +def test_adding_markers_as_world_recovers_with_get_markers(): # yep """ Make sure that our internal conversion from world to pixel coordinates doesn't mess anything up. @@ -128,7 +128,7 @@ def test_move_callback_includes_offset(): assert float(y_out) == data_y + offset -def test_can_add_markers_with_names(): +def test_can_add_markers_with_names(): # in test_marking_operations? """ Test a few things related to naming marker sets """ @@ -185,7 +185,7 @@ def test_can_add_markers_with_names(): assert image._interactive_marker_set_name in image._marktags -def test_mark_with_reserved_name_raises_error(): +def test_mark_with_reserved_name_raises_error(): # in test_marking_operations npix_side = 200 image = ImageWidget(image_width=npix_side, image_height=npix_side) @@ -258,7 +258,7 @@ def test_unknown_marker_name_error(): assert f"No markers named '{bad_name}'" in str(e.value) -def test_marker_name_has_no_marks_warning(): +def test_marker_name_has_no_marks_warning(): # in test_marking_operations """ Regression test for https://github.com/astropy/astrowidgets/issues/97 @@ -301,7 +301,7 @@ def test_empty_marker_name_works_with_all(): assert 'empty' not in marks['marker name'] -def test_add_single_marker(): +def test_add_single_marker(): # in test_marking_operations """ Test a few things related to naming marker sets """ diff --git a/astrowidgets/tests/test_widget_api_ginga.py b/astrowidgets/tests/test_widget_api_ginga.py new file mode 100644 index 0000000..4b47a77 --- /dev/null +++ b/astrowidgets/tests/test_widget_api_ginga.py @@ -0,0 +1,16 @@ +import pytest + +from .widget_api_test import ImageWidgetAPITest +from astrowidgets.interface_definition import ImageViewerInterface + +ginga = pytest.importorskip("ginga", + reason="Package required for test is not " + "available.") +from astrowidgets.ginga import ImageWidget # noqa: E402 + +def test_instance(): + image = ImageWidget() + assert isinstance(image, ImageViewerInterface) + +class TestGingaWidget(ImageWidgetAPITest): + image_widget_class = ImageWidget diff --git a/astrowidgets/tests/widget_api_test.py b/astrowidgets/tests/widget_api_test.py new file mode 100644 index 0000000..bd8827f --- /dev/null +++ b/astrowidgets/tests/widget_api_test.py @@ -0,0 +1,356 @@ +# TODO: How to enable switching out backend and still run the same tests? + +import pytest + +import numpy as np # noqa: E402 + +from astropy.coordinates import SkyCoord # noqa: E402 +from astropy.io import fits # noqa: E402 +from astropy.nddata import NDData # noqa: E402 +from astropy.table import Table, vstack # noqa: E402 +from astropy import units as u # noqa: E402 +from astropy.wcs import WCS # noqa: E402 + + +class ImageWidgetAPITest: + cursor_error_classes = (ValueError) + + @pytest.fixture + def data(self): + rng = np.random.default_rng(1234) + return rng.random((100, 100)) + + @pytest.fixture + def wcs(self): + # This is a copy/paste from the astropy 4.3.1 documentation... + + # Create a new WCS object. The number of axes must be set + # from the start + w = WCS(naxis=2) + + # Set up an "Airy's zenithal" projection + w.wcs.crpix = [-234.75, 8.3393] + w.wcs.cdelt = np.array([-0.066667, 0.066667]) + w.wcs.crval = [0, -90] + w.wcs.ctype = ["RA---AIR", "DEC--AIR"] + w.wcs.set_pv([(2, 1, 45.0)]) + return w + + # This setup is run before each test, ensuring that there are no + # side effects of one test on another + @pytest.fixture(autouse=True) + def setup(self): + """ + Subclasses MUST define ``image_widget_class`` -- doing so as a + class variable does the trick. + """ + self.image = self.image_widget_class(image_width=250, image_height=100) + + def test_width_height(self): + assert self.image.image_width == 250 + assert self.image.image_height == 100 + + width = 200 + height = 300 + self.image.image_width = width + self.image.image_height = height + assert self.image.image_width == width + assert self.image.image_height == height + + def test_load_fits(self, data): + hdu = fits.PrimaryHDU(data=data) + self.image.load_fits(hdu) + + def test_load_nddata(self, data): + nddata = NDData(data) + self.image.load_nddata(nddata) + + def test_load_array(self, data): + self.image.load_array(data) + + def test_center_on(self): + self.image.center_on((10, 10)) # X, Y + + def test_offset_by(self, data, wcs): + self.image.offset_by(10, 10) # dX, dY + + # Testing offset by WCS requires a WCS. The viewer will (or ought to + # have) taken care of setting up the WCS internally if initialized with + # an NDData that has a WCS. + ndd = NDData(data=data, wcs=wcs) + self.image.load_nddata(ndd) + + self.image.offset_by(10 * u.arcmin, 10 * u.arcmin) + + # A mix of pixel and sky should produce an error + with pytest.raises(ValueError, match='but dy is of type'): + self.image.offset_by(10 * u.arcmin, 10) + + # A mix of inconsistent units should produce an error + with pytest.raises(u.UnitConversionError): + self.image.offset_by(1 * u.arcsec, 1 * u.AA) + + def test_zoom_level(self, data): + # Set data first, since that is needed to determine zoom level + self.image.load_array(data) + self.image.zoom_level = 5 + assert self.image.zoom_level == 5 + + def test_zoom(self): + self.image.zoom_level = 3 + self.image.zoom(2) + assert self.image.zoom_level == 6 # 3 x 2 + + def test_marking_operations(self): + marks = self.image.get_all_markers() + assert marks is None + assert not self.image.is_marking + + # Ensure you cannot set it like this. + with pytest.raises(AttributeError): + self.image.is_marking = True + + # Setting these to check that start_marking affects them. + self.image.click_center = True # Disables click_drag + assert self.image.click_center + self.image.scroll_pan = False + assert not self.image.scroll_pan + + # Set the marker style + marker_style = {'color': 'yellow', 'radius': 10, 'type': 'cross'} + m_str = str(self.image.marker) + for key in marker_style.keys(): + assert key in m_str + + self.image.start_marking(marker_name='markymark', marker=marker_style) + assert self.image.is_marking + assert self.image.marker == marker_style + assert not self.image.click_center + assert not self.image.click_drag + + # scroll_pan better activate when marking otherwise there is + # no way to pan while interactively marking + assert self.image.scroll_pan + + # Make sure that when we stop_marking we get our old controls back. + self.image.stop_marking() + assert self.image.click_center + 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: + t = self.image.get_markers_by_name('markymark') + assert t is None + assert len(warning_lines) == 1 + + self.image.click_drag = True + self.image.start_marking() + assert not self.image.click_drag + + # Simulate a mouse click to add default marker name to the list. + self.image._mouse_click_cb(self.image.viewer, None, 50, 50) + assert self.image.get_marker_names() == [self.image._interactive_marker_set_name, 'markymark'] + + # Clear markers to not pollute other tests. + self.image.stop_marking(clear_markers=True) + + assert self.image.is_marking is False + assert self.image.get_all_markers() is None + assert len(self.image.get_marker_names()) == 0 + + # Make sure that click_drag is restored as expected + assert self.image.click_drag + + def test_add_markers(self): + rng = np.random.default_rng(1234) + data = rng.integers(0, 100, (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', + skycoord_colname='coord', marker_name='test1') + + # Make sure setting didn't change the default name + assert self.image._default_mark_tag_name == 'default-marker-name' + + # Regression test for GitHub Issue 45: + # Adding markers should not modify the input data table. + assert (tab == orig_tab).all() + + # Add more markers under different name. + self.image.add_markers(tab, x_colname='x', y_colname='y', + skycoord_colname='coord', marker_name='test2') + assert self.image.get_marker_names() == ['test1', 'test2'] + + # No guarantee markers will come back in the same order, so sort them. + t1 = self.image.get_markers_by_name('test1') + # Sort before comparing + t1.sort('x') + tab.sort('x') + assert np.all(t1['x'] == tab['x']) + assert (t1['y'] == tab['y']).all() + + # That should have given us two copies of the input table + t2 = self.image.get_all_markers() + expected = vstack([tab, tab], join_type='exact') + # Sort before comparing + t2.sort(['x', 'y']) + expected.sort(['x', 'y']) + assert (t2['x'] == expected['x']).all() + assert (t2['y'] == expected['y']).all() + + self.image.remove_markers_by_name('test1') + assert self.image.get_marker_names() == ['test2'] + + # Ensure unable to mark with reserved name + for name in self.image.RESERVED_MARKER_SET_NAMES: + with pytest.raises(ValueError, match='not allowed'): + self.image.add_markers(tab, marker_name=name) + + + # Add markers with no marker name and check we can retrieve them + # using the default marker name + self.image.add_markers(tab, x_colname='x', y_colname='y', + skycoord_colname='coord') + # Don't care about the order of the marker names so use set instead of + # list. + assert (set(self.image.get_marker_names()) == + set(['test2', self.image._default_mark_tag_name])) + + # Clear markers to not pollute other tests. + self.image.remove_all_markers() + assert len(self.image.get_marker_names()) == 0 + assert self.image.get_all_markers() is None + with pytest.warns(UserWarning, match='is empty'): + assert self.image.get_markers_by_name(self.image._default_mark_tag_name) is None + + with pytest.raises(ValueError, match="No markers named 'test1'"): + self.image.get_markers_by_name('test1') + with pytest.raises(ValueError, match="No markers named 'test2'"): + self.image.get_markers_by_name('test2') + + def test_remove_markers(self): + with pytest.raises(ValueError, match='arf'): + self.image.remove_markers_by_name('arf') + + 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)) + 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']) + np.testing.assert_allclose(result['y'], marks_pix['y']) + np.testing.assert_allclose(result['coord'].ra.deg, + mark_coord_table['coord'].ra.deg) + np.testing.assert_allclose(result['coord'].dec.deg, + mark_coord_table['coord'].dec.deg) + + def test_stretch(self): + original_stretch = self.image.stretch + + with pytest.raises(ValueError, match='must be one of'): + self.image.stretch = 'not a valid value' + + # A bad value should leave the stretch unchanged + assert self.image.stretch is original_stretch + + self.image.stretch = 'log' + # A valid value should change the stretch + assert self.image.stretch is not original_stretch + + def test_cuts(self, data): + with pytest.raises(ValueError, match='must be one of'): + self.image.cuts = 'not a valid value' + + with pytest.raises(ValueError, match=r'must be given as \(low, high\)'): + self.image.cuts = (1, 10, 100) + + assert 'histogram' in self.image.autocut_options + + # Setting using histogram requires data + self.image.load_array(data) + self.image.cuts = 'histogram' + assert len(self.image.cuts) == 2 + + self.image.cuts = (10, 100) + assert self.image.cuts == (10, 100) + + def test_colormap(self): + cmap_desired = 'gray' + cmap_list = self.image.colormap_options + assert len(cmap_list) > 0 and cmap_desired in cmap_list + self.image.set_colormap(cmap_desired) + + def test_cursor(self): + assert self.image.cursor in self.image.ALLOWED_CURSOR_LOCATIONS + with pytest.raises(self.cursor_error_classes): + self.image.cursor = 'not a valid option' + self.image.cursor = 'bottom' + assert self.image.cursor == 'bottom' + + def test_click_drag(self): + # Set this to ensure that click_drag turns it off + self.image.click_center = True + + # Make sure that setting click_drag to False does not turn off + # click_center. + self.image.click_drag = False + assert self.image.click_center + + self.image.click_drag = True + assert not self.image.click_center + + # If is_marking is true then trying to enable click_drag should fail + self.image._is_marking = True + self.image.click_drag = False + with pytest.raises(ValueError, match='[Ii]nteractive marking'): + self.image.click_drag = True + self.image._is_marking = False + + def test_click_center(self): + # Set this to ensure that click_center turns it off + self.image.click_drag = True + + # Make sure that setting click_center to False does not turn off + # click_draf. + self.image.click_center = False + assert self.image.click_drag + + self.image.click_center = True + assert not self.image.click_drag + + # If is_marking is true then trying to enable click_center should fail + self.image._is_marking = True + self.image.click_center = False + with pytest.raises(ValueError, match='[Ii]nteractive marking'): + self.image.click_center = True + self.image._is_marking = False + + def test_scroll_pan(self): + # Make sure scroll_pan is actually settable + for value in [True, False]: + self.image.scroll_pan = value + assert self.image.scroll_pan is value + + def test_save(self, tmpdir): + with pytest.raises(ValueError, match='not supported'): + self.image.save(str(tmpdir.join('woot.jpg'))) + + filename = str(tmpdir.join('woot.png')) + self.image.save(filename) + + with pytest.raises(ValueError, match='exists'): + self.image.save(filename) + + self.image.save(filename, overwrite=True) diff --git a/setup.cfg b/setup.cfg index 6fc5a7c..5cc2e38 100644 --- a/setup.cfg +++ b/setup.cfg @@ -13,6 +13,7 @@ filterwarnings = ignore:zmq\.eventloop\.ioloop is deprecated in pyzmq 17:DeprecationWarning ignore:Widget.* is deprecated:DeprecationWarning ignore:Marker set named:UserWarning + ignore:There is no current event loop:DeprecationWarning [flake8] # E501: line too long From a8f7cbd096327d8421d11f78ef8ada20ca4c163a Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:17:36 -0500 Subject: [PATCH 02/14] Make constants class variables --- astrowidgets/ginga.py | 17 ++++++++--------- astrowidgets/interface_definition.py | 6 ++++++ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/astrowidgets/ginga.py b/astrowidgets/ginga.py index 65f293c..7cb0215 100644 --- a/astrowidgets/ginga.py +++ b/astrowidgets/ginga.py @@ -23,12 +23,6 @@ __all__ = ['ImageWidget'] -# Allowed locations for cursor display -ALLOWED_CURSOR_LOCATIONS = ['top', 'bottom', None] - -# List of marker names that are for internal use only -RESERVED_MARKER_SET_NAMES = ['all'] - class ImageWidget(ipyw.VBox): """ @@ -55,6 +49,11 @@ class ImageWidget(ipyw.VBox): correct value to use.* """ + # Allowed locations for cursor display + ALLOWED_CURSOR_LOCATIONS = ('top', 'bottom', None) + + # List of marker names that are for internal use only + RESERVED_MARKER_SET_NAMES = ('all', ) def __init__(self, logger=None, image_width=500, image_height=500, pixel_coords_offset=0, **kwargs): @@ -648,11 +647,11 @@ 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: + if marker_name in self.RESERVED_MARKER_SET_NAMES: raise ValueError('The marker name {} is not allowed. Any name is ' 'allowed except these: ' '{}'.format(marker_name, - ', '.join(RESERVED_MARKER_SET_NAMES))) + ', '.join(self.RESERVED_MARKER_SET_NAMES))) def add_markers(self, table, x_colname='x', y_colname='y', skycoord_colname='coord', use_skycoord=False, @@ -888,7 +887,7 @@ def cursor(self, val): else: raise ValueError('Invalid value {} for cursor.' 'Valid values are: ' - '{}'.format(val, ALLOWED_CURSOR_LOCATIONS)) + '{}'.format(val, self.ALLOWED_CURSOR_LOCATIONS)) self._cursor = val @property diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index 987f2ce..de66a5e 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -30,6 +30,12 @@ class ImageViewerInterface(Protocol): stretch: str # viewer: Any + # Allowed locations for cursor display + ALLOWED_CURSOR_LOCATIONS = ('top', 'bottom', None) + + # List of marker names that are for internal use only + RESERVED_MARKER_SET_NAMES = ('all') + # The methods, grouped loosely by purpose # Methods for loading data From e735a23decaa3f91ca0bd32324c16471cc18737d Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:17:52 -0500 Subject: [PATCH 03/14] Fix logic error if there are no tables --- astrowidgets/ginga.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/astrowidgets/ginga.py b/astrowidgets/ginga.py index 7cb0215..d3cc6ef 100644 --- a/astrowidgets/ginga.py +++ b/astrowidgets/ginga.py @@ -564,6 +564,9 @@ def get_markers(self, x_colname='x', y_colname='y', del table[skycoord_colname] tables.append(table) + if len(tables) == 0: + return None + stacked = vstack(tables, join_type='exact') if coordinates: From 38cdcd7013ce7954ec1644c9c35eba28744c916e Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:18:43 -0500 Subject: [PATCH 04/14] Switch back to original marker API and add arguments to methods --- astrowidgets/interface_definition.py | 30 +++++++---- astrowidgets/tests/widget_api_test.py | 73 +++++++++++++++------------ 2 files changed, 61 insertions(+), 42 deletions(-) diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index de66a5e..e6f1464 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -58,31 +58,43 @@ def save(self, filename): # Marker-related methods @abstractmethod - def start_marking(self): + def start_marking(self, marker_name=None): raise NotImplementedError @abstractmethod - def stop_marking(self): + def stop_marking(self, clear_markers=False): raise NotImplementedError @abstractmethod - def add_markers(self): + def add_markers(self, table, x_colname='x', y_colname='y', + skycoord_colname='coord', use_skycoord=False, + marker_name=None): raise NotImplementedError - @abstractmethod - def remove_all_markers(self): - raise NotImplementedError + # @abstractmethod + # def remove_all_markers(self): + # raise NotImplementedError @abstractmethod - def remove_markers_by_name(self, marker_name=None): + def reset_markers(self): raise NotImplementedError + # @abstractmethod + # def remove_markers_by_name(self, marker_name=None): + # raise NotImplementedError + @abstractmethod - def get_all_markers(self): + def remove_markers(self, marker_name=None): raise NotImplementedError + # @abstractmethod + # def get_all_markers(self): + # raise NotImplementedError + @abstractmethod - def get_markers_by_name(self, marker_name=None): + def get_markers(self, x_colname='x', y_colname='y', + skycoord_colname='coord', + marker_name=None): raise NotImplementedError # Methods that modify the view diff --git a/astrowidgets/tests/widget_api_test.py b/astrowidgets/tests/widget_api_test.py index bd8827f..2faeac3 100644 --- a/astrowidgets/tests/widget_api_test.py +++ b/astrowidgets/tests/widget_api_test.py @@ -102,7 +102,7 @@ def test_zoom(self): assert self.image.zoom_level == 6 # 3 x 2 def test_marking_operations(self): - marks = self.image.get_all_markers() + marks = self.image.get_markers(marker_name="all") assert marks is None assert not self.image.is_marking @@ -141,7 +141,7 @@ def test_marking_operations(self): # Regression test for GitHub Issue 97: # Marker name with no markers should give warning. with pytest.warns(UserWarning, match='is empty') as warning_lines: - t = self.image.get_markers_by_name('markymark') + t = self.image.get_markers(marker_name='markymark') assert t is None assert len(warning_lines) == 1 @@ -150,15 +150,21 @@ def test_marking_operations(self): assert not self.image.click_drag # Simulate a mouse click to add default marker name to the list. - self.image._mouse_click_cb(self.image.viewer, None, 50, 50) - assert self.image.get_marker_names() == [self.image._interactive_marker_set_name, 'markymark'] + try: + self.image._mouse_click_cb(self.image.viewer, None, 50, 50) + assert self.image.get_marker_names() == [self.image._interactive_marker_set_name, 'markymark'] + except AttributeError: + pass # Clear markers to not pollute other tests. self.image.stop_marking(clear_markers=True) assert self.image.is_marking is False - assert self.image.get_all_markers() is None - assert len(self.image.get_marker_names()) == 0 + assert self.image.get_markers(marker_name="all") is None + + # Hate this, should add to public API + marknames = self.image._marktags + assert len(marknames) == 0 # Make sure that click_drag is restored as expected assert self.image.click_drag @@ -181,10 +187,13 @@ def test_add_markers(self): # Add more markers under different name. self.image.add_markers(tab, x_colname='x', y_colname='y', skycoord_colname='coord', marker_name='test2') - assert self.image.get_marker_names() == ['test1', 'test2'] + + marknames = self.image._marktags + assert marknames == set(['test1', 'test2']) + # assert self.image.get_marker_names() == ['test1', 'test2'] # No guarantee markers will come back in the same order, so sort them. - t1 = self.image.get_markers_by_name('test1') + t1 = self.image.get_markers(marker_name='test1') # Sort before comparing t1.sort('x') tab.sort('x') @@ -192,7 +201,7 @@ def test_add_markers(self): assert (t1['y'] == tab['y']).all() # That should have given us two copies of the input table - t2 = self.image.get_all_markers() + t2 = self.image.get_markers(marker_name="all") expected = vstack([tab, tab], join_type='exact') # Sort before comparing t2.sort(['x', 'y']) @@ -200,8 +209,10 @@ def test_add_markers(self): assert (t2['x'] == expected['x']).all() assert (t2['y'] == expected['y']).all() - self.image.remove_markers_by_name('test1') - assert self.image.get_marker_names() == ['test2'] + self.image.remove_markers(marker_name='test1') + marknames = self.image._marktags + assert marknames == set(['test2']) + # assert self.image.get_marker_names() == ['test2'] # Ensure unable to mark with reserved name for name in self.image.RESERVED_MARKER_SET_NAMES: @@ -215,24 +226,28 @@ def test_add_markers(self): skycoord_colname='coord') # Don't care about the order of the marker names so use set instead of # list. - assert (set(self.image.get_marker_names()) == + marknames = self.image._marktags + assert (set(marknames) == set(['test2', self.image._default_mark_tag_name])) + # assert (set(self.image.get_marker_names()) == + # set(['test2', self.image._default_mark_tag_name])) # Clear markers to not pollute other tests. - self.image.remove_all_markers() - assert len(self.image.get_marker_names()) == 0 - assert self.image.get_all_markers() is None + 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_by_name(self.image._default_mark_tag_name) is None + assert self.image.get_markers(marker_name=self.image._default_mark_tag_name) is None with pytest.raises(ValueError, match="No markers named 'test1'"): - self.image.get_markers_by_name('test1') + self.image.get_markers(marker_name='test1') with pytest.raises(ValueError, match="No markers named 'test2'"): - self.image.get_markers_by_name('test2') + self.image.get_markers(marker_name='test2') def test_remove_markers(self): with pytest.raises(ValueError, match='arf'): - self.image.remove_markers_by_name('arf') + self.image.remove_markers(marker_name='arf') def test_adding_markers_as_world(self, data, wcs): ndd = NDData(data=data, wcs=wcs) @@ -273,7 +288,7 @@ def test_cuts(self, data): with pytest.raises(ValueError, match='must be one of'): self.image.cuts = 'not a valid value' - with pytest.raises(ValueError, match=r'must be given as \(low, high\)'): + with pytest.raises(ValueError, match='must have length 2'): self.image.cuts = (1, 10, 100) assert 'histogram' in self.image.autocut_options @@ -312,9 +327,9 @@ def test_click_drag(self): assert not self.image.click_center # If is_marking is true then trying to enable click_drag should fail - self.image._is_marking = True self.image.click_drag = False - with pytest.raises(ValueError, match='[Ii]nteractive marking'): + self.image._is_marking = True + with pytest.raises(ValueError, match=r'([Ii]nteractive marking)|(while in marking mode)'): self.image.click_drag = True self.image._is_marking = False @@ -333,7 +348,7 @@ def test_click_center(self): # If is_marking is true then trying to enable click_center should fail self.image._is_marking = True self.image.click_center = False - with pytest.raises(ValueError, match='[Ii]nteractive marking'): + with pytest.raises(ValueError, match=r'([Ii]nteractive marking)|(while in marking mode)'): self.image.click_center = True self.image._is_marking = False @@ -343,14 +358,6 @@ def test_scroll_pan(self): self.image.scroll_pan = value assert self.image.scroll_pan is value - def test_save(self, tmpdir): - with pytest.raises(ValueError, match='not supported'): - self.image.save(str(tmpdir.join('woot.jpg'))) - - filename = str(tmpdir.join('woot.png')) + def test_save(self, tmp_path): + filename = tmp_path / 'woot.png' self.image.save(filename) - - with pytest.raises(ValueError, match='exists'): - self.image.save(filename) - - self.image.save(filename, overwrite=True) From 9ad77977b8dfb45e07734123a506ea3b59ac8242 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:20:11 -0500 Subject: [PATCH 05/14] Remove old API tests --- astrowidgets/tests/test_api.py | 304 --------------------------------- 1 file changed, 304 deletions(-) delete mode 100644 astrowidgets/tests/test_api.py diff --git a/astrowidgets/tests/test_api.py b/astrowidgets/tests/test_api.py deleted file mode 100644 index 706a502..0000000 --- a/astrowidgets/tests/test_api.py +++ /dev/null @@ -1,304 +0,0 @@ -import numpy as np - -import pytest - -from astropy import units as u -from astropy.io import fits -from astropy.nddata import NDData -from astropy.table import Table -from astropy.utils.exceptions import AstropyDeprecationWarning - -from ginga.ColorDist import ColorDistBase - -from ..ginga import ImageWidget, ALLOWED_CURSOR_LOCATIONS - - -def test_load_fits(): # yep - image = ImageWidget() - data = np.random.random([100, 100]) - hdu = fits.PrimaryHDU(data=data) - image.load_fits(hdu) - - -def test_load_nddata(): # yep - image = ImageWidget() - data = np.random.random([100, 100]) - nddata = NDData(data) - image.load_nddata(nddata) - - -def test_load_array(): # yep - image = ImageWidget() - data = np.random.random([100, 100]) - image.load_array(data) - - -def test_center_on(): # yep - image = ImageWidget() - x = 10 - y = 10 - image.center_on((x, y)) - - -def test_offset_to(): - image = ImageWidget() - dx = 10 - dy = 10 - with pytest.warns(AstropyDeprecationWarning): - image.offset_to(dx, dy) - - -def test_offset_by(): # yep - image = ImageWidget() - - # Pixels - image.offset_by(10, 10) - image.offset_by(10 * u.pix, 10 * u.dimensionless_unscaled) - image.offset_by(10, 10 * u.pix) - - # Sky - image.offset_by(1 * u.arcsec, 0.001 * u.deg) - - with pytest.raises(u.UnitConversionError): - image.offset_by(1 * u.arcsec, 1 * u.AA) - - with pytest.raises(ValueError, match='but dy is of type'): - image.offset_by(1 * u.arcsec, 1) - - -def test_zoom_level(): # yep - image = ImageWidget() - image.zoom_level = 5 - assert image.zoom_level == 5 - - -def test_zoom(): # yep - image = ImageWidget() - image.zoom_level = 3 - val = 2 - image.zoom(val) - assert image.zoom_level == 6 - - -@pytest.mark.xfail(reason='Not implemented yet') # Nope, note even sure what this is -def test_select_points(): - image = ImageWidget() - image.select_points() - - -def test_get_selection(): # Um, how is this testing a selection?!? - image = ImageWidget() - marks = image.get_markers() - assert isinstance(marks, Table) or marks is None - - -def test_stop_marking(): # In test_marking_options - image = ImageWidget() - # This is not much of a test... - image.stop_marking(clear_markers=True) - assert image.get_markers() is None - assert image.is_marking is False - - -def test_is_marking(): # In test_marking_options - image = ImageWidget() - assert image.is_marking in [True, False] - with pytest.raises(AttributeError): - image.is_marking = True - - -def test_start_marking(): # In test_marking_options - image = ImageWidget() - - # Setting these to check that start_marking affects them. - image.click_center = True - assert image.click_center - image.scroll_pan = False - assert not image.scroll_pan - - marker_style = {'color': 'yellow', 'radius': 10, 'type': 'cross'} - image.start_marking(marker_name='something', - marker=marker_style) - assert image.is_marking - assert image.marker == marker_style - assert not image.click_center - assert not image.click_drag - - # scroll_pan better activate when marking otherwise there is - # no way to pan while interactively marking - assert image.scroll_pan - - # Make sure that when we stop_marking we get our old - # controls back. - image.stop_marking() - assert image.click_center - assert not image.scroll_pan - - # Make sure that click_drag is restored as expected - image.click_drag = True - image.start_marking() - assert not image.click_drag - image.stop_marking() - assert image.click_drag - - -def test_add_markers(): # In test_marking_options - image = ImageWidget() - table = Table(data=np.random.randint(0, 100, [5, 2]), - names=['x', 'y'], dtype=('int', 'int')) - image.add_markers(table, x_colname='x', y_colname='y', - skycoord_colname='coord') - - -def test_set_markers(): # In test_marking_options - image = ImageWidget() - image.marker = {'color': 'yellow', 'radius': 10, 'type': 'cross'} - assert 'cross' in str(image.marker) - assert 'yellow' in str(image.marker) - assert '10' in str(image.marker) - - -def test_reset_markers(): # In test_marking_options - image = ImageWidget() - # First test: this shouldn't raise any errors - # (it also doesn't *do* anything...) - image.reset_markers() - assert image.get_markers() is None - table = Table(data=np.random.randint(0, 100, [5, 2]), - names=['x', 'y'], dtype=('int', 'int')) - image.add_markers(table, x_colname='x', y_colname='y', - skycoord_colname='coord', marker_name='test') - image.add_markers(table, x_colname='x', y_colname='y', - skycoord_colname='coord', marker_name='test2') - image.reset_markers() - with pytest.raises(ValueError): - image.get_markers(marker_name='test') - with pytest.raises(ValueError): - image.get_markers(marker_name='test2') - - -def test_remove_markers(): # In test_marking_options - image = ImageWidget() - # Add a tag name... - image._marktags.add(image._default_mark_tag_name) - with pytest.raises(ValueError) as e: - image.remove_markers('arf') - assert 'arf' in str(e.value) - - -def test_stretch(): # yep - image = ImageWidget() - with pytest.raises(ValueError) as e: - image.stretch = 'not a valid value' - assert 'must be one of' in str(e.value) - - image.stretch = 'log' - assert isinstance(image.stretch, (ColorDistBase)) - - -def test_cuts(): # yep - image = ImageWidget() - - # An invalid string should raise an error - with pytest.raises(ValueError) as e: - image.cuts = 'not a valid value' - assert 'must be one of' in str(e.value) - - # Setting cuts to something with incorrect length - # should raise an error. - with pytest.raises(ValueError) as e: - image.cuts = (1, 10, 100) - assert 'length 2' in str(e.value) - - # These ought to succeed - - image.cuts = 'histogram' - assert image.cuts == (0.0, 0.0) - - image.cuts = [10, 100] - assert image.cuts == (10, 100) - - -def test_colormap(): # yep - image = ImageWidget() - cmap_desired = 'gray' - cmap_list = image.colormap_options - assert len(cmap_list) > 0 and cmap_desired in cmap_list - - image.set_colormap(cmap_desired) - - -def test_cursor(): # yep - image = ImageWidget() - assert image.cursor in ALLOWED_CURSOR_LOCATIONS - with pytest.raises(ValueError): - image.cursor = 'not a valid option' - image.cursor = 'bottom' - assert image.cursor == 'bottom' - - -def test_click_drag(): # yep - image = ImageWidget() - # Set this to ensure that click_drag turns it off - image._click_center = True - - # Make sure that setting click_drag to False does not turn off - # click_center. - - image.click_drag = False - assert image.click_center - - image.click_drag = True - - assert not image.click_center - - # If is_marking is true then trying to click_drag - # should fail. - image._is_marking = True - with pytest.raises(ValueError) as e: - image.click_drag = True - assert 'Interactive marking' in str(e.value) - - -def test_click_center(): # yep - image = ImageWidget() - assert (image.click_center is True) or (image.click_center is False) - - # Set click_drag True and check that click_center affects it appropriately - image.click_drag = True - - image.click_center = False - assert image.click_drag - - image.click_center = True - assert not image.click_drag - - image.start_marking() - # If marking is in progress then setting click center should fail - with pytest.raises(ValueError) as e: - image.click_center = True - assert 'Cannot set' in str(e.value) - - # setting to False is fine though so no error is expected here - image.click_center = False - - -def test_scroll_pan(): # yep - image = ImageWidget() - - # Make sure scroll_pan is actually settable - for val in [True, False]: - image.scroll_pan = val - assert image.scroll_pan is val - - -def test_save(tmp_path): # yep - image = ImageWidget() - filename = 'woot.png' - image.save(tmp_path / filename) - - -def test_width_height(): # yep - image = ImageWidget(image_width=250, image_height=100) - assert image.image_width == 250 - assert image.image_height == 100 From aa84748c01ec2dbd235523a6df7fadea383c0d13 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:21:43 -0500 Subject: [PATCH 06/14] Make sure value is a tuple --- astrowidgets/interface_definition.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index e6f1464..a36cab5 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -34,7 +34,7 @@ class ImageViewerInterface(Protocol): ALLOWED_CURSOR_LOCATIONS = ('top', 'bottom', None) # List of marker names that are for internal use only - RESERVED_MARKER_SET_NAMES = ('all') + RESERVED_MARKER_SET_NAMES = ('all', ) # The methods, grouped loosely by purpose From 9baba2b5dd4fddfce2f1600ea20a0d5bdaf40e8f Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:21:57 -0500 Subject: [PATCH 07/14] Remove tests that are part of the API test now --- astrowidgets/tests/test_image_widget.py | 157 ------------------------ 1 file changed, 157 deletions(-) diff --git a/astrowidgets/tests/test_image_widget.py b/astrowidgets/tests/test_image_widget.py index c2398a5..4663134 100644 --- a/astrowidgets/tests/test_image_widget.py +++ b/astrowidgets/tests/test_image_widget.py @@ -42,60 +42,6 @@ def _make_fake_ccd(with_wcs=True): return CCDData(data=fake_image, wcs=wcs, unit='adu') -def test_setting_image_width_height(): # yep - image = ImageWidget() - width = 200 - height = 300 - image.image_width = width - image.image_height = height - assert image._viewer.get_window_size() == (width, height) - - -def test_add_marker_does_not_modify_input_table(): # in test_marking_operations - # Regression test for #45 - # Adding markers should not modify the input data table - image = ImageWidget(image_width=300, image_height=300, - pixel_coords_offset=5) - data = np.random.random([300, 300]) - image.load_array(data) - x = [20, 30, 40] - y = [40, 80, 100] - # Create two separate tables for comparison after add_markers. - orig_table = Table(data=[x, y], names=['x', 'y']) - in_table = Table(data=[x, y], names=['x', 'y']) - image.add_markers(in_table) - assert (in_table == orig_table).all() - - -def test_adding_markers_as_world_recovers_with_get_markers(): # yep - """ - Make sure that our internal conversion from world to pixel - coordinates doesn't mess anything up. - """ - 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 - marker_locs = np.random.randint(10, - high=npix_side - 10, - size=(100, 2)) - marks_pix = Table(data=marker_locs, names=['x', 'y']) - marks_world = wcs.all_pix2world(marker_locs, 0) - marks_coords = SkyCoord(marks_world, unit='degree') - mark_coord_table = Table(data=[marks_coords], names=['coord']) - iw.add_markers(mark_coord_table, use_skycoord=True) - result = iw.get_markers() - # Check the x, y positions as long as we are testing things... - np.testing.assert_allclose(result['x'], marks_pix['x']) - np.testing.assert_allclose(result['y'], marks_pix['y']) - np.testing.assert_allclose(result['coord'].ra.deg, - mark_coord_table['coord'].ra.deg) - np.testing.assert_allclose(result['coord'].dec.deg, - mark_coord_table['coord'].dec.deg) - - def test_can_set_pixel_offset_at_object_level(): # The pixel offset below is nonsensical. It is chosen simply # to make it easy to check for. @@ -128,73 +74,6 @@ def test_move_callback_includes_offset(): assert float(y_out) == data_y + offset -def test_can_add_markers_with_names(): # in test_marking_operations? - """ - Test a few things related to naming marker sets - """ - npix_side = 200 - image = ImageWidget(image_width=npix_side, - image_height=npix_side) - x = np.array([20, 30, 40]) - y = np.array([40, 80, 100]) - - # This should succeed without error - image.add_markers(Table(data=[x, y], names=['x', 'y']), - marker_name='nonsense') - - # The name 'nonsense', and nothing else, should be in the - # set of markers. - assert set(['nonsense']) == image._marktags - - # Add more markers with the same name - # This should succeed without error - image.add_markers(Table(data=[x, y], names=['x', 'y']), - marker_name='nonsense') - - # check that we get the right number of markers - marks = image.get_markers(marker_name='nonsense') - assert len(marks) == 6 - - # Make sure setting didn't change the default name - assert image._default_mark_tag_name == 'default-marker-name' - - # Try adding markers without a name - image.add_markers(Table(data=[x, y], names=['x', 'y'])) - assert image._marktags == set(['nonsense', image._default_mark_tag_name]) - - # Delete just the nonsense markers - image.remove_markers('nonsense') - - assert 'nonsense' not in image._marktags - assert image._default_mark_tag_name in image._marktags - - # Add the nonsense markers back... - image.add_markers(Table(data=[x, y], names=['x', 'y']), - marker_name='nonsense') - # ...and now delete all of the markers - image.reset_markers() - # We should have no markers on the image - assert image._marktags == set() - - # Simulate a mouse click and make sure the expected marker - # name has been added. - data_x = 50 - data_y = 50 - image._is_marking = True - image._mouse_click_cb(image._viewer, None, data_x, data_y) - assert image._interactive_marker_set_name in image._marktags - - -def test_mark_with_reserved_name_raises_error(): # in test_marking_operations - npix_side = 200 - image = ImageWidget(image_width=npix_side, - image_height=npix_side) - x = np.array([20, 30, 40]) - y = np.array([40, 80, 100]) - for name in RESERVED_MARKER_SET_NAMES: - with pytest.raises(ValueError): - image.add_markers(Table(data=[x, y], names=['x', 'y']), - marker_name=name) def test_get_marker_with_names(): @@ -258,23 +137,6 @@ def test_unknown_marker_name_error(): assert f"No markers named '{bad_name}'" in str(e.value) -def test_marker_name_has_no_marks_warning(): # in test_marking_operations - """ - 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 @@ -299,22 +161,3 @@ def test_empty_marker_name_works_with_all(): marks = iw.get_markers(marker_name='all') assert len(marks) == len(x) assert 'empty' not in marks['marker name'] - - -def test_add_single_marker(): # in test_marking_operations - """ - Test a few things related to naming marker sets - """ - 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 - marker_locs = np.random.randint(10, - high=npix_side - 10, - size=(100, 2)) - marks_world = wcs.all_pix2world(marker_locs, 0) - marks_coords = SkyCoord(marks_world, unit='degree') - mark_coord_table = Table(data=[marks_coords], names=['coord']) - iw.add_markers(mark_coord_table[0], use_skycoord=True) From e7e5f5cfb1c705af01748a3366a8e98e9c2e08be Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:27:39 -0500 Subject: [PATCH 08/14] Remove unneeded import --- astrowidgets/tests/test_image_widget.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astrowidgets/tests/test_image_widget.py b/astrowidgets/tests/test_image_widget.py index 4663134..13b6cb6 100644 --- a/astrowidgets/tests/test_image_widget.py +++ b/astrowidgets/tests/test_image_widget.py @@ -7,7 +7,7 @@ from astropy.nddata import CCDData from astropy.coordinates import SkyCoord -from ..ginga import ImageWidget, RESERVED_MARKER_SET_NAMES +from ..ginga import ImageWidget def _make_fake_ccd(with_wcs=True): From 9dffe05dbd43f6516277995d67308f76fb82d64d Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 17 May 2023 15:36:55 -0500 Subject: [PATCH 09/14] Fix PEP8 errors --- astrowidgets/tests/test_image_widget.py | 3 --- astrowidgets/tests/test_widget_api_ginga.py | 2 ++ astrowidgets/tests/widget_api_test.py | 10 ++++------ 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/astrowidgets/tests/test_image_widget.py b/astrowidgets/tests/test_image_widget.py index 13b6cb6..be0b848 100644 --- a/astrowidgets/tests/test_image_widget.py +++ b/astrowidgets/tests/test_image_widget.py @@ -5,7 +5,6 @@ from astropy.table import Table, vstack from astropy.wcs import WCS from astropy.nddata import CCDData -from astropy.coordinates import SkyCoord from ..ginga import ImageWidget @@ -74,8 +73,6 @@ def test_move_callback_includes_offset(): assert float(y_out) == data_y + offset - - def test_get_marker_with_names(): # Check a few ways of getting markers out npix_side = 200 diff --git a/astrowidgets/tests/test_widget_api_ginga.py b/astrowidgets/tests/test_widget_api_ginga.py index 4b47a77..041cb52 100644 --- a/astrowidgets/tests/test_widget_api_ginga.py +++ b/astrowidgets/tests/test_widget_api_ginga.py @@ -8,9 +8,11 @@ "available.") from astrowidgets.ginga import ImageWidget # noqa: E402 + def test_instance(): image = ImageWidget() assert isinstance(image, ImageViewerInterface) + class TestGingaWidget(ImageWidgetAPITest): image_widget_class = ImageWidget diff --git a/astrowidgets/tests/widget_api_test.py b/astrowidgets/tests/widget_api_test.py index 2faeac3..8dbdbe4 100644 --- a/astrowidgets/tests/widget_api_test.py +++ b/astrowidgets/tests/widget_api_test.py @@ -219,16 +219,14 @@ def test_add_markers(self): with pytest.raises(ValueError, match='not allowed'): self.image.add_markers(tab, marker_name=name) - # Add markers with no marker name and check we can retrieve them # using the default marker name - self.image.add_markers(tab, x_colname='x', y_colname='y', + self.image.add_markers(tab, x_colname='x', y_colname='y', skycoord_colname='coord') # Don't care about the order of the marker names so use set instead of # list. marknames = self.image._marktags - assert (set(marknames) == - set(['test2', self.image._default_mark_tag_name])) + assert (set(marknames) == set(['test2', self.image._default_mark_tag_name])) # assert (set(self.image.get_marker_names()) == # set(['test2', self.image._default_mark_tag_name])) @@ -267,9 +265,9 @@ def test_adding_markers_as_world(self, data, wcs): np.testing.assert_allclose(result['x'], marks_pix['x']) np.testing.assert_allclose(result['y'], marks_pix['y']) np.testing.assert_allclose(result['coord'].ra.deg, - mark_coord_table['coord'].ra.deg) + mark_coord_table['coord'].ra.deg) np.testing.assert_allclose(result['coord'].dec.deg, - mark_coord_table['coord'].dec.deg) + mark_coord_table['coord'].dec.deg) def test_stretch(self): original_stretch = self.image.stretch From 75bc9dfdb779c859f963244170b89a87b150b348 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 19 Jun 2023 13:27:43 -0500 Subject: [PATCH 10/14] Make constants immutable It seems like these should not, by default, be modifiable by the user. --- astrowidgets/interface_definition.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index a36cab5..59a2160 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -2,10 +2,10 @@ from abc import abstractmethod # Allowed locations for cursor display -ALLOWED_CURSOR_LOCATIONS = ['top', 'bottom', None] +ALLOWED_CURSOR_LOCATIONS = ('top', 'bottom', None) # List of marker names that are for internal use only -RESERVED_MARKER_SET_NAMES = ['all'] +RESERVED_MARKER_SET_NAMES = ('all',) __all__ = [ 'ImageViewerInterface', From 367d61f7123efa87fa0ef282cffc715edb889ccf Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 19 Jun 2023 13:28:19 -0500 Subject: [PATCH 11/14] Use constants from interface definition --- astrowidgets/ginga.py | 9 +++++++-- astrowidgets/interface_definition.py | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/astrowidgets/ginga.py b/astrowidgets/ginga.py index d3cc6ef..44afc7d 100644 --- a/astrowidgets/ginga.py +++ b/astrowidgets/ginga.py @@ -21,6 +21,11 @@ from ginga.web.jupyterw.ImageViewJpw import EnhancedCanvasView from ginga.util.wcs import ra_deg_to_str, dec_deg_to_str +from astrowidgets.interface_definition import ( + ALLOWED_CURSOR_LOCATIONS, + RESERVED_MARKER_SET_NAMES +) + __all__ = ['ImageWidget'] @@ -50,10 +55,10 @@ class ImageWidget(ipyw.VBox): """ # Allowed locations for cursor display - ALLOWED_CURSOR_LOCATIONS = ('top', 'bottom', None) + ALLOWED_CURSOR_LOCATIONS = ALLOWED_CURSOR_LOCATIONS # List of marker names that are for internal use only - RESERVED_MARKER_SET_NAMES = ('all', ) + RESERVED_MARKER_SET_NAMES = RESERVED_MARKER_SET_NAMES def __init__(self, logger=None, image_width=500, image_height=500, pixel_coords_offset=0, **kwargs): diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index 59a2160..e27bcae 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -31,10 +31,10 @@ class ImageViewerInterface(Protocol): # viewer: Any # Allowed locations for cursor display - ALLOWED_CURSOR_LOCATIONS = ('top', 'bottom', None) + ALLOWED_CURSOR_LOCATIONS : tuple = ALLOWED_CURSOR_LOCATIONS # List of marker names that are for internal use only - RESERVED_MARKER_SET_NAMES = ('all', ) + RESERVED_MARKER_SET_NAMES : tuple = RESERVED_MARKER_SET_NAMES # The methods, grouped loosely by purpose From f23ed645ed4a6912accfeddd7793d02d6cd45501 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 19 Jun 2023 13:29:06 -0500 Subject: [PATCH 12/14] Add a few attributes that were missing These are present in the ginga implementation but were omitted here. --- astrowidgets/interface_definition.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index e27bcae..1782086 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -16,7 +16,7 @@ @runtime_checkable class ImageViewerInterface(Protocol): - # This are attributes, not methods. The type annotations are there + # These are attributes, not methods. The type annotations are there # to make sure Protocol knows they are attributes. Python does not # do any checking at all of these types. click_center: bool @@ -25,6 +25,10 @@ class ImageViewerInterface(Protocol): image_width: int image_height: int zoom_level: float + is_marking: bool + stretch_options: tuple + autocut_options: tuple + cursor: str marker: Any cuts: Any stretch: str From fc0056f10258dabdfdc55efeb6f5ecc25155ac90 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 19 Jun 2023 13:29:40 -0500 Subject: [PATCH 13/14] Document all of the methods in the interface --- astrowidgets/interface_definition.py | 144 ++++++++++++++++++++++++++- 1 file changed, 142 insertions(+), 2 deletions(-) diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index 1782086..209650e 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -45,34 +45,110 @@ class ImageViewerInterface(Protocol): # Methods for loading data @abstractmethod def load_fits(self, file): + """ + Load a FITS file into the viewer. + + Parameters + ---------- + file : str or `astropy.io.fits.HDU` + The FITS file to load. If a string, it can be a URL or a + file path. + """ raise NotImplementedError @abstractmethod def load_array(self, array): + """ + Load a 2D array into the viewer. + + Parameters + ---------- + array : array-like + The array to load. + """ raise NotImplementedError @abstractmethod def load_nddata(self, data): + """ + Load an `astropy.nddata.NDData` object into the viewer. + + Parameters + ---------- + data : `astropy.nddata.NDData` + The NDData object to load. + """ raise NotImplementedError # Saving contents of the view and accessing the view @abstractmethod def save(self, filename): + """ + Save the current view to a file. + + Parameters + ---------- + filename : str + The file to save to. The format is determined by the + extension. + """ raise NotImplementedError # Marker-related methods @abstractmethod def start_marking(self, marker_name=None): + """ + Start interactive marking of points on the image. + + Parameters + ---------- + marker_name : str, optional + The name of the marker set to use. If not given, a unique + name will be generated. + """ raise NotImplementedError @abstractmethod def stop_marking(self, clear_markers=False): + """ + Stop interactive marking of points on the image. + + Parameters + ---------- + clear_markers : bool, optional + If `True`, clear the markers that were created during + interactive marking. Default is `False`. + """ raise NotImplementedError @abstractmethod def add_markers(self, table, x_colname='x', y_colname='y', skycoord_colname='coord', use_skycoord=False, marker_name=None): + """ + Add markers to the image. + + Parameters + ---------- + table : `astropy.table.Table` + The table containing the marker positions. + x_colname : str, optional + The name of the column containing the x positions. Default + is ``'x'``. + y_colname : str, optional + The name of the column containing the y positions. Default + is ``'y'``. + skycoord_colname : str, optional + The name of the column containing the sky coordinates. If + given, the ``use_skycoord`` parameter is ignored. Default + is ``'coord'``. + use_skycoord : bool, optional + If `True`, the ``skycoord_colname`` column will be used to + get the marker positions. Default is `False`. + marker_name : str, optional + The name of the marker set to use. If not given, a unique + name will be generated. + """ raise NotImplementedError # @abstractmethod @@ -81,6 +157,9 @@ def add_markers(self, table, x_colname='x', y_colname='y', @abstractmethod def reset_markers(self): + """ + Remove all markers from the image. + """ raise NotImplementedError # @abstractmethod @@ -89,6 +168,15 @@ def reset_markers(self): @abstractmethod def remove_markers(self, marker_name=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. + """ raise NotImplementedError # @abstractmethod @@ -99,17 +187,69 @@ def remove_markers(self, marker_name=None): def get_markers(self, x_colname='x', y_colname='y', skycoord_colname='coord', marker_name=None): + """ + Get the marker positions. + + Parameters + ---------- + x_colname : str, optional + The name of the column containing the x positions. Default + is ``'x'``. + y_colname : str, optional + The name of the column containing the y positions. Default + is ``'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. + + Returns + ------- + table : `astropy.table.Table` + The table containing the marker positions. + """ raise NotImplementedError # Methods that modify the view @abstractmethod - def center_on(self): + def center_on(self, point): + """ + Center the view on the point. + + Parameters + ---------- + tuple or `~astropy.coordinates.SkyCoord` + If tuple of ``(X, Y)`` is given, it is assumed + to be in data coordinates. + """ raise NotImplementedError @abstractmethod - def offset_by(self): + def offset_by(self, dx, dy): + """ + Move the center to a point that is given offset + away from the current center. + + Parameters + ---------- + dx, dy : float or `~astropy.unit.Quantity` + Offset value. Without a unit, assumed to be pixel offsets. + If a unit is attached, offset by pixel or sky is assumed from + the unit. + """ raise NotImplementedError @abstractmethod def zoom(self): + """ + Zoom in or out by the given factor. + + Parameters + ---------- + val : int + The zoom level to zoom the image. + See `zoom_level`. + """ raise NotImplementedError From e9afc485b77f6baf45f0d60e1ed71a0ddea3c0df Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 19 Jun 2023 13:33:05 -0500 Subject: [PATCH 14/14] Satisfy PEP8 bot --- astrowidgets/interface_definition.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/astrowidgets/interface_definition.py b/astrowidgets/interface_definition.py index 209650e..49bff2a 100644 --- a/astrowidgets/interface_definition.py +++ b/astrowidgets/interface_definition.py @@ -35,10 +35,10 @@ class ImageViewerInterface(Protocol): # viewer: Any # Allowed locations for cursor display - ALLOWED_CURSOR_LOCATIONS : tuple = ALLOWED_CURSOR_LOCATIONS + ALLOWED_CURSOR_LOCATIONS: tuple = ALLOWED_CURSOR_LOCATIONS # List of marker names that are for internal use only - RESERVED_MARKER_SET_NAMES : tuple = RESERVED_MARKER_SET_NAMES + RESERVED_MARKER_SET_NAMES: tuple = RESERVED_MARKER_SET_NAMES # The methods, grouped loosely by purpose