From 402d635f00454159d06e51c790fa1ebbc2591d56 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 9 Jul 2025 16:16:38 -0700 Subject: [PATCH 1/4] Change catalog_names to catalog_labels for consistency --- src/astro_image_display_api/api_test.py | 8 ++++---- src/astro_image_display_api/image_viewer_logic.py | 2 +- src/astro_image_display_api/interface_definition.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/astro_image_display_api/api_test.py b/src/astro_image_display_api/api_test.py index ea6501e..a5979ac 100644 --- a/src/astro_image_display_api/api_test.py +++ b/src/astro_image_display_api/api_test.py @@ -79,8 +79,8 @@ def _assert_empty_catalog_table(self, table): assert len(table) == 0 assert sorted(table.colnames) == sorted(["x", "y", "coord"]) - def _get_catalog_names_as_set(self): - marks = self.image.catalog_names + def _get_catalog_labels_as_set(self): + marks = self.image.catalog_labels return set(marks) @pytest.mark.parametrize("load_type", ["fits", "nddata", "array"]) @@ -489,7 +489,7 @@ def test_load_multiple_catalogs(self, catalog): catalog_label="test2", ) - assert sorted(self.image.catalog_names) == ["test1", "test2"] + assert sorted(self.image.catalog_labels) == ["test1", "test2"] # No guarantee markers will come back in the same order, so sort them. t1 = self.image.get_catalog(catalog_label="test1") @@ -513,7 +513,7 @@ def test_load_multiple_catalogs(self, catalog): # other one without a label. self.image.remove_catalog(catalog_label="test1") # Make sure test1 is really gone. - assert self.image.catalog_names == ("test2",) + assert self.image.catalog_labels == ("test2",) # Get without a catalog t2 = self.image.get_catalog() diff --git a/src/astro_image_display_api/image_viewer_logic.py b/src/astro_image_display_api/image_viewer_logic.py index 597a5aa..817872b 100644 --- a/src/astro_image_display_api/image_viewer_logic.py +++ b/src/astro_image_display_api/image_viewer_logic.py @@ -590,7 +590,7 @@ def get_catalog( return result @property - def catalog_names(self) -> tuple[str, ...]: + def catalog_labels(self) -> tuple[str, ...]: return tuple(self._user_catalog_labels()) # Methods that modify the view diff --git a/src/astro_image_display_api/interface_definition.py b/src/astro_image_display_api/interface_definition.py index b951ef1..010bc78 100644 --- a/src/astro_image_display_api/interface_definition.py +++ b/src/astro_image_display_api/interface_definition.py @@ -543,7 +543,7 @@ def get_catalog( @property @abstractmethod - def catalog_names(self) -> tuple[str, ...]: + def catalog_labels(self) -> tuple[str, ...]: """ Names of the loaded catalogs. From 87b0116ef91edeea78212546505ae5605320345d Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 9 Jul 2025 16:22:02 -0700 Subject: [PATCH 2/4] Clarify that loading a catalog for an existing catalog replaces the data --- src/astro_image_display_api/api_test.py | 2 +- src/astro_image_display_api/image_viewer_logic.py | 15 +++------------ .../interface_definition.py | 3 +++ 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/astro_image_display_api/api_test.py b/src/astro_image_display_api/api_test.py index a5979ac..9e97c92 100644 --- a/src/astro_image_display_api/api_test.py +++ b/src/astro_image_display_api/api_test.py @@ -534,7 +534,7 @@ def test_load_catalog_multiple_same_label(self, catalog): self.image.load_catalog(catalog, catalog_label="test1") retrieved_catalog = self.image.get_catalog(catalog_label="test1") - assert len(retrieved_catalog) == 2 * len(catalog) + assert len(retrieved_catalog) == len(catalog) def test_load_catalog_with_skycoord_no_wcs(self, catalog, data): # Check that loading a catalog with skycoord but no x/y and diff --git a/src/astro_image_display_api/image_viewer_logic.py b/src/astro_image_display_api/image_viewer_logic.py index 817872b..619df85 100644 --- a/src/astro_image_display_api/image_viewer_logic.py +++ b/src/astro_image_display_api/image_viewer_logic.py @@ -9,7 +9,7 @@ from astropy import units as u from astropy.coordinates import SkyCoord from astropy.nddata import CCDData, NDData -from astropy.table import Table, vstack +from astropy.table import Table from astropy.units import Quantity from astropy.visualization import ( AsymmetricPercentileInterval, @@ -509,17 +509,8 @@ def load_catalog( catalog_label = self._resolve_catalog_label(catalog_label) - # Either set new data or append to existing data - if ( - catalog_label in self._catalogs - and self._catalogs[catalog_label].data is not None - ): - # If the catalog already exists, we append to it - old_table = self._catalogs[catalog_label].data - self._catalogs[catalog_label].data = vstack([old_table, to_add]) - else: - # If the catalog does not exist, we create a new one - self._catalogs[catalog_label].data = to_add + # Set the new data + self._catalogs[catalog_label].data = to_add # Ensure a catalog always has a style if catalog_style is None: diff --git a/src/astro_image_display_api/interface_definition.py b/src/astro_image_display_api/interface_definition.py index 010bc78..160dabd 100644 --- a/src/astro_image_display_api/interface_definition.py +++ b/src/astro_image_display_api/interface_definition.py @@ -345,6 +345,9 @@ def load_catalog( """ Add catalog entries to the viewer at positions given by the catalog. + Loading a catalog using a ``catalog_label`` that already exists will + overwrite the existing catalog with the new one. + Parameters ---------- table : `astropy.table.Table` From ab383e153a01465175334ec43b7f7f2a612be123 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Fri, 11 Jul 2025 10:19:18 -0700 Subject: [PATCH 3/4] Use only valid marker shapes in test --- src/astro_image_display_api/api_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/astro_image_display_api/api_test.py b/src/astro_image_display_api/api_test.py index 9e97c92..09fa2f0 100644 --- a/src/astro_image_display_api/api_test.py +++ b/src/astro_image_display_api/api_test.py @@ -362,7 +362,7 @@ def test_set_catalog_style_before_catalog_data_raises_error(self): with pytest.raises( ValueError, match="Must load a catalog before setting a catalog style" ): - self.image.set_catalog_style(color="red", shape="x", size=10) + self.image.set_catalog_style(color="red", shape="circle", size=10) def test_set_get_catalog_style_no_labels(self, catalog): # Check that getting without setting returns a dict that contains @@ -376,7 +376,7 @@ def test_set_get_catalog_style_no_labels(self, catalog): # Add some data before setting a style self.image.load_catalog(catalog) # Check that setting a marker style works - marker_settings = dict(color="red", shape="x", size=10) + marker_settings = dict(color="red", shape="crosshair", size=10) self.image.set_catalog_style(**marker_settings.copy()) retrieved_style = self.image.get_catalog_style() @@ -421,7 +421,7 @@ def test_set_get_catalog_style_preserves_extra_keywords(self, catalog): # The only required keywords are color, shape, and size. # Add some extra keyword to the style. style = dict( - color="blue", shape="x", size=10, extra_kw="extra_value", alpha=0.5 + color="blue", shape="circle", size=10, extra_kw="extra_value", alpha=0.5 ) self.image.set_catalog_style(**style.copy()) @@ -605,7 +605,7 @@ def test_load_catalog_with_no_style_has_a_style(self, catalog): def test_load_catalog_with_style_sets_style(self, catalog): # Check that loading a catalog with a style sets the style # for that catalog. - style = dict(color="blue", shape="x", size=10) + style = dict(color="blue", shape="square", size=10) self.image.load_catalog( catalog, catalog_label="test1", catalog_style=style.copy() ) From 711b66422f1343de7d677d199802ee63780fcac3 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Fri, 11 Jul 2025 10:24:23 -0700 Subject: [PATCH 4/4] Fix illogical test This had tested what would happen if you wanted to use x/y for a catalog but had no x/y in the catalog table and didn't expect that to raise an error. --- src/astro_image_display_api/api_test.py | 11 ++++----- .../image_viewer_logic.py | 23 +++++++++++++------ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/astro_image_display_api/api_test.py b/src/astro_image_display_api/api_test.py index 09fa2f0..ba2e6e3 100644 --- a/src/astro_image_display_api/api_test.py +++ b/src/astro_image_display_api/api_test.py @@ -543,13 +543,10 @@ def test_load_catalog_with_skycoord_no_wcs(self, catalog, data): # Remove x/y columns from the catalog del catalog["x", "y"] - self.image.load_catalog(catalog) - # Retrieve the catalog and check that the x and y columns are None - retrieved_catalog = self.image.get_catalog() - assert "x" in retrieved_catalog.colnames - assert "y" in retrieved_catalog.colnames - assert all(rc is None for rc in retrieved_catalog["x"]) - assert all(rc is None for rc in retrieved_catalog["y"]) + with pytest.raises( + ValueError, match="Cannot use pixel coordinates without pixel columns" + ): + self.image.load_catalog(catalog) def test_load_catalog_with_use_skycoord_no_skycoord_no_wcs(self, catalog, data): # Check that loading a catalog with use_skycoord=True but no diff --git a/src/astro_image_display_api/image_viewer_logic.py b/src/astro_image_display_api/image_viewer_logic.py index 619df85..96cc993 100644 --- a/src/astro_image_display_api/image_viewer_logic.py +++ b/src/astro_image_display_api/image_viewer_logic.py @@ -492,9 +492,16 @@ def load_catalog( x, y = self._wcs.world_to_pixel(coords) to_add[x_colname] = x to_add[y_colname] = y + xy = (x, y) else: to_add[x_colname] = to_add[y_colname] = None + if not use_skycoord and xy is None: + raise ValueError( + "Cannot use pixel coordinates without pixel columns or both " + "coordinates and a WCS." + ) + if coords is None: if use_skycoord and self._wcs is None: raise ValueError( @@ -703,13 +710,15 @@ def get_viewport( "sky coordinates." ) else: - center = viewport.wcs.pixel_to_world( - viewport.center[0], viewport.center[1] - ) - pixel_scale = proj_plane_pixel_scales(viewport.wcs)[ - viewport.largest_dimension - ] - fov = pixel_scale * viewport.fov * u.degree + if center is None: + center = viewport.wcs.pixel_to_world( + viewport.center[0], viewport.center[1] + ) + if fov is None: + pixel_scale = proj_plane_pixel_scales(viewport.wcs)[ + viewport.largest_dimension + ] + fov = pixel_scale * viewport.fov * u.degree else: # Pixel coordinates if isinstance(viewport.center, SkyCoord):