Imviz: Make the WCS match tool into a pan/zoom with WCS matching tool #613

astrofrog · 2021-05-13T13:48:36Z

This PR changes the existing WCS matching tool to be a Pan/Zoom tool which matches WCS at the end of the pan/zoom as described in #519.

A few high-level decisions are needed:

For now, the datasets are linked on the fly when the tool is enabled, but this seems ugly, so I guess the question is whether we want to always link WCSes by default, either in jdaviz in general or in imviz. Is there any reason we would not want images linked by WCS in imviz? The linking doesn't mean the axes have to match on all image viewers, but more that glue will automatically know how to e.g. overlay datasets and so on and will allow blinking (since that also requires the WCSes to be matched). I personally think that imviz should always link datasets by WCS if present, but happy to hear counter-argumnents (we don't have to do this for all of jdaviz though)
Clicking on the WCS pan/zoom tool will automatically cause images in other viewers to be registered to the image in the viewer where the tool is enabled. This seems sensible, but what should users have to do if they want to get back to a view that is not matched? Should the 'home' icon do this?

Here's a preview of the current functionality (sorry for the bit where the viewer scrolls out of view, this is the bug fixed by @maartenbreddels in bqplot but I wasn't using that version):

Closes #519

astrofrog · 2021-05-13T13:54:24Z

jdaviz/configs/imviz/plugins/viewers.py

+    # Whether to inherit tools from glue-jupyter automatically. Set this to
+    # False to have full control here over which tools are shown in case new
+    # ones are added in glue-jupyter in future that we don't want here.
+    inherit_tools = False


@pllim - just FYI this is why the home button was inherited by default before. I think we don't actually want the inheritance here because who knows what tools will get added in glue-jupyter in future so I have disabled it for imviz. We should perhaps do this for other jdaviz apps but this is beyond the scope of this PR.

astrofrog · 2021-05-13T14:26:04Z

(this should be rebased once #614 is merged)

pllim · 2021-05-13T16:45:32Z

always link WCSes by default, either in jdaviz in general or in imviz

Just doing it in Imviz is probably less controversial. I have no idea how other viz is set up right now. Does this mean, we can remove the "link" button then?

automatically cause images in other viewers to be registered to the image in the viewer where the tool is enabled

I guess this satisfies the "or" clause under, "Match and/or Lock all viewers or selected viewers." Not sure how you select viewers but maybe that can be future work.

Should the 'home' icon do this?

Yes, that makes sense to me.

pllim · 2021-05-13T16:47:23Z

jdaviz/data/icons/convert.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+inkscape --without-gui --export-png=$PWD/pan_wcs.png --export-width=125 --export-height=125 $PWD/pan_wcs.svg


What is this for? Is this executed at run time?

It's just so that if we add more svg icons we can convert them all in a consistent way. It's not meant to be part of the installed package. Maybe I can move it to the root and call it .convert_icons_to_png.sh or something

It is only one line of command. Can we just move it to https://github.com/spacetelescope/jdaviz/wiki ?

True, my point was more that it might grow as more icons are added and we don't want all icons to be added in an inconsistent way. What about putting it as a comment in the __init__.py of the icons directory?

Not sure how discoverable that is. How about a short paragraph in dev docs then?

Actually let me just remove it - we really should just get the SVG icons to work.

Which I'll look into in a separate PR but my point is we don't need to document how to get the SVG.

pllim · 2021-05-20T18:32:47Z

#614 is merged. Please rebase, @astrofrog . Thanks!

PatrickOgle · 2021-05-20T19:16:21Z

Yes there are reasons not to link the WCS in separate viewers. I might want to keep a larger FOV in one window and a zoom only into one of the viewers. The lock function should be toggle-able.

PatrickOgle · 2021-05-20T19:17:43Z

What happens when you push the button a 3rd time and a 3rd viewer appears, both in terms of the locking capabililty and how the viewers are arranged on the screen. In a grid?

pllim · 2021-05-21T15:58:41Z

Let's not let perfect be the enemy of good. Can we have the basic functionality wrapped up here and discuss auto-linking in a separate issue/PR? Whether initial default is "link all" or "opt in only," I'll leave that to y'all to fight it out.

astrofrog · 2021-05-25T10:15:09Z

Yes there are reasons not to link the WCS in separate viewers. I might want to keep a larger FOV in one window and a zoom only into one of the viewers. The lock function should be toggle-able.

To clarify, this is how this behaves at the moment. Auto-linking data in glue won't cause viewers to always be locked - linking is just a way for glue to understand the connection between datasets, but I fully intend to have the pan/zoom locking be opt-in.

astrofrog · 2021-05-25T10:23:35Z

This is now rebased and to be clear the linking only happens when the tool is toggled. I'll open a separate PR about auto-linking.

pllim · 2021-05-25T13:52:23Z

The alldeps failure is annoying and appears unrelated. I have asked jwst devs. Unfortunately, it blocks the rest of the CI, though I see little reason why CI would fail with this patch.

tl;dr -- Ignore the CI failure (see #637)

pllim

Works as advertised with one caveat: If I zoom first on the reference image and then click on the "arrow with linking symbol", the other image zoom does not automatically update to reference image zoom.

pllim · 2021-05-25T14:35:36Z

The slight lag is also unfortunate but is out of scope here (see #564).

astrofrog · 2021-05-25T14:38:23Z

@pllim - the latest commit fixes that small issue you noticed - now when clicking on the tool the initial limits will be matched.

pllim

Works great now, both on "standalone" and notebook form. Didn't even break when I activate the "arrows with linking" on multiple sub-viewers. There is still some lag, but like I said, that is a separate known issue.

Standalone

Classic notebook

codecov · 2021-05-27T09:08:42Z

Codecov Report

Merging #613 (7787092) into main (2dc49f1) will decrease coverage by 0.07%.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
- Coverage   59.67%   59.60%   -0.08%     
==========================================
  Files          65       65              
  Lines        4000     4005       +5     
==========================================
  Hits         2387     2387              
- Misses       1613     1618       +5

Impacted Files	Coverage Δ
jdaviz/configs/imviz/plugins/tools.py	`38.61% <34.61%> (-2.63%)`	⬇️
jdaviz/configs/imviz/plugins/viewers.py	`27.63% <100.00%> (+0.96%)`	⬆️

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 2dc49f1...7787092. Read the comment docs.

duytnguyendtn

Tested in both the desktop and notebook interface. They look good to me!

pllim · 2021-06-04T19:37:37Z

Still works after rebase. I will merge when CI passes.

pllim · 2021-06-04T19:54:00Z

Thanks!

astrofrog requested review from duytnguyendtn, ibusko, javerbukh and rosteen as code owners May 13, 2021 13:48

github-actions bot added the imviz label May 13, 2021

astrofrog commented May 13, 2021

View reviewed changes

astrofrog mentioned this pull request May 13, 2021

Update pinned glue-jupyter #614

Merged

astrofrog force-pushed the wcs-pixel-matching branch from a45e8a5 to 539d21e Compare May 13, 2021 14:25

astrofrog mentioned this pull request May 13, 2021

Imviz: Add simple blink tool #611

Merged

3 tasks

astrofrog changed the title ~~Make the WCS match tool into a pan/zoom with WCS matching tool~~ Imviz: Make the WCS match tool into a pan/zoom with WCS matching tool May 13, 2021

pllim added this to the Imviz 1.0 milestone May 13, 2021

pllim reviewed May 13, 2021

View reviewed changes

pllim mentioned this pull request May 21, 2021

JDAT-1223: Tiling images #521

Closed

3 tasks

astrofrog force-pushed the wcs-pixel-matching branch from 539d21e to 8e33c5f Compare May 25, 2021 10:23

pllim added the Ready for final review label May 25, 2021

pllim approved these changes May 25, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

astrofrog force-pushed the wcs-pixel-matching branch from 77c2340 to d7465a8 Compare May 27, 2021 09:03

astrofrog force-pushed the wcs-pixel-matching branch from d7465a8 to 0dc1916 Compare May 27, 2021 09:04

This comment has been minimized.

Sign in to view

duytnguyendtn approved these changes Jun 2, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

Make the WCS match tool into a pan/zoom with WCS matching tool

37cd8c8

pllim force-pushed the wcs-pixel-matching branch from 0dc1916 to 37cd8c8 Compare June 4, 2021 18:19

pllim added 2 commits June 4, 2021 14:28

It's all about that rebase

b63be85

Fix example lable name

7787092

pllim merged commit 1aa0dce into spacetelescope:main Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imviz: Make the WCS match tool into a pan/zoom with WCS matching tool #613

Imviz: Make the WCS match tool into a pan/zoom with WCS matching tool #613

astrofrog commented May 13, 2021 •

edited

Loading

astrofrog May 13, 2021

astrofrog commented May 13, 2021

pllim commented May 13, 2021

pllim May 13, 2021

astrofrog May 15, 2021

pllim May 17, 2021

astrofrog May 17, 2021

pllim May 17, 2021

astrofrog May 25, 2021

astrofrog May 25, 2021

pllim commented May 20, 2021

PatrickOgle commented May 20, 2021

PatrickOgle commented May 20, 2021

pllim commented May 21, 2021

astrofrog commented May 25, 2021

astrofrog commented May 25, 2021

pllim commented May 25, 2021 •

edited

Loading

pllim left a comment

pllim commented May 25, 2021

astrofrog commented May 25, 2021

pllim left a comment

This comment has been minimized.

This comment has been minimized.

codecov bot commented May 27, 2021 •

edited

Loading

duytnguyendtn left a comment

This comment has been minimized.

pllim commented Jun 4, 2021

pllim commented Jun 4, 2021

		@@ -0,0 +1,3 @@
		#!/bin/bash

		inkscape --without-gui --export-png=$PWD/pan_wcs.png --export-width=125 --export-height=125 $PWD/pan_wcs.svg

Imviz: Make the WCS match tool into a pan/zoom with WCS matching tool #613

Imviz: Make the WCS match tool into a pan/zoom with WCS matching tool #613

Conversation

astrofrog commented May 13, 2021 • edited Loading

Choose a reason for hiding this comment

astrofrog commented May 13, 2021

pllim commented May 13, 2021

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 commented May 20, 2021

PatrickOgle commented May 20, 2021

PatrickOgle commented May 20, 2021

pllim commented May 21, 2021

astrofrog commented May 25, 2021

astrofrog commented May 25, 2021

pllim commented May 25, 2021 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

pllim commented May 25, 2021

astrofrog commented May 25, 2021

pllim left a comment

Choose a reason for hiding this comment

Standalone

Classic notebook

This comment has been minimized.

This comment has been minimized.

codecov bot commented May 27, 2021 • edited Loading

Codecov Report

duytnguyendtn left a comment

Choose a reason for hiding this comment

This comment has been minimized.

pllim commented Jun 4, 2021

pllim commented Jun 4, 2021

astrofrog commented May 13, 2021 •

edited

Loading

pllim commented May 25, 2021 •

edited

Loading

codecov bot commented May 27, 2021 •

edited

Loading