-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Meta PR] Imviz: astrowidget API #632
[Meta PR] Imviz: astrowidget API #632
Conversation
@eteq -- Does the separation look about right (MVP vs not MVP)? I will only attempt to implement MVP items for this round. @astrofrog -- If some of the things are simply impossible, please let me know. Hopefully the API doc from astrowidgets is sufficient to explain what they are supposed to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly agree with the MVP or not separation, but see my inline comments about the two that I think are currently "maybe depending on interpretation"
def load_fits(self, fitsorfn, numhdu=None, memmap=None): | ||
raise NotImplementedError | ||
|
||
def load_nddata(self, nddata): | ||
raise NotImplementedError | ||
|
||
def load_array(self, arr): | ||
raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is MVP in the sense that it's part of the existing load_data
, right? I.e., these could be trivial wrappers into that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we debated whether having load_xxx
in addition to imviz.load_data
would be too confusing.
PEP 20: There should be one-- and preferably only one --obvious way to do it.
raise NotImplementedError | ||
|
||
@property | ||
def scroll_pan(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first red this as scroll_pun
, and have to say I'm a bit disappointed in the reality that it's pan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have a scroll_pun
mode where the viewer just spews puns in the "snackbar" as you scroll...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also removed from MVP, see https://github.com/spacetelescope/jdaviz/pull/632/files#r683793189
11f61e9
to
1c5316d
Compare
def save(self, filename): | ||
raise NotImplementedError | ||
|
||
# markers (MVP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eteq -- Are we on the same page w.r.t. markers now?
Still need to discuss loaders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markers MVP: See #699
def center_on(self, point): | ||
raise NotImplementedError | ||
|
||
def offset_to(self, dx, dy, skycoord_offset=False): | ||
raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #687
As discussed between @PatrickOgle and I on 2021-07-06, we removed |
raise NotImplementedError | ||
|
||
@property | ||
def click_center(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: In an offline conversion with @PatrickOgle , we have removed click_center
and click_drag
from MVP, as one can just use the pan/zoom button.
All the MVP API have been implemented and you can see the PRs linked to this one, so I am closing this meta PR. |
Fix #631
Update: Actual implementation will be a series of smaller PRs with real code. But I would like to keep this open as a meta PR to keep track of progress.
MVP API list
center_on
,offset_to
(implemented in Imviz: astrowidget API for center_on and offset_to #687)zoom_level
,zoom
(maybe only pick one way to set zoom, also see Zoom: Why two ways to do the same thing? astropy/astrowidgets#144): Imviz astrowidgets API MVP: zoom_level, zoom #715stretch_options
,stretch
: Imviz astrowidgets API MVP: stretch_options, stretch #716autocut_options
,cuts
: Imviz astrowidgets API MVP: autocut_options, cuts #717colormap_options
,set_colormap
: Imviz astrowidgets API MVP: colormap_options, set_colormap #718save
: Imviz astrowidgets API MVP: save #719markers
,add_markers
,remove_markers
,reset_markers
(implemented in Imviz: astrowidgets markers API #699)Removed from MVP, see #632 (comment)
click_center
click_drag
scroll_pan
TODO
astrowidgets/glupyter.py
).