Better viewer zoom/center sync with different refdata #12

bmorris3 · 2023-10-16T20:31:04Z

At time of writing, spacetelescope#2179 un-syncs the pan and zoom between viewers in Imviz when different reference data are selected in each viewer, and re-syncs them when the reference data are made to match.

This PR syncs the sky centroids of the FOV in each viewer, and approximately syncs the zoom level. I say the zoom sync is "approximate" because we can't uniformly assign one set of {x/y}_{min/max} for all viewers when any are allowed to have different reference data (/rotation). Instead, I compute the average width of each side of the active viewer in world coordinates, and set the zoom level in the other viewers to match the active viewer's width.

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)? 🐱

kecnry · 2023-10-17T12:58:35Z

jdaviz/configs/imviz/plugins/tools.py

-        return (
-            isinstance(viewer, BqplotImageView) and
-            viewer.state.reference_data == self.viewer.state.reference_data
-        )
-
-    def _post_activate(self):
-        # NOTE: For Imviz only.
-        # Set the reference data in other viewers to be the same as the current viewer.
-        # If adding the data to the viewer, make sure it is not actually shown since the
-        # user didn't request it.
-        for viewer in self._iter_matched_viewers(include_self=False):
-            if self.viewer.state.reference_data not in viewer.state.layers_data:
-                viewer.add_data(self.viewer.state.reference_data)
-                for layer in viewer.state.layers:
-                    if layer.layer is self.viewer.state.reference_data:
-                        layer.visible = False
-                        break
-            viewer.state.reference_data = self.viewer.state.reference_data


so glad to finally be rid of this - thanks!

pllim · 2023-10-17T15:32:53Z

As I mentioned at tag-up, since this could be confusing and has confused you more than once, is it less confusing to force the other viewers to switch to the same rotation as the viewer where this button is clicked first before link pan/zoom is executed?

Would anyone sane want to link pan/zoom on data rotated differently in the sky? Usually when I do it, I expect the data to be aligned (by pixel if I am inspecting data quality flags or by sky if I am inspecting an astronomical object of interest).

bmorris3 · 2023-10-17T15:38:28Z

@pllim The first example that comes to mind is something like: "I have two galaxies in my image. I want to make elliptical subsets for each, aligning the major axis of the ellipse with the major axis of the galaxy. I open the same image in two viewers with the correct rotation for each galaxy, and draw the elliptical subset in each viewer (so I can avoid changing subset rotation via the plugin)."

pllim · 2023-10-17T15:44:26Z

Re: #12 (comment)

Why would linked pan/zoom be useful in that situation?

bmorris3 · 2023-10-17T15:55:18Z

@pllim When you load the images, in general, the pan and zoom won't be ideal for drawing your subsets, so you'll probably want to center and zoom into the region with your targets.

pllim · 2023-10-17T16:39:34Z

From "offline" conversations, Brett and I agree that these are viable alternative solutions that would be less confusing but need other devs or POs inputs. Though what I wrote here is a bit more than what was on Slack because my ideas evolved a bit as I typed.

Force all other viewers to switch to using reference image of the viewer where the button is activated and used. This might be complicated if we allow the same button to be active on different viewers at the same time?
Only execute linked pan/zoom on viewers with matching reference data. We need to know which viewer is under the mouse and whether link pan/zoom is asked for in that viewer. Then we look for only other viewers with the same reference data.
Only execute linked pan/zoom if the reference data is the same across all viewers. That is, this button is disabled altogether unless all viewers have the same reference data.

kecnry · 2023-10-17T17:19:02Z

Force all other viewers to switch to using reference image of the viewer where the button is activated and used. This might be complicated if we allow the same button to be active on different viewers at the same time?
Only execute linked pan/zoom on viewers with matching reference data. We need to know which viewer is under the mouse and whether link pan/zoom is asked for in that viewer. Then we look for only other viewers with the same reference data.

I personally think either of these would be more confusing/jarring than a not-exact match of zoom limits.

Only execute linked pan/zoom if the reference data is the same across all viewers. That is, this button is disabled altogether unless all viewers have the same reference data.

This is an option, but is it worth disabling just for some slight approximation. Can we just update the tooltip in these cases instead to include some text for "approximate zoom-level matching"?

codecov-commenter · 2023-10-18T15:01:39Z

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (wcs-only-layers@588e459). Click here to learn what that means.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                Coverage Diff                 @@
##             wcs-only-layers      #12   +/-   ##
==================================================
  Coverage                   ?   90.28%           
==================================================
  Files                      ?      159           
  Lines                      ?    19503           
  Branches                   ?        0           
==================================================
  Hits                       ?    17609           
  Misses                     ?     1894           
  Partials                   ?        0

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

kecnry · 2023-10-18T16:36:42Z

jdaviz/core/tools.py

+                viewer.zoom(
+                    float(to_fov_sky / orig_fov_sky)
+                )
+                viewer.center_on(sky_cen)


are these processed sequentially? If they're just modifying the limits under the hood, does wrapping in delay_callback work/help?

I didn't expect it to matter or make a difference.

kecnry · 2023-10-18T16:37:33Z

jdaviz/core/tools.py

+
+            to_refdata = viewer.state.reference_data
+
+            if orig_refdata != to_refdata and hasattr(self.viewer, '_get_fov'):


this mixin is also used for spectrum-viewers... is this useful there too or do we need to check the viewer type?

The hasattr check only passes for image viewers, so this shouldn't be a problem.

kecnry

I think this is a good improvement for now and we can always tweak based on user feedback.

Screen.Recording.2023-10-20.at.8.45.23.AM.mov

bmorris3 added the no-changelog-entry-needed label Oct 16, 2023

github-actions bot added the imviz label Oct 16, 2023

bmorris3 force-pushed the wcs-only-layers-viewer-sync branch from b9d7274 to 2d67a4f Compare October 17, 2023 12:56

kecnry reviewed Oct 17, 2023

View reviewed changes

bmorris3 force-pushed the wcs-only-layers branch from 58c2d0f to 588e459 Compare October 17, 2023 21:00

bmorris3 force-pushed the wcs-only-layers-viewer-sync branch from 2d67a4f to a2ea6f9 Compare October 18, 2023 14:45

kecnry reviewed Oct 18, 2023

View reviewed changes

kecnry approved these changes Oct 20, 2023

View reviewed changes

bmorris3 force-pushed the wcs-only-layers branch from 588e459 to 8a168bb Compare October 23, 2023 13:28

bmorris3 force-pushed the wcs-only-layers-viewer-sync branch from a2ea6f9 to 5e60a51 Compare October 23, 2023 13:30

bmorris3 force-pushed the wcs-only-layers branch 2 times, most recently from fd9d67d to 8a1bbaf Compare October 25, 2023 14:45

better viewer zoom/center sync with different refdata

b061435

bmorris3 force-pushed the wcs-only-layers-viewer-sync branch from 5e60a51 to b061435 Compare October 25, 2023 15:11

bmorris3 merged commit ed8890d into wcs-only-layers Oct 25, 2023
8 of 12 checks passed

bmorris3 mentioned this pull request Oct 25, 2023

ENH: Image rotation via WCS-only layers spacetelescope/jdaviz#2179

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better viewer zoom/center sync with different refdata #12

Better viewer zoom/center sync with different refdata #12

bmorris3 commented Oct 16, 2023

kecnry Oct 17, 2023

pllim commented Oct 17, 2023

bmorris3 commented Oct 17, 2023

pllim commented Oct 17, 2023

bmorris3 commented Oct 17, 2023

pllim commented Oct 17, 2023

kecnry commented Oct 17, 2023

codecov-commenter commented Oct 18, 2023

kecnry Oct 18, 2023

bmorris3 Oct 23, 2023

kecnry Oct 18, 2023

bmorris3 Oct 23, 2023

kecnry left a comment


		to_refdata = viewer.state.reference_data

		if orig_refdata != to_refdata and hasattr(self.viewer, '_get_fov'):

Better viewer zoom/center sync with different refdata #12

Better viewer zoom/center sync with different refdata #12

Conversation

bmorris3 commented Oct 16, 2023

Change log entry

Checklist for package maintainer(s)

kecnry Oct 17, 2023

Choose a reason for hiding this comment

pllim commented Oct 17, 2023

bmorris3 commented Oct 17, 2023

pllim commented Oct 17, 2023

bmorris3 commented Oct 17, 2023

pllim commented Oct 17, 2023

kecnry commented Oct 17, 2023

codecov-commenter commented Oct 18, 2023

Codecov Report

kecnry Oct 18, 2023

Choose a reason for hiding this comment

bmorris3 Oct 23, 2023

Choose a reason for hiding this comment

kecnry Oct 18, 2023

Choose a reason for hiding this comment

bmorris3 Oct 23, 2023

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment