Move interface definition from base class to protocol #162

mwcraig · 2023-05-17T20:29:56Z

This PR contains the minimal pieces from #142 to change the bottom layer from an abstract base class to a protocol.

There are some API changes in #142 that are not included in this PR so that we can discuss those separately.

EDIT:

xref Do not merge: Plain text description of the API #147

mwcraig · 2023-05-17T20:33:15Z

This also contains the minimal ginga implementation, in part to illustrate how tests would be set up in other implements. See the ginga-specific test code

pllim · 2023-05-17T21:28:58Z

Thanks! I'll put this on my queue to review.

pllim · 2023-06-16T03:30:03Z

astrowidgets/interface_definition.py

+
+    # Methods for loading data
+    @abstractmethod
+    def load_fits(self, file):


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

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

I opened #163

pllim · 2023-06-16T03:31:57Z

astrowidgets/ginga.py

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

I opened #164 and #165

pllim

Looking at Astropy Coordination Meeting 2023 notes, there is a note that says @eteq was going to "split that out into two classes on the train" but I forgot what that was about. Do you remember? Could it be the "DataLoader protocol"?

pllim · 2023-06-16T13:45:28Z

astrowidgets/interface_definition.py

+    # to make sure Protocol knows they are attributes. Python does not
+    # do any checking at all of these types.


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

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?

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.

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

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

eteq

I like where this is going! I left an inline comment and I have two more general comments below. But I'm approving because all of them might be "lets leave that to a later PR" if you want, @mwcraig .

Right now the API methods don't provide any context about what they are supposed to do. I'm thinking the interface is now the best place to provide that. For example, load_fits might get a docstring that says something like "The UI behavior expected is that the specified FITS file appear in the UI window in addition to being loaded into the application." Even though we can't guarantee that from an automated testing point of view, we can at least say in the docstring what the API is supposed to do from a UI perspective, to make it easier for a user to report variances from a backend to the expected behavior.
Maybe we should use type hinting in the methods to also show what each of the methods return? I don't think we need to go full-on with the type-hinting, but as simply a documentation mechanism of the expected return type it might be useful.

eteq · 2023-06-16T13:47:56Z

astrowidgets/ginga.py

+    # 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', )


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

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

eteq · 2023-06-16T14:43:02Z

astrowidgets/interface_definition.py

+    # to make sure Protocol knows they are attributes. Python does not
+    # do any checking at all of these types.


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?

pllim

In principle, I think Protocol is a great idea and we should push this forward. Let's get this in. Thanks!

Matt, in case if you want to address Erik comments, I won't merge now. Feel free to merge when you think it is ready.

mwcraig · 2023-06-19T15:04:03Z

Maybe we should use type hinting in the methods to also show what each of the methods return? I don't think we need to go full-on with the type-hinting, but as simply a documentation mechanism of the expected return type it might be useful.

May as well give it a go at a stage when the whole thing is typing. Pretty sure that isinstance won't catch that since it is a runtime check but should allow mypy checking.

mwcraig · 2023-06-19T15:07:06Z

Right now the API methods don't provide any context about what they are supposed to do. I'm thinking the interface is now the best place to provide that. For example, load_fits might get a docstring that says something like "The UI behavior expected is that the specified FITS file appear in the UI window in addition to being loaded into the application." Even though we can't guarantee that from an automated testing point of view, we can at least say in the docstring what the API is supposed to do from a UI perspective, to make it easier for a user to report variances from a backend to the expected behavior.

Going to implement that in this PR because I know from experience I will never come back to it...

It seems like these should not, by default, be modifiable by the user.

These are present in the ginga implementation but were omitted here.

mwcraig · 2023-06-19T18:30:41Z

astrowidgets/interface_definition.py

+    # to make sure Protocol knows they are attributes. Python does not
+    # do any checking at all of these types.


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

mwcraig · 2023-06-19T18:31:21Z

astrowidgets/interface_definition.py

+    stretch_options: tuple
+    autocut_options: tuple
+    cursor: str
+    marker: Any


These were omitted the first time through:

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

mwcraig · 2023-06-26T14:30:34Z

Merging, thanks!

mwcraig added 8 commits May 17, 2023 12:27

Minimally disruptive change to Protocol

62f6b97

Make constants class variables

a8f7cbd

Fix logic error if there are no tables

e735a23

Switch back to original marker API and add arguments to methods

38cdcd7

Remove old API tests

9ad7797

Make sure value is a tuple

aa84748

Remove tests that are part of the API test now

9baba2b

Remove unneeded import

e7e5f5c

mwcraig added the enhancement New feature or request label May 17, 2023

mwcraig requested review from pllim and eteq May 17, 2023 20:30

Fix PEP8 errors

9dffe05

pllim reviewed Jun 16, 2023

View reviewed changes

This was referenced Jun 16, 2023

Widget should not inherit from VBox #132

Closed

Keep "implementation details" out of the top-level API #13

Closed

Refactoring to an abstract interface and concrete implementations #93

Closed

API design for choosing between multiple backends #122

Closed

eteq approved these changes Jun 16, 2023

View reviewed changes

This was referenced Jun 16, 2023

Work towards the first stable release #119

Closed

Initial implementation of abstract class #126

Closed

Initial draft of interface definition #142

Draft

pllim added the API Issues that relate to the API itself rather than implementations label Jun 16, 2023

This was referenced Jun 16, 2023

File loading: Just have one "load" method and let backend decide on its own what to load #163

Open

Move backend specific implementations out of this repo #164

Open

pllim approved these changes Jun 16, 2023

View reviewed changes

mwcraig mentioned this pull request Jun 19, 2023

Add more types to the protocol #167

Closed

mwcraig added 4 commits June 19, 2023 13:27

Make constants immutable

75bc9df

It seems like these should not, by default, be modifiable by the user.

Use constants from interface definition

367d61f

Add a few attributes that were missing

f23ed64

These are present in the ginga implementation but were omitted here.

Document all of the methods in the interface

fc0056f

mwcraig commented Jun 19, 2023

View reviewed changes

Satisfy PEP8 bot

e9afc48

mwcraig merged commit 210d772 into astropy:main Jun 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move interface definition from base class to protocol #162

Move interface definition from base class to protocol #162

mwcraig commented May 17, 2023 •

edited by pllim

Loading

mwcraig commented May 17, 2023

pllim commented May 17, 2023

pllim Jun 16, 2023

mwcraig Jun 16, 2023

pllim Jun 16, 2023

pllim Jun 16, 2023

pllim Jun 16, 2023

pllim left a comment •

edited

Loading

pllim Jun 16, 2023

eteq Jun 16, 2023

mwcraig Jun 19, 2023

pllim Jun 19, 2023

mwcraig Jun 19, 2023

eteq left a comment

eteq Jun 16, 2023

mwcraig Jun 19, 2023

eteq Jun 16, 2023

pllim left a comment

mwcraig commented Jun 19, 2023

mwcraig commented Jun 19, 2023

mwcraig Jun 19, 2023

mwcraig Jun 19, 2023

mwcraig commented Jun 26, 2023

		# to make sure Protocol knows they are attributes. Python does not
		# do any checking at all of these types.

Move interface definition from base class to protocol #162

Move interface definition from base class to protocol #162

Conversation

mwcraig commented May 17, 2023 • edited by pllim Loading

mwcraig commented May 17, 2023

pllim commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eteq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

mwcraig commented Jun 19, 2023

mwcraig commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwcraig commented Jun 26, 2023

mwcraig commented May 17, 2023 •

edited by pllim

Loading

pllim left a comment •

edited

Loading