jdaviz icon indicating copy to clipboard operation
jdaviz copied to clipboard

Astrowidgets API: Markers to use Astropy regions

Open pllim opened this issue 11 months ago • 2 comments

Description

This pull request is to explore what if markers use Astropy regions instead of Astropy Table. There is no expectation for this to be merged until more backends are explored and then API is finalized upstream in https://github.com/astropy/astrowidgets

Screenshot 2023-09-06 155420

Related issues:

  • https://github.com/astropy/astrowidgets/issues/137
  • https://github.com/astropy/astrowidgets/issues/51

Pros:

  • Markers no longer cause data tables to be added to app data collection. They are now pure bqplot figure marks. This probably also improved performance.
  • Linking can now change link type with or without markers. This also means we can now get rid of MarkersChangedMessage.
  • When you zoom in now, marker gets larger. That is, marker now follows data size, not viewer.

Cons:

  • You can add sky regions but it does not respect WCS linking because under the hood, everything gets turned into pixels for bqplot.
  • We need to render all the shapes into polygons for bqplot so the shape follows data size, not viewer. This means we need to track the input regions separately to preserve API roundtrip.
  • API changes are totally not backward compatible.
  • Afraid to expose more customization (e.g., alpha for the fill color) in fear of causing Astrowidgets API to be unrealistic for other backends out there.
  • Can be easily confused with Footprints and Markers plugins. (This is already a problem regardless of this change or not but might as well list it.)

🐱

Change log entry

  • [x] Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts, list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • [x] Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • [ ] Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • [x] Do the proposed changes follow the STScI Style Guides?
  • [x] Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • [x] Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • [ ] Did the CI pass? If not, are the failures related?
  • [x] Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
  • [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

pllim avatar Aug 30 '23 21:08 pllim