Initial draft of interface definition #142

mwcraig · 2021-07-02T20:34:37Z

This is the beginning of an alternative to #126 using typing.Protocol to define the interface instead of an abstract base class. The test at https://github.com/astropy/astrowidgets/compare/main...mwcraig:interface-definition?expand=1#diff-06b6503d76a97db0c1f54a2065996b518f9e862f9f2fcd158fb3a04c2963d321R17 effectively checks that all of the required attributes and/or methods are present.

This does NOT include the changes in the API introduced in #126 yet (that adds a viewer attribute and renames some of the marker-related methods) or the change in #140 (renaming offset_to to offset_by and changing the behavior).

I'm leaning towards a protocol instead of abstract class because it seems to maximize flexibility: with no changes to the current ImageWidget class at all, or changes in how it subclasses, we can verify that it, at least in principle, satisfies the protocol. Different backends can subclass from whatever they want but still promise to deliver the interface.

Different implementations can choose different routes for representing the attributes and that is fine. For example, an ipywidgets-based implementation will make many of the attributes traitlets so that they can be linked together to provide synced viewers. A glue-based implementation may have some other method, and some other implementation may implement them as properties with setters.

Todo

Update interface to match changes in Initial implementation of abstract class #126 and New offset_by with new API #140. Still missing: Several methods in the Protocol related to markers are either commented out or need to be revised.
Update/rename/move the current ginga backend along the lines of what is done in Initial implementation of abstract class #126.
Decide whether the interface stays in this repo (whether it is a Protocol or an ABC is irrelevant to this) or moves to its own, separately versioned repo.
Decide whether to use a Protocol or an ABC.

Fix remaining bqplot test failures and xfails/skips:

test_cuts fails because histogram isn't in autocut_options...not sure it needs to be or what it is (there is not histogram astropy Interval)
test_marking_operations fails because the bqplot version only adds a marker name if there are markers, and removes the name if there are no markers for it, so it always raises a ValueError for no markers. The ginga version warns if a marker name is in the list of marker names but there are no markers with that name.
Currently the bqplot viewer skips the save test because saving is done on the Javascript side.
Although the scroll_pan test passes there is no way to make bqplot actually scroll_pan that I have found yet.
click_center does not actually work even though the test passes
Initial zoom_level is 0.

Fix these GUI issues in bqplot viewer

mwcraig · 2021-07-02T20:47:01Z

@pllim @eteq -- this is clearly incomplete but I'm curious to get your reaction. It turns out that a Protocol can also be used as a superclass so the pattern ImageWidget(ImageWidgetInterface) is permitted but provides the option of not subclassing. That might help address the concerns raised by @astrofrog in #132 and by @gpdf in #93.

pllim · 2021-07-02T20:49:09Z

This is the first I have heard of Protocol, so I have no strong opinions other than it looks very TypeScripty. 😹

mwcraig · 2021-07-02T20:52:04Z

I only heard about Protocol a few days ago -- this may be a case of being overly enthusiastic about something shiny and new...

pllim · 2021-07-02T20:54:24Z

There is a fine line between being too strict or too flexible. Too strict, you will have a hard time adopting new backend. Too flexible, people cannot swap backend without everything breaking. But then again, at a glance, it is hard to see just how comparable this is to abstract class.

How about pushing on with some real implementation of a few important methods, so we can compare?

pllim · 2021-07-19T14:48:07Z

WIP in case my local computer combusts 😹

p.s. Hmm there are conflicts for some reason.

mwcraig · 2021-07-19T16:22:11Z

Hmm there are conflicts for some reason.

Fixed, thanks for catching

mwcraig · 2021-07-19T16:25:35Z

This has a mostly-implemented bqplot-based viewer. There are several things missing (displaying cursor position, start/stop marking, scroll_pan, likely more) but it is far enough along to get a feel for it.

Whether we end up providing a bqplot viewer, it was useful to try to implement this with a viewer that is not at all astronomy-focused. I hadn't realized how much the current reference implementation depends on the astro-specific nature of ginga.

Of the unimplemented items there are two things I don't know how to do:

Keep the markers a fixed size on the image (if we want that)
Make scroll-pan happen.

I did try this with 500k markers and it was fine when using bqplot's ScatterGL.

pllim · 2021-07-23T15:32:33Z

How do you get the bqplot image widget to display? I cannot get it to work and I don't see any example notebook for bqplot backend here.

pllim · 2021-07-23T15:35:44Z

astrowidgets/bqplot.py

pllim · 2021-07-23T15:46:27Z

astrowidgets/bqplot.py

+RESERVED_MARKER_SET_NAMES = ['all']
+
+
+class _AstroImage(ipw.VBox):


It is really confusing to have the API split between _AstroImage and the official ImageWidget.

I was trying to put as little as possible in ImageWidget that depended on the details of the backend implementation in the hope that we could factor some backend-indpenedent stuff out. The dividing line between the two got a little unclear, though.

Yeah... At some point in my past effort, I just ended up not splitting them but rather hide the backend specific stuff with leading underscore. 😹

pllim · 2021-07-23T15:54:17Z

A bit more luck showing something by accessing a hidden attribute but I can't see the data even after trying to change stretch and/or cuts. Zoom seems to do something and I can scroll to zoom in/out on it. Here is the default display:

mwcraig · 2021-07-26T22:12:50Z

Not sure what the issues were -- I'll have a chance to do more work on this tomorrow afternoon.

eteq · 2021-07-28T18:38:19Z

A few reactions:

In concept I really like this. (Although I might also be falling into the "new and shiny" trap...) I do very much like the fact that this will make it a lot easier for backend implementers to have their astrowidgets backend be the same class as their "normal" object if they want to. It's technically also possible with a abstract class like #126, but I think this is better in that there's less to go wrong in initialization, etc, given that widget implementations already be embedded in complex class hierarchies. And as #126 shows, we really are more interested in astrowidgets as an interface API than a "is a/has a" relationship that subclasses are nominally supposed to follow.

Now the downside: the Protocol machinery itself really only is about type/name-matching. And for astrowidgets, I think we're at least as interested in behavior-matching. Abstract classes give more opportunities to check "behavior".

So what I think I settle on is I like this but only with the addition of some clear machinery that's basically "the tests to run to see if you match the interface". Ideally this would be as simple as telling backend implementers write a unit test that looks like this:

def test_api():
    astrowidgets.<somewhere>.test_backend(instance_of_class_that_implements_the_backend)

or similar, which basically does what this PR's two api test suites do. Of course that still doesn't check that a UI has the correct behvaior, but it's probably just too complex for us to implement any sort of meaningful generic UI-testing tool.

eteq · 2021-07-28T18:40:51Z

astrowidgets/tests/test_api_ginga.py

+
+def test_consistent_interface():
+    iw = ImageWidget()
+    assert isinstance(iw, ImageViewerInterface)


Does this actually do the test of whether the API matches with no code changes needed in the ginga object at all? If so, that's very magical! (in a good way)

Yes -- it does not check signatures, etc, but it makes sure that all of the properties/methods are there.

eteq · 2021-07-28T21:12:56Z

In some out-of-band discussion, @mwcraig, @pllim, and I concluded we want to go in this direction at least to start with over a abstract/subclass approach like that from #126. But some of the machinery from #126 should be ported over here - especially the documentation of the API/intent and the testing framework from there.

pllim · 2021-08-04T22:14:51Z

astrowidgets/bqplot.py

+        self._image.scales['image'].colors = colors
+
+    def save_png(self, filename):
+        self._figure.save_png(filename)


This actually pops up a file dialog, not quite what we want?

TST: Update test matrix and added a test. DOC: Re-organized doc and notebooks. DOC: Update example notebooks

DOC: Cannot use automodapi for Ginga API because we need to inherit docstring. DOC: Do not display inherited members because too confusing. [ci skip]

The base class is gone now that we are switching to a Protocol instead

pllim · 2023-06-16T15:14:05Z

The underlying design has been superseded by #162 but I am leaving this open in case there are some stuff here you still want to move to follow-up PRs after that one. Turning this into draft for now.

mwcraig force-pushed the interface-definition branch from edf546e to 41d3335 Compare July 17, 2021 20:17

mwcraig force-pushed the interface-definition branch from 41d3335 to 1e71af4 Compare July 19, 2021 15:09

mwcraig mentioned this pull request Jul 19, 2021

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

Draft

mwcraig force-pushed the interface-definition branch from 1e71af4 to dccf354 Compare July 19, 2021 16:21

pllim reviewed Jul 23, 2021

View reviewed changes

astrowidgets/bqplot.py Outdated Show resolved Hide resolved

pllim reviewed Jul 23, 2021

View reviewed changes

eteq reviewed Jul 28, 2021

View reviewed changes

eteq mentioned this pull request Jul 28, 2021

Initial implementation of abstract class #126

Closed

7 tasks

mwcraig force-pushed the interface-definition branch from 19cfcb0 to 24cb2cf Compare July 28, 2021 22:50

pllim mentioned this pull request Jul 29, 2021

Markers API refactor #145

Closed

Jdaviz-Triage-Bot mentioned this pull request Jul 29, 2021

Imviz astrowidgets API sync with updated API upstream spacetelescope/jdaviz#751

Open

pllim mentioned this pull request Aug 2, 2021

Imviz: Add zoom and zoom_level API spacetelescope/jdaviz#744

Merged

4 tasks

pllim reviewed Aug 4, 2021

View reviewed changes

mwcraig force-pushed the interface-definition branch from fd2f284 to 07c047d Compare August 27, 2021 15:18

mwcraig mentioned this pull request May 15, 2025

Coordinates in cursor display may not be RA/Dec astropy/astro-image-display-api#26

Open

mwcraig added 2 commits May 4, 2023 19:25

Initial draft on interface definition

4b18213

Minimal change implementation of concrete ginga viewer

9ee09df

pllim and others added 23 commits May 4, 2023 19:38

DOC: Update user-facing doc.

1e9962e

TST: Update test matrix and added a test. DOC: Re-organized doc and notebooks. DOC: Update example notebooks

DOC: Fix inheritance diagram for Ginga API.

33799e3

DOC: Cannot use automodapi for Ginga API because we need to inherit docstring. DOC: Do not display inherited members because too confusing. [ci skip]

Allow the image widget to detect GUI zoom changes

bedd9f6

Add click_center functionality

c8cd3cf

Respond to changes in cursor value

da5ac8e

Fix bug in setting up stretch

5c47658

Fix errors in marking setup

0c5048a

Handle case when user does not specify marker name in add_markers

a7e9384

Add click-to-mark functionality

584ebf5

WIP bqplot widget notebook

3de921a

Propogate marker API changes

838d027

Use proper numpy type names

da4fd3f

Finish most of API implementation

a196d29

Clean up tests

bf3f0bb

Remove cell outputs in sample notebook

5fd58a3

Add traitlet/widget related warnings to ignore list

9b747ff

Restore a couple of constants that got los in the refactor

10abb5e

Remove base class tests

92c2eb5

The base class is gone now that we are switching to a Protocol instead

Remove pixel offset from ginga widget

2f3bd38

Update marker method names in test

cccd747

Replace cut test with something sensible

8bc7c3d

Update more marker tests

5be93fb

Add widget arguments to ginga

82a7844

mwcraig force-pushed the interface-definition branch from 6f14f09 to 82a7844 Compare May 5, 2023 00:50

This was referenced May 17, 2023

Move interface definition from base class to protocol #162

Merged

API changes to marker methods mwcraig/astrowidgets#1

Open

pllim mentioned this pull request Jun 5, 2023

Imviz: Maybe we should not use Subset at all? spacetelescope/jdaviz#2238

Open

pllim marked this pull request as draft June 16, 2023 15:13

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

		RESERVED_MARKER_SET_NAMES = ['all']


		class _AstroImage(ipw.VBox):

Uh oh!

Initial draft of interface definition #142

Are you sure you want to change the base?

Initial draft of interface definition #142

Uh oh!

Conversation

mwcraig commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Fix remaining bqplot test failures and xfails/skips:

Fix these GUI issues in bqplot viewer

Uh oh!

mwcraig commented Jul 2, 2021

Uh oh!

pllim commented Jul 2, 2021

Uh oh!

mwcraig commented Jul 2, 2021

Uh oh!

pllim commented Jul 2, 2021

Uh oh!

pllim commented Jul 19, 2021

Uh oh!

mwcraig commented Jul 19, 2021

Uh oh!

mwcraig commented Jul 19, 2021

Uh oh!

pllim commented Jul 23, 2021

Uh oh!

pllim commented Jul 23, 2021

Uh oh!

Uh oh!

pllim Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

mwcraig Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Jul 23, 2021

Uh oh!

mwcraig commented Jul 26, 2021

Uh oh!

eteq commented Jul 28, 2021

Uh oh!

eteq Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

mwcraig Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

eteq commented Jul 28, 2021

Uh oh!

pllim Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

pllim commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwcraig commented Jul 2, 2021 •

edited

Loading

pllim Jul 27, 2021 •

edited

Loading