Imviz: astrowidget API for center_on and offset_to #687

pllim · 2021-06-16T21:59:05Z

Implement some of #631 and #632.

Make it work. Why is it zooming? See https://github.com/astropy/astrowidgets/pull/131/files#r585698007 (delay_callback is very important...)
See if SkyCoord implementation can be trivial. (Yes)
Add tests.
Update example notebook.

Out of scope: Unable to use this API on a tiled viewers; i.e., it only works on the first viewer that the app opened. See #643

codecov · 2021-06-16T22:05:10Z

Codecov Report

Merging #687 (7039327) into main (ccabe72) will increase coverage by 0.66%.
The diff coverage is 81.66%.

@@            Coverage Diff             @@
##             main     #687      +/-   ##
==========================================
+ Coverage   60.13%   60.80%   +0.66%     
==========================================
  Files          65       65              
  Lines        4099     4156      +57     
==========================================
+ Hits         2465     2527      +62     
+ Misses       1634     1629       -5

Impacted Files	Coverage Δ
jdaviz/configs/imviz/plugins/tools.py	`37.62% <0.00%> (-1.00%)`	⬇️
jdaviz/configs/imviz/plugins/viewers.py	`40.74% <0.00%> (+13.10%)`	⬆️
jdaviz/configs/imviz/helper.py	`95.55% <94.23%> (+0.81%)`	⬆️
jdaviz/configs/imviz/plugins/parsers.py	`97.80% <0.00%> (+0.54%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccabe72...7039327. Read the comment docs.

jdaviz/configs/imviz/helper.py

pllim · 2021-06-17T21:43:20Z

notebooks/ImvizExample.ipynb

@@ -2,13 +2,15 @@
 "cells": [
  {
   "cell_type": "markdown",
+   "id": "17b4af6f",


This isn't me. Jupyter did this after I upgraded. 🤷

pllim · 2021-06-18T20:40:44Z

jdaviz/configs/imviz/helper.py

+                    new_sky_cen = sky_cen.__class__(
+                        SkyOffsetFrame(dx, dy, origin=sky_cen.frame).transform_to(sky_cen))
+                else:
+                    new_sky_cen = sky_cen.spherical_offsets_by(dx, dy)


Note: This was added in astropy/astropy#11635

pllim · 2021-06-18T20:43:28Z

notebooks/ImvizExample.ipynb

   "metadata": {},
   "outputs": [],
   "source": [
    "from astropy.utils.data import download_file\n",
    "\n",
-    "gc_2mass_j = download_file('https://www.astropy.org/astropy-data/galactic_center/gc_2mass_j.fits', cache=True)\n",
-    "gc_msx_e = download_file('https://www.astropy.org/astropy-data/galactic_center/gc_msx_e.fits', cache=True)"
+    "acs_47tuc_1 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03gjq_flc.fits', cache=True)\n",


For my sanity, I am replacing the example images with real HST/ACS data. Everyone loves 47 Tuc, right? Or is it just calibration team? 😉

pllim · 2021-06-18T20:44:00Z

notebooks/ImvizExample.ipynb

   "metadata": {},
   "outputs": [],
   "source": [
-    "viewer.state.layers[0].percentile = 99"
+    "viewer.state.layers[0].percentile = 95"


Also for my sanity. Things are easier to see with 95th percentile.

pllim · 2021-06-18T21:33:59Z

jdaviz/configs/imviz/tests/test_astrowidgets_api.py

+
+class TestAstrowidgetsAPI:
+    @pytest.fixture(autouse=True)
+    def setup_class(self, imviz_app):


Note: This is basically the same test setup in #630 . When we get back to the other PR, will have to see how to consolidate this to avoid code duplication.

astrofrog

Just a couple of comments but looks good otherwise!

astrofrog · 2021-06-22T14:30:56Z

jdaviz/configs/imviz/helper.py

+
+        if isinstance(point, SkyCoord):
+            i_top = get_top_layer_index(viewer)
+            image = viewer.layers[i_top].layer


I think you should probably use image = viewer.reference_data as that is the one which is used to define the pixel space

Your suggestion does not seem to work: AttributeError: 'ImvizImageView' object has no attribute 'reference_data'

viewer.state.reference_data exists but it causes my test to fail:

# Blink to the one without WCS self.viewer.blink_once() with pytest.raises(AttributeError, match='does not have a valid WCS'): > self.imviz.center_on(sky) E Failed: DID NOT RAISE <class 'AttributeError'> jdaviz/configs/imviz/tests/test_astrowidgets_api.py:83: Failed

It appears reference_data does not keep track of which layer is actually visible to the user after blinking.

astrofrog · 2021-06-22T14:31:19Z

jdaviz/configs/imviz/helper.py

+
+        if skycoord_offset:
+            i_top = get_top_layer_index(viewer)
+            image = viewer.layers[i_top].layer


rosteen · 2021-06-25T15:33:59Z

Looks pretty good, the only thing I think might be good to add is some checking to see if the center_on input is within the bounds of the image. Inputting pixel coordinates beyond the bounds of the image centers there and just shows no data, which I guess is fine. Trying to center_on with sky coordinates that are outside the image, on the other hand, leads to a long stack trace:

---------------------------------------------------------------------------
NoConvergence                             Traceback (most recent call last)
<ipython-input-25-3978131487f0> in <module>
      3 
      4 sky = SkyCoord('01h24m07.33s -71d52m50.71s')
----> 5 imviz.center_on(sky)

~/projects/jdaviz/jdaviz/configs/imviz/helper.py in center_on(self, point)
    109             image = viewer.layers[i_top].layer
    110             if hasattr(image, 'coords') and isinstance(image.coords, BaseHighLevelWCS):
--> 111                 pix = image.coords.world_to_pixel(point)  # 0-indexed X, Y
    112             else:
    113                 raise AttributeError(f'{image.label} does not have a valid WCS')

~/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages/astropy/wcs/wcsapi/high_level_api.py in world_to_pixel(self, *world_objects)
    230 
    231         # Finally we convert to pixel coordinates
--> 232         pixel = self.low_level_wcs.world_to_pixel_values(*world)
    233 
    234         return pixel

~/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages/astropy/wcs/wcsapi/fitswcs.py in world_to_pixel_values(self, *world_arrays)
    315 
    316     def world_to_pixel_values(self, *world_arrays):
--> 317         pixel = self.all_world2pix(*world_arrays, 0)
    318         return pixel[0] if self.pixel_n_dim == 1 else tuple(pixel)
    319 

~/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages/astropy/wcs/wcs.py in all_world2pix(self, tolerance, maxiter, adaptive, detect_divergence, quiet, *args, **kwargs)
   1884             raise ValueError("No basic WCS settings were created.")
   1885 
-> 1886         return self._array_converter(
   1887             lambda *args, **kwargs:
   1888             self._all_world2pix(

~/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages/astropy/wcs/wcs.py in _array_converter(self, func, sky, ra_dec_order, *args)
   1334                     "a 1-D array for each axis, followed by an origin.")
   1335 
-> 1336             return _return_list_of_arrays(axes, origin)
   1337 
   1338         raise TypeError(

~/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages/astropy/wcs/wcs.py in _return_list_of_arrays(axes, origin)
   1288             if ra_dec_order and sky == 'input':
   1289                 xy = self._denormalize_sky(xy)
-> 1290             output = func(xy, origin)
   1291             if ra_dec_order and sky == 'output':
   1292                 output = self._normalize_sky(output)

~/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages/astropy/wcs/wcs.py in <lambda>(*args, **kwargs)
   1886         return self._array_converter(
   1887             lambda *args, **kwargs:
-> 1888             self._all_world2pix(
   1889                 *args, tolerance=tolerance, maxiter=maxiter,
   1890                 adaptive=adaptive, detect_divergence=detect_divergence,

~/opt/anaconda3/envs/viz_dev/lib/python3.9/site-packages/astropy/wcs/wcs.py in _all_world2pix(self, world, origin, tolerance, maxiter, adaptive, detect_divergence, quiet)
   1868                     slow_conv=ind, divergent=None)
   1869             else:
-> 1870                 raise NoConvergence(
   1871                     "'WCS.all_world2pix' failed to "
   1872                     "converge to the requested accuracy.\n"

NoConvergence: 'WCS.all_world2pix' failed to converge to the requested accuracy.
After 2 iterations, the solution is diverging at least for one input point.

Perhaps it would be useful to check whether the sky coordinates input are within the image bounds, and if not raise a more informative/concise error that you can't center on sky coordinates outside the image.

pllim · 2021-06-25T15:42:30Z

@rosteen , I think @mcara said this error only happens if image is distorted...? Which image did you use? Thanks for smoking this out!

rosteen · 2021-06-25T15:45:47Z

That's with acs_47tuc_1 from the Imviz example notebook.

pllim · 2021-06-25T15:51:07Z

Ah, yes. FLC is not corrected for distortion. 😬

pllim · 2021-06-25T16:14:38Z

@rosteen , is this better?

rosteen · 2021-06-25T16:41:38Z

I like capturing it that way much better than the long stack trace in the notebook. Approving, since making this work with more viewers needs to happen after some other work.

ojustino · 2021-06-25T20:14:24Z

Just did. No more error printouts, but the viewer still whites out and trying to recenter on an in-image location doesn't have a visible effect.

and update notebook

pllim · 2021-06-25T20:30:01Z

Added one more check. Is this a satisfactory solution to the error you encounter, @ojustino ?

pllim · 2021-06-25T20:35:42Z

The CI failure smells like upstream problem in Actions:

  Error: fatal: unable to access 'https://github.com/spacetelescope/jdaviz/': The requested URL returned error: 500
  The process '/usr/bin/git' failed with exit code 128

ojustino · 2021-06-25T20:43:59Z

@pllim With this commit, center_on triggers the new dropdown message and blocks execution if I try SkyCoords or pixels that are off-screen but do exist "on-image."

It is not blocking SkyCoords that are off-screen and off-image, and that case still causes the white out and does not trigger the dropdown message.

pllim · 2021-06-25T20:46:16Z

@ojustino , at this point... please point me to that image you are using, and provide me with the calls that (1) block execution, and (2) white out.

Thanks for the very thorough checks!

ibusko

I was able to run the demo notebook and exercise all cells that have the centering and offseting operations. The image seemed to behave in the way we expect.

ojustino · 2021-06-25T20:58:12Z

Perhaps the problems are with my environment, then? In any case, these are my images:

cygA1 = download_file('https://hla.stsci.edu/cgi-bin/getdata.cgi?dataset=hst_05368_01_wfpc2_f555w_pc_drz.fits', cache=True)
cygA2 = download_file('https://hla.stsci.edu/cgi-bin/getdata.cgi?dataset=hst_05368_01_wfpc2_f555w_wf_drz.fits', cache=True)

This is the offending code:

from astropy.coordinates import SkyCoord
sky = SkyCoord('09h59m26.02s 40d44m17.92s')

I also tried the notebook's original 47 Tuc images and get the same unintended blocking behavior in the "off-screen, on-image" case, though the "off-screen, off-image" case is blocked as intended. I should also note that anything "on-screen and on-image" has been working fine.

pllim · 2021-06-25T21:00:27Z

What does "off-screen, on-image" mean?

ojustino · 2021-06-25T21:03:01Z

A set of coordinates that has data in the image but is not currently visible in the viewer. An example would be if I'm zoomed in at one corner and want to center on a location on the opposite side of the image.

pllim · 2021-06-25T21:09:26Z

A set of coordinates that has data in the image but is not currently visible in the viewer.

Oh... In this case, perhaps changing x/y_min/max of the layer does not actually load the data that is currently not displayed. If you know how to force data loading, please let me know. Otherwise, here is my proposal:

Let me know if 7039327 needs to be reverted or not.
Once (1) is resolved, we merge and open new issue about the "off-screen, on-image" use case. This way, we can wait for SME, while I can also continue onto Imviz: astrowidgets markers API #699 while we wait till they become available.

ojustino · 2021-06-25T21:17:28Z

That's an answer I don't know.

I think 7039327 is a good idea for avoiding the whiteout scenario, so I'm fine with a merge, especially since other developers didn't encounter any issues.
I'm fine with migrating this problem into a new issue.

pllim · 2021-06-25T21:18:37Z

@astrofrog , if you feel strongly about using i_top to grab the top visible layer, we can handle it in a follow-up PR. Thanks!

Follow up next time

pllim · 2021-06-25T21:25:30Z

Thank you for the very thorough reviews, everyone!

@ojustino , I opened #704 as a follow-up issue. Please correct me if I got anything wrong there. Thanks again!

eteq · 2021-06-30T14:22:54Z

Post-hoc review: this looks great, except for one thing: I think it should be offset_by isntead of offset_to! Will create a follow-on issue to capture that.

pllim · 2021-06-30T14:27:36Z

@eteq , I think that ship has sailed?

https://github.com/astropy/astrowidgets/blob/ace5abb820c030122add475ecf106114aaa68e1b/astrowidgets/core.py#L346

https://github.com/eteq/nb-astroimage-api/blob/ee6f0a30673b11c0c68192c6c2fd1dfa47ceb23c/minimum_viable_imagewidget.py#L53

pllim added this to the Imviz 1.0 milestone Jun 16, 2021

pllim requested a review from astrofrog June 16, 2021 21:59

github-actions bot added the imviz label Jun 16, 2021

pllim mentioned this pull request Jun 16, 2021

[Meta PR] Imviz: astrowidget API #632

Closed

14 tasks

pllim commented Jun 16, 2021

View reviewed changes

jdaviz/configs/imviz/helper.py Outdated Show resolved Hide resolved

pllim force-pushed the astrowidgets-quack-center-offset branch from 30f1543 to e1e8906 Compare June 17, 2021 21:40

pllim commented Jun 17, 2021

View reviewed changes

pllim force-pushed the astrowidgets-quack-center-offset branch from e1e8906 to a96032d Compare June 18, 2021 20:38

pllim commented Jun 18, 2021

View reviewed changes

pllim force-pushed the astrowidgets-quack-center-offset branch from a96032d to c910d1d Compare June 18, 2021 21:31

pllim marked this pull request as ready for review June 18, 2021 21:32

pllim requested review from duytnguyendtn, ibusko, javerbukh and rosteen as code owners June 18, 2021 21:32

pllim commented Jun 18, 2021

View reviewed changes

pllim added the Ready for final review label Jun 18, 2021

pllim requested a review from eteq June 18, 2021 21:35

astrofrog previously requested changes Jun 22, 2021

View reviewed changes

pllim force-pushed the astrowidgets-quack-center-offset branch from c910d1d to 2650711 Compare June 25, 2021 16:14

rosteen approved these changes Jun 25, 2021

View reviewed changes

pllim added 6 commits June 25, 2021 16:28

Imviz: API for center_on and offset_to in pixels

9ac687a

Imviz center_on, offset_to for sky

45c779d

and update notebook

Add tests for Imviz center_on, offset_to

edc3c9c

Graceful warning on WCS no convergence

cb518b5

Handle out of bounds mouseover

8a0497f

Disallow centering out of bounds

7039327

pllim force-pushed the astrowidgets-quack-center-offset branch from d077314 to 7039327 Compare June 25, 2021 20:29

ibusko mentioned this pull request Jun 25, 2021

Fix bug with empty snackbar queue #703

Merged

ibusko approved these changes Jun 25, 2021

View reviewed changes

pllim merged commit e45e503 into spacetelescope:main Jun 25, 2021

pllim deleted the astrowidgets-quack-center-offset branch June 25, 2021 21:19

pllim mentioned this pull request Jun 25, 2021

[BUG] Imviz: "White out" when center_on "off-screen, on-image" #704

Closed

eteq mentioned this pull request Jun 30, 2021

Imviz helper: rename offset_to to offset_byand have it use units #705

Closed

pllim mentioned this pull request Jul 1, 2021

Imviz: Replace offset_to with offset_by #709

Merged

pllim mentioned this pull request Aug 2, 2021

Expose astrowidgets API to all viz via mixin class #754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imviz: astrowidget API for center_on and offset_to #687

Imviz: astrowidget API for center_on and offset_to #687

pllim commented Jun 16, 2021 •

edited

Loading

codecov bot commented Jun 16, 2021 •

edited

Loading

pllim Jun 17, 2021

pllim Jun 18, 2021

pllim Jun 18, 2021

pllim Jun 18, 2021

pllim Jun 18, 2021

astrofrog left a comment

astrofrog Jun 22, 2021

pllim Jun 22, 2021

pllim Jun 22, 2021

pllim Jun 22, 2021

astrofrog Jun 22, 2021

rosteen commented Jun 25, 2021

pllim commented Jun 25, 2021

rosteen commented Jun 25, 2021

pllim commented Jun 25, 2021

pllim commented Jun 25, 2021

rosteen commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

pllim commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

ibusko left a comment

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

pllim commented Jun 25, 2021

eteq commented Jun 30, 2021

pllim commented Jun 30, 2021

Imviz: astrowidget API for center_on and offset_to #687

Imviz: astrowidget API for center_on and offset_to #687

Conversation

pllim commented Jun 16, 2021 • edited Loading

codecov bot commented Jun 16, 2021 • edited Loading

Codecov Report

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

astrofrog 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

rosteen commented Jun 25, 2021

pllim commented Jun 25, 2021

rosteen commented Jun 25, 2021

pllim commented Jun 25, 2021

pllim commented Jun 25, 2021

rosteen commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

pllim commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

ibusko left a comment

Choose a reason for hiding this comment

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

ojustino commented Jun 25, 2021

pllim commented Jun 25, 2021

pllim commented Jun 25, 2021

eteq commented Jun 30, 2021

pllim commented Jun 30, 2021

pllim commented Jun 16, 2021 •

edited

Loading

codecov bot commented Jun 16, 2021 •

edited

Loading