Skip to content
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

Move interface definition from base class to protocol #162

Merged
merged 14 commits into from
Jun 26, 2023
2 changes: 1 addition & 1 deletion astrowidgets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
from ._astropy_init import * # noqa
# ----------------------------------------------------------------------------

from .core import * # noqa
from .ginga import * # noqa
25 changes: 16 additions & 9 deletions astrowidgets/core.py → astrowidgets/ginga.py
Copy link
Member

Choose a reason for hiding this comment

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

I know you say API change is out of scope, but I want to note that we agreed at Astropy Coordination Meeting 2023 to move backend specific implementations (ginga, bqplot, Firefly, etc) out of astrowidgets repo.

https://docs.google.com/document/d/1sBImbw1p4mM3VYWGQJDRbj2N3uqP5Ed_Wfb8BDjJj2o/edit#heading=h.8ah2ugluz5xr

Copy link
Member

Choose a reason for hiding this comment

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

I opened #164 and #165

Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@
from ginga.web.jupyterw.ImageViewJpw import EnhancedCanvasView
from ginga.util.wcs import ra_deg_to_str, dec_deg_to_str

__all__ = ['ImageWidget']

# Allowed locations for cursor display
ALLOWED_CURSOR_LOCATIONS = ['top', 'bottom', None]
from astrowidgets.interface_definition import (
ALLOWED_CURSOR_LOCATIONS,
RESERVED_MARKER_SET_NAMES
)

# List of marker names that are for internal use only
RESERVED_MARKER_SET_NAMES = ['all']
__all__ = ['ImageWidget']


class ImageWidget(ipyw.VBox):
Expand Down Expand Up @@ -55,6 +54,11 @@ class ImageWidget(ipyw.VBox):
correct value to use.*

"""
# Allowed locations for cursor display
ALLOWED_CURSOR_LOCATIONS = ALLOWED_CURSOR_LOCATIONS

# List of marker names that are for internal use only
RESERVED_MARKER_SET_NAMES = RESERVED_MARKER_SET_NAMES

def __init__(self, logger=None, image_width=500, image_height=500,
pixel_coords_offset=0, **kwargs):
Expand Down Expand Up @@ -565,6 +569,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:
Expand Down Expand Up @@ -648,11 +655,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,
Expand Down Expand Up @@ -888,7 +895,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
Expand Down
255 changes: 255 additions & 0 deletions astrowidgets/interface_definition.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,255 @@
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):
# 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.
Comment on lines +20 to +21
Copy link
Member

Choose a reason for hiding this comment

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

We could enforce it with third-party type checker package but is that going too far?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, although it's a bit odd, when I think about it I like the cleanness of the interface not doing any checking but the APITest class doing it. So maybe if we implemented type-checking it could use this information but actually do the type checking when the APITest is invoked by a backend?

Copy link
Member Author

Choose a reason for hiding this comment

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

I should clarify -- I think mypy checking can be done, but Python never enforces type checking at runtime. If we wanted to, or maybe better, if we want an implementation to enforce run-time checking we could use pydantic to make that happen.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we want to enforce run-time checking. Might be too controversial.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed -- and the way we have set things up I don't think we can.

click_center: bool
click_drag: bool
scroll_pan: bool
image_width: int
image_height: int
zoom_level: float
is_marking: bool
stretch_options: tuple
autocut_options: tuple
cursor: str
marker: Any
Copy link
Member Author

Choose a reason for hiding this comment

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

These were omitted the first time through:

zoom_level: float
is_marking: bool
stretch_options: tuple
autocut_options: tuple
cursor: str

cuts: Any
stretch: str
# viewer: Any

# Allowed locations for cursor display
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

# The methods, grouped loosely by purpose

# Methods for loading data
@abstractmethod
def load_fits(self, file):
Copy link
Member

Choose a reason for hiding this comment

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

I know you say API change is out of scope, but I want to note that we agreed at Astropy Coordination Meeting 2023 to change load_* methods to just load so Astrowidgets does not dictate what file format each backend choose to support.

https://docs.google.com/document/d/1sBImbw1p4mM3VYWGQJDRbj2N3uqP5Ed_Wfb8BDjJj2o/edit#heading=h.8ah2ugluz5xr

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks -- I added this to the agenda in the running notes.

Copy link
Member

Choose a reason for hiding this comment

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

I opened #163

"""
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
# def remove_all_markers(self):
# raise NotImplementedError

@abstractmethod
def reset_markers(self):
"""
Remove all markers from the image.
"""
raise NotImplementedError

# @abstractmethod
# def remove_markers_by_name(self, marker_name=None):
# raise NotImplementedError

@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
# def get_all_markers(self):
# raise NotImplementedError

@abstractmethod
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, 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, 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
Loading