viewer user-APIs exposing zoom/limit options #2563

kecnry · 2023-11-13T17:22:11Z

Description

This pull request adds access to viewer user-APIs (in the same way that plugin user-APIs are handled) with access to zoom/zoom_level (as well as all astrowidgets API methods that were previously available through imviz.default_viewer) for image viewers and set_lims(x_min=None, x_max=None, y_min=None, y_max=None) for other viewers. This also point imviz.default_viewer to this user-API layer for consistency and to prevent accessing methods/attributes that are not currently considered public.

~~In order to add zoom/zoom_level to image viewers in Cubeviz, this also adds the astrowidgets mixin to the image viewers in cubeviz.~~

For example:

imviz.viewers['imviz-0'].zoom(2)
cubeviz.viewers['spectrum-viewer'].set_lims(x_min=4000, y_max=1.3)

Change log entry

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.

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.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
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?
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)?

codecov · 2023-11-13T17:38:57Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ffb585b) 91.47% compared to head (89db77e) 91.46%.

Files	Patch %	Lines
jdaviz/configs/default/plugins/viewers.py	92.85%	2 Missing ⚠️
jdaviz/core/user_api.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2563      +/-   ##
==========================================
- Coverage   91.47%   91.46%   -0.01%     
==========================================
  Files         160      160              
  Lines       19438    19477      +39     
==========================================
+ Hits        17780    17815      +35     
- Misses       1658     1662       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pllim

In general, if we want to expose API to users, we need to define the API in concepts that users understand. If I were a naive user, I won't know at a glance what x_min and x_max is supposed to be. I would only care about zooming to a feature of interest, say, between 5000-6000 Angstrom (which might or might not be the unit the viewer is currently displaying).

jdaviz/configs/cubeviz/plugins/viewers.py

pllim · 2023-11-13T18:05:30Z

jdaviz/configs/default/plugins/viewers.py

+        if isinstance(self, AstrowidgetsImageViewerMixin):
+            expose = ['zoom', 'zoom_level']
+        else:
+            expose = ['set_lims']


Why set_lims and not set_limits?

Also, is this unit aware? What if data of native different units are loaded? Or does this only care about the currently displayed units?

Would set_spectral_axis_limits and set_flux_limits make more sense as separate methods for 1D spectrum viewer? Though I am not sure about the 2D spectrum viewer or the Mosviz table viewer.

Why set_lims and not set_limits?

No good reason, that's what we have in the plugin plots, but if we would prefer limits, we can change them both here, I don't think that is considered public API.

Also, is this unit aware? What if data of native different units are loaded? Or does this only care about the currently displayed units?

This is axes limits (based on the current axes units) - if there is no opposition to this, I'll update the docstring accordingly and throw errors for passing quantities. We could of course support quantities in the future by converting within the method, I can see how that would be useful. (EDIT: docstring and type-check added).

Mosviz table viewer.

Good point - I need to either have the table viewer not inherit this method or at least exclude it from the user API. (EDIT: done).

Why set_lims and not set_limits?

Glue does use reset_lims (not limits), so I think that is where the plot options API adopted this. (Similarly for the choice of x_min vs xmin to match glue-state).

kecnry · 2023-11-13T19:09:45Z

If I were a naive user, I won't know at a glance what x_min and x_max is supposed to be. I would only care about zooming to a feature of interest, say, between 5000-6000 Angstrom (which might or might not be the unit the viewer is currently displaying).

I don't know if that's true. I think setting limits on a viewer is a common thing and is useful to be able to set specific units from the notebook for reproducible workflows (this is the exact use-case that motivated this PR). We can of course create more methods that automate the selection of limits though.

pllim · 2023-11-13T19:13:40Z

reproducible workflows

Reproducible workflow in notebook cells that can be run out of order is... ambitious.

What if someone run an extra cell to change the viewer unit, and then run the API call with the numbers for the previous unit?

kecnry · 2023-11-13T19:17:48Z

The same could be said about any API command. They do what they say they do, if you add something in between, you will get different results, that is ok and expected.

javerbukh

I'm ambivalent about set_lims vs set_limits but otherwise I think this is a net positive. It's nice to have such simple access to the viewers!

pllim

Need change log and maybe also tests?

Do we also need to update user docs?

pllim · 2023-11-27T17:16:34Z

jdaviz/configs/default/plugins/viewers.py

+    @property
+    def user_api(self):
+        # default exposed user APIs.  Can override this method in any particular viewer.
+        if isinstance(self, BqplotImageView):


Hmm. What about Astrowidgets API?

what about it?

Should they be exposed here?

I think this would be the place to expose them, but maybe not quite yet since you said they are still actively changing, and exposing them here would commit us to supporting/deprecating/etc?

Hmm, I have to think about the implications here a bit more. It also won't do if we disallow people to use the API this way.

It also won't do if we disallow people to use the API this way.

Can you elaborate what you mean?

I don't know yet. Lemme try to use it. Will let you know soon. 😆

See #2563 (review)

pllim

This is pretty confusing when Astrowidgets API is involved. For example, this is the behavior with this PR as it currently stands:

imviz.default_viewer.stretch = "sqrt"  # OK
imviz.default_viewer.stretch_options  # Returns list of str

vv = imviz.viewers["imviz-0"]  # As far as user is concerned, this is still default viewer
vv.stretch = "linear"  # No error but does nothing
vv.stretch_options  # Error

kecnry · 2023-11-27T18:47:51Z

Is default_viewer and the underlying astrowidgets API considered public/stable? If so, perhaps we should expose all of that here and have default_viewer just be a shortcut to this same layer. If we don't want to do that, then I think this actually acts as expected and we should consider deprecating default_viewer in the future.

We probably could also forbid setting a non-existing attribute, but I'd argue that is out-of-scope for this PR as we should consider it for the plugins as well.

pllim · 2023-11-27T18:53:04Z

Is default_viewer and the underlying astrowidgets API considered public/stable?

default_viewer is everywhere in our examples but I really don't know if it is going to disappear or not in the future if someone wants the ability to create N number of viewers and then close everything out except viewer N-1 or if we decide to somehow want an Imviz without no viewer (though why, I have no idea). So I would say it being advertised a lot... but does that count as public?

To disallow Astrowidgets API now would be a step backward. I would say it is public as per @eteq's request. If anything is changed upstream, I would have to deprecate accordingly anyway to match those changes, regardless of this PR or not.

kecnry · 2023-11-27T18:55:10Z

See #2577 for preventing the case shown in your example. With that in place... would you rather we expose all of astrowidgets API here as well and have default_viewer point to the user-API layer, or leave that for down the road?

pllim · 2023-11-27T18:56:39Z

would you rather we expose all of astrowidgets API here as well

Yes.

And since this would be the recommended way to access viewer API going forward, I also say in this PR, you should update the docs and notebooks.

kecnry · 2023-11-27T19:13:33Z

Ok, I've added in the astrowidgets methods/attributes for Imviz and have imviz.default_viewer to return the same layer as going through imviz.viewers. Since we don't seem to have a plan to deprecate default_viewer, I don't see the need to update docs/notebooks at this time, but as this gets used more and we want to reference it in example scripts, etc, we may want to either extend or have another page similar to https://jdaviz.readthedocs.io/en/latest/plugin_api.html.

CHANGES.rst

kecnry · 2023-11-27T19:25:39Z

(I'll need to update a bunch of tests which used default_viewer.state to call a lot of internal functionality. Putting in draft until that is done.

pllim · 2023-11-27T19:42:26Z

Sorry for the extra work, but if we want this to be public, might as well go all the way? 😬

* to use public API where possible, and otherwise go through ._obj

pllim

Example Imviz notebook works and diff LGTM. Thanks!

pllim · 2023-11-28T17:52:55Z

Remote timeout is unrelated but it did cancel the OSX job, so I restarted them to be sure.

javerbukh

Thank you for the improvements, looks good!

* viewer user-APIs exposing zoom/limit options * defer exposing astrowidgets API in cubeviz * no public API (yet) for table viewers * docstring and type-checks for set_lims * set_lims > set_limits * changelog and basic test coverage * include astrowidgets API * add blink_once and reset_limits to user API * update internal calls to default_viewer * to use public API where possible, and otherwise go through ._obj

kecnry added this to the 3.8 milestone Nov 13, 2023

github-actions bot added the cubeviz label Nov 13, 2023

pllim added the api API change label Nov 13, 2023

pllim reviewed Nov 13, 2023

View reviewed changes

pllim mentioned this pull request Nov 13, 2023

Unleash Astrowidgets API on Cubeviz image viewers #2564

Open

kecnry marked this pull request as ready for review November 16, 2023 19:27

kecnry requested review from rosteen, javerbukh, bmorris3, haticekaratay and cshanahan1 as code owners November 16, 2023 19:27

kecnry mentioned this pull request Nov 16, 2023

demo notebook for triple eclipsing system spacetelescope/lcviz#65

Closed

javerbukh approved these changes Nov 22, 2023

View reviewed changes

kecnry added the Ready for final review label Nov 27, 2023

pllim reviewed Nov 27, 2023

View reviewed changes

kecnry mentioned this pull request Nov 27, 2023

user-APIs: forbid setting non-existing attribute #2577

Merged

9 tasks

kecnry force-pushed the viewer-user-apis branch from 90e85fc to 8e7dcb5 Compare November 27, 2023 19:11

pllim reviewed Nov 27, 2023

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

kecnry marked this pull request as draft November 27, 2023 19:25

kecnry force-pushed the viewer-user-apis branch 2 times, most recently from 729f810 to 28118cf Compare November 27, 2023 21:06

kecnry requested a review from javerbukh November 28, 2023 13:16

kecnry marked this pull request as ready for review November 28, 2023 13:16

kecnry removed the Ready for final review label Nov 28, 2023

kecnry force-pushed the viewer-user-apis branch from 28118cf to 848014c Compare November 28, 2023 14:42

kecnry added 9 commits November 28, 2023 10:09

viewer user-APIs exposing zoom/limit options

3da22c5

defer exposing astrowidgets API in cubeviz

824fd9d

no public API (yet) for table viewers

257871b

docstring and type-checks for set_lims

229b4d5

set_lims > set_limits

e10b918

changelog and basic test coverage

99f382f

include astrowidgets API

96566ad

add blink_once and reset_limits to user API

79b7bbc

update internal calls to default_viewer

89db77e

* to use public API where possible, and otherwise go through ._obj

kecnry force-pushed the viewer-user-apis branch from 848014c to 89db77e Compare November 28, 2023 15:10

pllim approved these changes Nov 28, 2023

View reviewed changes

pllim added the Ready for final review label Nov 28, 2023

javerbukh approved these changes Nov 28, 2023

View reviewed changes

pllim merged commit 9ecfec6 into spacetelescope:main Nov 28, 2023
17 of 19 checks passed

kecnry deleted the viewer-user-apis branch November 28, 2023 19:33

kecnry mentioned this pull request Jan 5, 2024

Fix matched pan zoom bmorris3/jdaviz#29

Merged

9 tasks

kecnry mentioned this pull request Feb 19, 2024

ephemeris plugin: phase viewer user API and float support for times_to_phases spacetelescope/lcviz#87

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

viewer user-APIs exposing zoom/limit options #2563

viewer user-APIs exposing zoom/limit options #2563

kecnry commented Nov 13, 2023 •

edited

Loading

codecov bot commented Nov 13, 2023 •

edited

Loading

pllim left a comment

pllim Nov 13, 2023

kecnry Nov 13, 2023 •

edited

Loading

kecnry Nov 13, 2023

kecnry commented Nov 13, 2023

pllim commented Nov 13, 2023

kecnry commented Nov 13, 2023

javerbukh left a comment

pllim left a comment

pllim Nov 27, 2023

kecnry Nov 27, 2023

pllim Nov 27, 2023

kecnry Nov 27, 2023

pllim Nov 27, 2023

kecnry Nov 27, 2023

pllim Nov 27, 2023

pllim Nov 27, 2023

pllim left a comment

kecnry commented Nov 27, 2023

pllim commented Nov 27, 2023

kecnry commented Nov 27, 2023

pllim commented Nov 27, 2023

kecnry commented Nov 27, 2023 •

edited

Loading

kecnry commented Nov 27, 2023

pllim commented Nov 27, 2023

pllim left a comment

pllim commented Nov 28, 2023

javerbukh left a comment

viewer user-APIs exposing zoom/limit options #2563

viewer user-APIs exposing zoom/limit options #2563

Conversation

kecnry commented Nov 13, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry commented Nov 13, 2023

pllim commented Nov 13, 2023

kecnry commented Nov 13, 2023

javerbukh left a comment

Choose a reason for hiding this comment

pllim 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

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

Choose a reason for hiding this comment

kecnry commented Nov 27, 2023

pllim commented Nov 27, 2023

kecnry commented Nov 27, 2023

pllim commented Nov 27, 2023

kecnry commented Nov 27, 2023 • edited Loading

kecnry commented Nov 27, 2023

pllim commented Nov 27, 2023

pllim left a comment

Choose a reason for hiding this comment

pllim commented Nov 28, 2023

javerbukh left a comment

Choose a reason for hiding this comment

kecnry commented Nov 13, 2023 •

edited

Loading

codecov bot commented Nov 13, 2023 •

edited

Loading

kecnry Nov 13, 2023 •

edited

Loading

kecnry commented Nov 27, 2023 •

edited

Loading