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
20 changes: 11 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 @@ -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):
"""
Expand All @@ -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', )
Copy link
Member

Choose a reason for hiding this comment

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

Can these be imported from interface_definition instead of defined here? Or are they meant to be disitnct for some reason I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, they should only be in one place. It feels a little inelegant to do it as constants. Will update.


def __init__(self, logger=None, image_width=500, image_height=500,
pixel_coords_offset=0, **kwargs):
Expand Down Expand Up @@ -565,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:
Expand Down Expand Up @@ -648,11 +650,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 +890,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
111 changes: 111 additions & 0 deletions astrowidgets/interface_definition.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
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.
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
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 = ('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
@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

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, marker_name=None):
raise NotImplementedError

@abstractmethod
def stop_marking(self, clear_markers=False):
raise NotImplementedError

@abstractmethod
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 reset_markers(self):
raise NotImplementedError

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

@abstractmethod
def remove_markers(self, marker_name=None):
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):
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
Loading