Imviz: Add simple blink tool

[pllim]

Fix #520

Out of scope: Nice UI/UX stuff.

TODO

• I simply grabbed an icon from Glue. If you have a better one, let me know.
• When I blink, the second image does not show. I see a white screen where it should have been. This is the same whether I use the existing "b" key or this new button. But if I check the layers menu, the "eye" and "no eye" icon change as expected. Is it just me? (Data needs to auto-link on load, see Imviz: Make the WCS match tool into a pan/zoom with WCS matching tool #613)
• Get approval from someone so we can merge and close this out.

[pllim]

RTD warning seems unrelated: docs/dev/release.rst:89: WARNING: undefined label: virtual_envs

[astrofrog]

I simply grabbed an icon from Glue. If you have a better one, let me know

Note that with glue-jupyter 0.5 (#614) you can now give a full path to an icon (before you had to use one of the glue.icons icons). See #613 for an example.

When I blink, the second image does not show. I see a white screen where it should have been

This is because the datasets need to be linked - see #613 for a discussion of this in the description. I think in imviz we should probably link datasets by default.

[pllim]

It is not intuitive to me that datasets need to be linked to be blinked. I thought linking is only when they have shared data that I want to explore simultaneously. Blinking is just... blinking.

Also linking everything might have performance impact if we ended up "loading everything" as requested in #600 . It simply does not make sense to try to link all 14 extensions in a drizzled HST product.

[astrofrog]

Blinking requires the images to be registered though, which requires linking. Without this you won't be able to blink images with different WCS.

Note that for 14 extensions on the same WCS we can use pixel-wise linking instead of WCS which would be very fast and wouldn't have a performance impact.

What DS9 does is assume the WCS is always correct and is effectively doing what I am calling WCS linking between all loaded datasets.

[pllim]

14 extensions on the same WCS

The thing is... I am not sure if they are. Maybe the first 3.

[astrofrog]

I guess my main point is, regardless of what happens during data loading, once Data objects exist with valid WCSes, we can auto-link them and I think we can do it in a way that does not cause performance issues.

[pllim]

Re: link -- OK, fine. I guess that will be implemented over at #613 anyway, so I think this is ready if no one has a better idea for the icon.

[astrofrog]

To be clear #613 only links the data once the pan/zoom icon is clicked. However if a user blinks before hitting that tool then it won't work correctly, so we really should set up the links when data are added to the viewer instead (I can do a separate PR for that).

[pllim]

Please do, thanks!

[codecov]

Codecov Report

Merging #611 (ea0d9e0) into main (6ac1256) will increase coverage by 0.10%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main     #611      +/-   ##
==========================================
+ Coverage   59.25%   59.36%   +0.10%     
==========================================
  Files          65       65              
  Lines        3888     3910      +22     
==========================================
+ Hits         2304     2321      +17     
- Misses       1584     1589       +5     

Impacted Files	Coverage Δ
jdaviz/configs/imviz/plugins/viewers.py	26.66% <13.04%> (-1.46%)	⬇️
jdaviz/cli.py	42.50% <33.33%> (ø)
jdaviz/configs/imviz/plugins/tools.py	43.10% <83.33%> (+9.06%)	⬆️
jdaviz/configs/imviz/plugins/parsers.py	94.84% <100.00%> (ø)
...configs/default/plugins/data_tools/file_chooser.py	69.14% <0.00%> (+3.42%)	⬆️

---

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 3115559...ea0d9e0. Read the comment docs.

[rosteen]

This doesn't seem to play well with subsets at the moment. With no subsets defined I can blink between the two images. But if I choose a circular region to define a subset, the blinking then switches between the two images and a blank white image with the subset overlayed (actually two blank white images with the subset, I assume one for each original image). It also starts throwing this error:

AttributeError                            Traceback (most recent call last)
~/opt/anaconda3/envs/viz_dev/lib/python3.8/site-packages/glue_jupyter/bqplot/common/viewer.py in _on_mouse_interaction(self, interaction, data, buffers)
    151     def _on_mouse_interaction(self, interaction, data, buffers):
    152         for callback in self._event_callbacks:
--> 153             callback(data)
    154 
    155     @debounced(delay_seconds=0.5, method=True)

~/projects/jdaviz/jdaviz/configs/imviz/plugins/viewers.py in on_mouse_or_key_event(self, data)
     64             self.label_mouseover.pixel = (fmt.format(x, y))
     65 
---> 66             if isinstance(image.coords, BaseHighLevelWCS):
     67                 # Convert these to a SkyCoord via WCS - note that for other datasets
     68                 # we aren't actually guaranteed to get a SkyCoord out, just for images

AttributeError: 'GroupedSubset' object has no attribute 'coords'

Another small note, it took me a minute to realize I needed to select both images in the data dropdown to be able to blink between them. Maybe we could display a snackbar message if the user is trying to blink only one image? I think that's an enhancement, not strictly needed here.

[pllim]

Ops... maybe the code needs to be smart enough to ignore subsets.

Also, will it be less confusing if I have the example notebook show both images? It's my bad to set the second one to not show earlier in the development work.

[rosteen]

I just tried deleting show_in_viewer=False from the second data load and oddly, the second image shows as all black if I do that. I'm not sure why, I thought maybe the scaling for the second image is different than if you go into the data and select it manually after loading, but then I tried to change the scaling and it stayed black. So...maybe just leave that part as-is for now.

[pllim]

second image shows as all black

I think that's to do with no auto-linking. Did you run the cell that says you need to run this to link them first?

[rosteen]

I did run the linking cell, and it doesn't just show up as black when blinking it shows up as black no matter how I display it. It almost seems like the color scaling of the second image loaded is getting locked somehow, such that I can't change it with the Layer options. It's a little easier to see if I switch the order in which the two images are loaded, in which case I see a gray square for the second loaded image rather than all black:

[rosteen]

Confirmed that the solution to this is to deselect and reselect the images from the Data menu after doing the WCS linking, and that it behaves the same on the main branch. So not something that was introduced by this PR.

[pllim]

I actually don't know under what condition we will fall into this block but doesn't hurt?

[pllim]

I think I have addressed the issues reported by @rosteen here:

• The notebook now tells you to uncheck and recheck the Data stuff to avoid display confusion.
• Coordinates info logic is now more resistant against callback invoked by Subset object.
• Revamped blink logic to ignore Subsets, since they are global anyway. And also reduced code duplication.
• Added "snackbar" warning message as requested. Missed opportunity to insert Scooby Snackbar into commit message.

[pllim]

The coverage reduction is unavoidable, as this only adds interactive stuff... The rest of CI passed. Is this good enough for approval?

[javerbukh]

Glad I caught the branch name before I merged 😂 Looks great!