From 0c65fab2f878fe9a2b32da5a7cea46b2988507c9 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 14 Jun 2025 17:08:55 -0600 Subject: [PATCH 1/7] Add exceptions to docstrings --- .../interface_definition.py | 74 ++++++++++++++++++- 1 file changed, 71 insertions(+), 3 deletions(-) diff --git a/src/astro_image_display_api/interface_definition.py b/src/astro_image_display_api/interface_definition.py index ae7e289..747282e 100644 --- a/src/astro_image_display_api/interface_definition.py +++ b/src/astro_image_display_api/interface_definition.py @@ -69,6 +69,16 @@ def set_cuts(self, cuts: tuple | BaseInterval, image_label: str | None = None) - image_label : str, optional The label of the image to set the cuts for. If not given and there is only one image loaded, the cuts for that image are set. + + Raises + ------ + TypeError + If the `cuts` parameter is not a tuple or an `astropy.visualization.BaseInterval` + object. + + ValueError + If the `image_label` is not provided when there are multiple images loaded, + or if the `image_label` does not correspond to a loaded image. """ raise NotImplementedError @@ -88,6 +98,12 @@ def get_cuts(self, image_label: str | None = None) -> BaseInterval: ------- cuts : `~astropy.visualization.BaseInterval` The Astropy interval object representing the current cuts. + + Raises + ------ + ValueError + If the `image_label` is not provided when there are multiple images loaded, + or if the `image_label` does not correspond to a loaded image. """ raise NotImplementedError @@ -105,6 +121,15 @@ def set_stretch(self, stretch: BaseStretch, image_label: str | None = None) -> N image_label : str, optional The label of the image to set the cuts for. If not given and there is only one image loaded, the cuts for that image are set. + + Raises + ------ + TypeError + If the `stretch` is not a valid `BaseStretch` object + + ValueError + if the `image_label` is not provided when there are multiple images loaded + or if the `image_label` does not correspond to a loaded image. """ raise NotImplementedError @@ -194,6 +219,11 @@ def save(self, filename: str | os.PathLike, overwrite: bool = False) -> None: overwrite : bool, optional If `True`, overwrite the file if it exists. Default is `False`. + + Raises + ------ + FileExistsError + If the file already exists and `overwrite` is `False`. """ raise NotImplementedError @@ -229,6 +259,13 @@ def load_catalog(self, table: Table, x_colname: str = 'x', y_colname: str = 'y', A dictionary that specifies the style of the markers used to represent the catalog. See `ImageViewerInterface.set_catalog_style` for details. + + Raises + ------ + ValueError + If the `table` does not contain the required columns, or if + the `catalog_label` is not provided when there are multiple + catalogs loaded. """ raise NotImplementedError @@ -262,6 +299,13 @@ def set_catalog_style( ----- The following shapes are supported: "circle", "square", "crosshair", "plus", "diamond". + + Raises + ------ + ValueError + If there are multiple catalog styles set and the user has not + specified a `catalog_label` for which to set the style, or if + an style is set for a catalog that does not exist. """ raise NotImplementedError @@ -270,6 +314,15 @@ def get_catalog_style(self, catalog_label: str | None = None) -> dict: """ Get the style of the catalog markers. + Parameters + ---------- + catalog_label : str, optional + The name of the catalog. If not given and there is + only one catalog loaded, the style for that catalog is returned. + If there are multiple catalogs and no label is provided, an error + is raised. If the label is not does not correspond to a loaded + catalog, an empty dictionary is returned. + Returns ------- dict @@ -281,6 +334,7 @@ def get_catalog_style(self, catalog_label: str | None = None) -> dict: ValueError If there are multiple catalog styles set and the user has not specified a `catalog_label` for which to get the style. + """ raise NotImplementedError @@ -292,8 +346,16 @@ def remove_catalog(self, catalog_label: str | list[str] | None = None) -> None: Parameters ---------- catalog_label : str, optional - The name of the marker set to remove. If the value is ``"all"``, - then all markers will be removed. + The name of the catalog to remove. The value ``'*'`` can be used to + remove all catalogs. If not given and there is + only one catalog loaded, that catalog is removed. + + Raises + ------ + ValueError + If the `catalog_label` is not provided when there are multiple + catalogs loaded, or if the `catalog_label` does not correspond to a + loaded catalog. """ raise NotImplementedError @@ -324,6 +386,11 @@ def get_catalog(self, x_colname: str = 'x', y_colname: str = 'y', table : `astropy.table.Table` The table containing the marker positions. If no markers match the ``catalog_label`` parameter, an empty table is returned. + + Raises + ------ + ValueError + If the `catalog_label` is not provided when there are multiple catalogs loaded. """ raise NotImplementedError @@ -408,6 +475,7 @@ def get_viewport(self, sky_or_pixel: str | None = None, image_label: str | None ------- ValueError If the `sky_or_pixel` parameter is not one of 'sky', 'pixel', or `None`, or if - the `image_label` is not provided when there are multiple images loaded. + the `image_label` is not provided when there are multiple images loaded, or if + the `image_label` does not correspond to a loaded image. """ raise NotImplementedError From 76f5fe974e54e1835aacaf723c414d7e5c4a55f6 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 14 Jun 2025 17:09:19 -0600 Subject: [PATCH 2/7] Change some error types to be more appropriate --- src/astro_image_display_api/widget_api_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index 57484f2..0bb183f 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -616,7 +616,7 @@ def test_remove_catalog_does_not_accept_list(self): self.image.load_catalog(tab, catalog_label='test2', use_skycoord=False) with pytest.raises( - ValueError, + TypeError, match='Cannot remove multiple catalogs from a list' ): self.image.remove_catalog(catalog_label=['test1', 'test2']) @@ -646,7 +646,7 @@ def test_adding_catalog_as_world(self, data, wcs): def test_stretch(self): original_stretch = self.image.get_stretch() - with pytest.raises(ValueError, match=r'Stretch.*not valid.*'): + with pytest.raises(TypeError, match=r'Stretch.*not valid.*'): self.image.set_stretch('not a valid value') # A bad value should leave the stretch unchanged @@ -658,10 +658,10 @@ def test_stretch(self): assert isinstance(self.image.get_stretch(), LogStretch) def test_cuts(self, data): - with pytest.raises(ValueError, match='[mM]ust be'): + with pytest.raises(TypeError, match='[mM]ust be'): self.image.set_cuts('not a valid value') - with pytest.raises(ValueError, match='[mM]ust be'): + with pytest.raises(TypeError, match='[mM]ust be'): self.image.set_cuts((1, 10, 100)) # Setting using histogram requires data From 22771e417195cde1151f64b75a1a384e69096c6e Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 14 Jun 2025 17:18:37 -0600 Subject: [PATCH 3/7] Update dummy viewer to produce correct errors --- src/astro_image_display_api/dummy_viewer.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/astro_image_display_api/dummy_viewer.py b/src/astro_image_display_api/dummy_viewer.py index dfc848c..ecf4bde 100644 --- a/src/astro_image_display_api/dummy_viewer.py +++ b/src/astro_image_display_api/dummy_viewer.py @@ -127,7 +127,7 @@ def get_stretch(self, image_label: str | None = None) -> BaseStretch: def set_stretch(self, value: BaseStretch, image_label: str | None = None) -> None: if not isinstance(value, BaseStretch): - raise ValueError(f"Stretch option {value} is not valid. Must be an Astropy.visualization Stretch object.") + raise TypeError(f"Stretch option {value} is not valid. Must be an Astropy.visualization Stretch object.") image_label = self._resolve_image_label(image_label) if image_label not in self._images: raise ValueError(f"Image label '{image_label}' not found. Please load an image first.") @@ -145,7 +145,7 @@ def set_cuts(self, value: tuple[numbers.Real, numbers.Real] | BaseInterval, imag elif isinstance(value, BaseInterval): self._cuts = value else: - raise ValueError("Cuts must be an Astropy.visualization Interval object or a tuple of two values.") + raise TypeError("Cuts must be an Astropy.visualization Interval object or a tuple of two values.") image_label = self._resolve_image_label(image_label) if image_label not in self._images: raise ValueError(f"Image label '{image_label}' not found. Please load an image first.") @@ -498,7 +498,7 @@ def remove_catalog(self, catalog_label: str | None = None) -> None: then all markers will be removed. """ if isinstance(catalog_label, list): - raise ValueError( + raise TypeError( "Cannot remove multiple catalogs from a list. Please specify " "a single catalog label or use '*' to remove all catalogs." ) From 7c3608f6ae120ee6a47714cb198cba7e0ab805dd Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 14 Jun 2025 17:42:28 -0600 Subject: [PATCH 4/7] Clean up a couple minor things --- src/astro_image_display_api/interface_definition.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/astro_image_display_api/interface_definition.py b/src/astro_image_display_api/interface_definition.py index 747282e..d2a0290 100644 --- a/src/astro_image_display_api/interface_definition.py +++ b/src/astro_image_display_api/interface_definition.py @@ -320,7 +320,7 @@ def get_catalog_style(self, catalog_label: str | None = None) -> dict: The name of the catalog. If not given and there is only one catalog loaded, the style for that catalog is returned. If there are multiple catalogs and no label is provided, an error - is raised. If the label is not does not correspond to a loaded + is raised. If the label is does not correspond to a loaded catalog, an empty dictionary is returned. Returns @@ -339,7 +339,7 @@ def get_catalog_style(self, catalog_label: str | None = None) -> dict: raise NotImplementedError @abstractmethod - def remove_catalog(self, catalog_label: str | list[str] | None = None) -> None: + def remove_catalog(self, catalog_label: str | None = None) -> None: """ Remove markers from the image. @@ -356,6 +356,10 @@ def remove_catalog(self, catalog_label: str | list[str] | None = None) -> None: If the `catalog_label` is not provided when there are multiple catalogs loaded, or if the `catalog_label` does not correspond to a loaded catalog. + + TypeError + If the `catalog_label` is not a string or `None`, or if it is not + one of the allowed values. """ raise NotImplementedError From d2d8c471573c8551fab782e4bc6d34e76c91b9be Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 14 Jun 2025 18:48:52 -0500 Subject: [PATCH 5/7] Update src/astro_image_display_api/interface_definition.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/astro_image_display_api/interface_definition.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/astro_image_display_api/interface_definition.py b/src/astro_image_display_api/interface_definition.py index d2a0290..e1cba0b 100644 --- a/src/astro_image_display_api/interface_definition.py +++ b/src/astro_image_display_api/interface_definition.py @@ -320,7 +320,7 @@ def get_catalog_style(self, catalog_label: str | None = None) -> dict: The name of the catalog. If not given and there is only one catalog loaded, the style for that catalog is returned. If there are multiple catalogs and no label is provided, an error - is raised. If the label is does not correspond to a loaded + is raised. If the label does not correspond to a loaded catalog, an empty dictionary is returned. Returns From 12713d565768fb29034bf37ba98533425bfb14e0 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 14 Jun 2025 17:53:16 -0600 Subject: [PATCH 6/7] Add type clarification --- src/astro_image_display_api/interface_definition.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/astro_image_display_api/interface_definition.py b/src/astro_image_display_api/interface_definition.py index e1cba0b..ac1b787 100644 --- a/src/astro_image_display_api/interface_definition.py +++ b/src/astro_image_display_api/interface_definition.py @@ -1,5 +1,6 @@ from typing import Protocol, runtime_checkable, Any from abc import abstractmethod +import numbers import os from astropy.coordinates import SkyCoord @@ -55,7 +56,7 @@ def load_image(self, data: Any, image_label: str | None = None) -> None: # Setting and getting image properties @abstractmethod - def set_cuts(self, cuts: tuple | BaseInterval, image_label: str | None = None) -> None: + def set_cuts(self, cuts: tuple[numbers.Real, numbers.Real] | BaseInterval, image_label: str | None = None) -> None: """ Set the cuts for the image. From 56bf76f4b608c6ecece841283f17bc143e46aa59 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 14 Jun 2025 18:16:34 -0600 Subject: [PATCH 7/7] More docstring fixees --- src/astro_image_display_api/interface_definition.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/astro_image_display_api/interface_definition.py b/src/astro_image_display_api/interface_definition.py index ac1b787..43e83fd 100644 --- a/src/astro_image_display_api/interface_definition.py +++ b/src/astro_image_display_api/interface_definition.py @@ -120,8 +120,8 @@ def set_stretch(self, stretch: BaseStretch, image_label: str | None = None) -> N `~astropy.visualization.BaseStretch`. image_label : str, optional - The label of the image to set the cuts for. If not given and there is - only one image loaded, the cuts for that image are set. + The label of the image to set the stretch for. If not given and there is + only one image loaded, the stretch for that image are set. Raises ------ @@ -367,7 +367,7 @@ def remove_catalog(self, catalog_label: str | None = None) -> None: @abstractmethod def get_catalog(self, x_colname: str = 'x', y_colname: str = 'y', skycoord_colname: str = 'coord', - catalog_label: str | list[str] | None = None) -> Table: + catalog_label: str | None = None) -> Table: """ Get the marker positions. @@ -382,9 +382,8 @@ def get_catalog(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'``. - catalog_label : 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. + catalog_label : str, optional + The name of the catalog set to get. Returns -------