Update ginga viewer to match AIDA interface#205
Draft
mwcraig wants to merge 14 commits into
Draft
Conversation
Bring the ginga backend up to the same astro-image-display-api (AIDA) standard as the bqplot backend. The widget now inherits ImageViewerLogic and exposes only the AIDA API publicly, plus the interactive extras that ginga supports natively. - Replace the broken ImageViewerInterface.* class attributes (which no longer exist and prevented import) with plain literals. - load_image stores data/WCS via super(), builds a ginga AstroImage, and re-applies the stored viewport, cuts, stretch and colormap. - set_cuts/set_stretch map astropy objects onto ginga cut levels and color algorithms without disturbing the viewport; set_colormap maps the name. - Catalog API draws on the ginga canvas under a tag = str(catalog_label). - Viewport API converts the pixel field of view to a ginga scale as scale = window / fov; get_viewport syncs live ginga pan/zoom into the stored viewport (only when the user actually interacted, keeping programmatic round-trips exact). - Keep interactive extras: cursor, click_center, click_drag, scroll_pan, print_out, image_width/height, and interactive marking wired to the catalog API. - Drop the ginga-native public methods (center_on, offset_by, zoom, add_markers, marker, cuts/stretch properties, ...). - Fix the save() existence check (Path(filename).exists()). Passes the AIDA compliance suite (56 passed, 2 save tests skipped), matching the bqplot backend. Co-written with Claude Opus 4.8
Rebuild example_notebooks/ginga_widget.ipynb around the AIDA API, mirroring bqplot_widget.ipynb cell-for-cell for the shared sections (load_image, set/get_viewport with pixel/SkyCoord/angular fov, cuts/stretch, colormap, catalog load/style/remove including a use_skycoord catalog and the ipywidgets slider demo, get_image/save). Because ginga has a real WCS, the viewport and catalog cells run for real rather than as placeholders. The interactive tail is replaced with working ginga-native demos -- the key-bindings table, live cursor bar, click_center, and start_marking/stop_marking/is_marking with the points retrieved via get_catalog -- instead of bqplot's "not available" notes. Imports now use `from astrowidgets.ginga import ImageWidget`. Verified by executing the notebook headless with no cell errors. Co-written with Claude Opus 4.8
Use ImageViewerLogic.__post_init__ to set up dataclass state instead of hand-rolling the call to _set_up_catalog_image_dicts, drop the stale ipyevents comment, and remove the redundant image_label resolution in load_image (the helpers resolve it internally). Co-written with Claude Opus 4.8
Pass extra keyword arguments through to ipywidgets.VBox so custom widget traits (e.g. layout) are honored. The deprecated use_opencv kwarg is popped first so it still warns rather than reaching VBox as an unknown trait. Co-written with Claude Opus 4.8
Drop the pixel_coords_offset constructor argument, the pixel_offset property, and the associated offset arithmetic in the mouse callbacks and _center_on. The API-approved way to shift coordinates is through the viewport, so this special-case offset is no longer needed. Co-written with Claude Opus 4.8
Replace the direct ginga set_pan calls in _center_on with a call to the public set_viewport, so all re-centering goes through the AIDA viewport bookkeeping (including the WCS handling for SkyCoord arguments). Behavior is unchanged; the new tests are regression coverage. The now-unused SkyCoord import is dropped. Co-written with Claude Opus 4.8
Drop the image_width/image_height setters; the API-approved way to resize the view is through the viewport. The getters are kept because they are used by _viewport_window_size and set_window_size. Co-written with Claude Opus 4.8
Use the ginga viewer's save_rgb_image_as_file renderer instead of writing out the JPEG-encoded widget buffer. This produces a file in the format implied by the extension (e.g. a real PNG for a .png filename) and does not depend on a running browser. The overwrite guard is unchanged. Co-written with Claude Opus 4.8
Replace the open review questions about cuts limits and the colormap None guard with the confirmed answers: get_limits is exact for any interval type, and the colormap legitimately defaults to None. Co-written with Claude Opus 4.8
The viewport conversion already works for a non-square viewer: ginga scales isotropically and both set_viewport and get_viewport use the window width as the single fov reference dimension, so the round-trip is exact regardless of aspect ratio. Document this in _viewport_window_size and add a regression test for a non-square viewer. Co-written with Claude Opus 4.8
Widen the try/except to cover the whole WCS-construction block (header serialization, ginga's header parser, and set_wcs), any of which can fail on a malformed or exotic WCS, so a bad WCS degrades gracefully to displaying the image without one. Keep catching Exception (not BaseException, so Ctrl-C still propagates) but log the traceback via the ginga logger instead of printing only the message. Co-written with Claude Opus 4.8
Ginga's ColorDist classes are parametrized, so a parametrized astropy stretch can be reproduced instead of losing its shape parameter. Inject a fully parametrized distribution via rgbmap.set_dist (set_color_algorithm only ever builds ginga defaults): - AsinhStretch/SinhStretch map exactly onto AsinhDist/SinhDist via factor=1/a, nonlinearity=arcsinh(1/a) (resp. sinh). - LogStretch/PowerDistStretch configure LogDist/PowerDist with exp=a, matching the curve shape to within a quantization level. - SqrtStretch/SquaredStretch/default LinearStretch use the matching parameterless native dist. - Everything ginga lacks a family for -- PowerStretch (x**a, a different family than ginga's 'power' = PowerDistStretch), ContrastBiasStretch, HistEqStretch, composites -- is reproduced exactly by a new _AstropyStretchDist adapter that evaluates the stretch on ginga's 0..1 ramp. This removes the warn-and-fall-back-to-linear path. Tests assert the configured ginga ColorDist's lookup table matches the astropy stretch on the same ramp, that the shape parameter reaches ginga, and that PowerStretch is not mis-mapped to ginga's 'power'. Co-written with Claude Opus 4.8
There was a problem hiding this comment.
Pull request overview
This pull request updates the Ginga-backed ImageWidget to implement the astro-image-display-api (AIDA) interface, aligning behavior with the shared viewer API used across backends and updating examples/tests accordingly.
Changes:
- Refactors
astrowidgets.ginga.ImageWidgetto integrateImageViewerLogicand implement AIDA methods (viewport, cuts/stretch/colormap, catalogs). - Adds a comprehensive Ginga-specific AIDA test suite and removes the prior minimal/legacy test stub.
- Updates the Ginga example notebook to use the AIDA API methods and separates “AIDA API” vs “Ginga-native” interactions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
astrowidgets/ginga.py |
Major refactor to AIDA-compliant implementation + Ginga-specific adapters for stretch/cuts/catalog rendering. |
astrowidgets/tests/test_widget_api_ginga.py |
New AIDA conformance + Ginga-behavior tests for viewport and stretch mapping. |
astrowidgets/tests/_test_widget_api_ginga.py |
Removes old minimal test file superseded by the new suite. |
example_notebooks/ginga_widget.ipynb |
Updates notebook examples to use AIDA API methods and documents Ginga-native interactivity separately. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
159
to
166
| if 'use_opencv' in kwargs: | ||
| # Pop it so it is not forwarded to VBox, which would raise a | ||
| # TraitError for an unknown trait. | ||
| kwargs.pop('use_opencv') | ||
| warnings.warn("use_opencv kwarg has been deprecated--" | ||
| "opencv will be used if it is installed", | ||
| DeprecationWarning) | ||
|
|
Comment on lines
+455
to
+457
| shape = style.get('shape', 'circle') | ||
| color = style.get('color', 'red') | ||
| size = style.get('size', 5) |
| "ra_str = '01h13m23.193s'\n", | ||
| "dec_str = '+00d12m32.19s'\n", | ||
| "frame = 'icrs'\n", | ||
| "frame = 'galactic'\n", |
Comment on lines
+616
to
+620
| " try:\n", | ||
| " w.remove_catalog(catalog_label='stars')\n", | ||
| " except ValueError as exc:\n", | ||
| " raise exc # Catalog doesn't exist\n", | ||
| " \n", |
Member
|
I guess you and the AI better hash things out first before I approve? |
Member
Author
|
Good call -- turns out that the bqplot viewer that I thought was done is definitely not done...lesson being do more local tests in a browser before pushing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request updates the ginga viewer to use the astro-image-display-api. The commit history is a little noisy -- I was using AI to help update the code and wanted small commits I could more easily review -- but I'm happy to squash before merging.
This also contains the broken import that appears in #204, so once this is merged a rebase of #204 should fix that one.