Initial draft of interface definition

[mwcraig]

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

• Setting cursor does not change where it is located.
• Centering on SkyCoord does not properly center image -- actually it does, but coordinate in the test notebook needs to be in Galactic coordinates to match the image WCS.
• Setting the zoom_level to 1 recenters the viewer and moves the center out of the FOV
• Saving the image downloads it in your browser but does not save it in the working directory. May need a bit of javascript to make this happen?
• stretch_options is an attribute error
• Changing the stretch does not change the display
• autocut_options is an attribute error
• The __str__ for cuts is just the __repr__ for the underlying astropy Interval
• Setting cuts to a tuple fails
• Setting cuts to a name (e.g. zscale) fails
• Setting click_center does not actually do anything -- clicking does not center
• start_marking fails
• add_markers fails
• print_out is not implemented
• all markers with the same name have the same symbol.
• Warning from traitlets when drawing markers

[mwcraig]

@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]

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

[mwcraig]

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

[pllim]

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]

WIP in case my local computer combusts 😹

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

[mwcraig]

Hmm there are conflicts for some reason.

Fixed, thanks for catching

[mwcraig]

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]

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]

[pllim]

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

[mwcraig]

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.

[pllim]

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]

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]

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

[eteq]

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]

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)

[mwcraig]

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

[eteq]

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]

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

[pllim]

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.