Fix matched pan zoom #29

bmorris3 · 2024-01-04T21:20:13Z

Description

This PR fixes matched pan/zoom in Imviz. 🐱

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

jdaviz/configs/imviz/plugins/orientation/orientation.py

kecnry · 2024-01-05T13:19:04Z

jdaviz/configs/imviz/plugins/orientation/orientation.py

+                if wcs_layer not in self.app.get_viewer_by_id(viewer_ref).state.wcs_only_layers:
+                    self.app.add_data_to_viewer(viewer_ref, wcs_layer)
+
+            self.viewer.selected = viewer_ref


this will actually cycle the viewer select in the orientation plugin, right? Is that for the sake of having the choices update in self.orientation? Why do we need to do that now for all viewers_to_update?

We need that because we call self.orientation.update_wcs_only_filter below, which is particular to each viewer.

but self.orientation won't remember per-viewer filters, so setting for anything but the last iteration of the for-loop will just be wasted effort... I think

The one time that this is necessary is when a new viewer is created and the WCS-only reference data have been added to the viewer but not yet selected as reference data. We first need to change the filter to "see" the orientation options, then we need to select the default orientation – I do this a few lines below (https://github.com/bmorris3/jdaviz/pull/29/files/e2f36a547e94cb0d38e45b807cb7dfba38c85122#diff-022bcba39f3fd68183229b5433146c2a21ac800db2778790152edc9a6b1b01caR398).

ok, and in that case viewers_to_update is passed and only has one entry it seems. I wonder if instead of having

self.viewer.selected = viewer_ref ... self.orientation.selected = base_wcs_layer_label

in the for-loop and run for all (passed) viewers, we should instead have

if self.viewer.selected == viewer: .... self.orientation.selected = base_wcs_layer_label

Does that make sense?

I understand what you're going for, but this method is also used when new orientation layers are created. In that case, it adds the new orientation layer to each viewer, so that it can be selected as refdata in that viewer. So the function is written for the general case.

jdaviz/configs/imviz/plugins/orientation/orientation.py

jdaviz/core/template_mixin.py

kecnry · 2024-01-05T13:30:48Z

jdaviz/app.py

@@ -2254,7 +2255,7 @@ def _create_viewer_item(self, viewer, vid=None, name=None, reference=None):
            'config': self.config,  # give viewer access to app config/layout
            'data_open': False,
            'collapse': True,
-            'reference': reference,
+            'reference': reference or name or vid,


why was this needed (specifically for this PR - I agree we should try to merge these eventually, I just worry this might add more confusion since now we'll have "reference" in the viewer item that isn't necessarily the same as "reference" on the viewer itself).

This is needed because when a new image viewer is created, there are hooks in place to set vid and name, but not reference (https://github.com/spacetelescope/jdaviz/blob/6372879516cdb96081e90b4b5ced404dc66448fe/jdaviz/configs/imviz/helper.py#L54C28-L56), but the reference is the version used in some places.

jdaviz/configs/imviz/plugins/orientation/orientation.py

pllim

Technical review as follows. Thanks!

pllim · 2024-01-12T14:53:12Z

jdaviz/app.py

@@ -2319,6 +2320,12 @@ def _on_new_viewer(self, msg, vid=None, name=None, add_layers_to_viewer=False):
        ref_data = self._jdaviz_helper.default_viewer._obj.state.reference_data
        new_viewer_item['reference_data_label'] = getattr(ref_data, 'label', None)

+        if (
+                getattr(ref_data, 'meta', {}).get('_WCS_ONLY', False) and


Is it more consistent to use layer_is_wcs_only function here that you added in spacetelescope#2179 in Imviz helper, or is there a reason to not do that here?

I had avoided doing that only because of circular imports, originally. But I've pushed this commit which imports the function within the method and dodges the circularity.

pllim · 2024-01-12T14:57:09Z

jdaviz/configs/imviz/plugins/tools.py

@@ -51,7 +51,10 @@ def on_click(self, data):
        if x is None or y is None:  # Out of bounds
            return
        x, y, _, _ = self.viewer._get_real_xy(image, x, y)
-        self.viewer.center_on((x, y))
+        if image.coords is not None:
+            self.viewer.center_on(image.coords.pixel_to_world(x, y))


Why this logic? It kinda defeats the purpose of _get_real_xy above.

The use of _get_real_xy above ensures that the behavior for data with WCS (in the else) and without WCS (in the if) is consistent. image here is the first visible layer, which is not the reference data layer when linked by WCS. So this call tries to be explicit.

If you want to switch the call to using sky coordinates, you don't need _get_real_xy. You can theoretically pass (x, y) directly to reference WCS because input (x, y) is always w.r.t. reference data.

So do you want to see line 53 moved inside the conditional?

If this line makes the final cut, I think you can change the call to something like reference_data.coords.pixel_to_world(x, y) without ever calling viewer._get_real_xy(image, ...).

But I am not sure what you need to do to fix #29 (comment)

This line is already out of the final cut 👍🏻

pllim · 2024-01-12T14:57:44Z

jdaviz/configs/imviz/tests/test_tools.py

Why did the results change?

These test results have changed because the matched pan/zoom approach for data with WCS is slightly different than before, producing limits different by one pixel. The main differences come from trying to match zoom approximately even when viewers have different reference data.

I have to think about this:

When you give 'domain': {'x': 1, 'y': 1}, that is where you are telling the reference data to go. In this case, v has both images loaded, so second image is on top. Second image is offset by +1 pix in the WCS. So if the adjusted limits reflects second image limits, then I think this change makes sense.

But then on L63-L62, v2 should only have the first image, and both v and v2 give the same limits. Then the assumption above breaks down. Let me run this example interactively and see what is actually going on.

I still don't get this change. This test case is linked by pixel. Even though both images have WCS, it should not have mattered. No orientation layer was created.

With this PR, when I run this test case interactively, this centered the image at x=2.5 y=1.5:

t_normpan = v.toolbar.tools['jdaviz:imagepanzoom'] t_normpan.activate() t_normpan.on_click({'event': 'click', 'domain': {'x': 1, 'y': 1}})

And if I have the pan/zoom tool active, where a single click would center to the pixel under cursor, that pixel is not actually the center when "pan to click location" is done.

I do not think this change in test behavior is correct.

On main, the same test case does not have this x offset. Pixel under cursor is centered though I suspect there might be a 0.5 offset in y but hard to tell without built in grids on the viewer.

pllim · 2024-01-12T14:58:51Z

jdaviz/configs/imviz/wcs_utils.py

-    sequential_data = np.arange(
-        np.prod(refdata_shape), dtype=np.int8
-    ).reshape(refdata_shape)
+    placeholder_data = np.nan * np.ones(refdata_shape)


What is the benefit of changing sequential to NaN? Kinda make it hard to debug when the fake layer accidentally shows up.

This was discussed in an earlier PR that we merged, but then I botched a rebase, so I'm fixing that mistake in this PR. Since this was approved before, I'm just reinserting it.

There are two main benefits if the "WCS-only reference data" have no data:

when a WCS-only layer is created, it is visible for a few milliseconds, before the visibility is set to False. If the data are nan, they are never rendered, and the viewer does less crazy flashing during rotation.

If you somehow do math on the WCS-only layer's pixels, the result will never appear to be legitimate. This should not be easy to do, given the checks we have in place, but switching to nan makes it much easier to debug if we accidentally do operations on the WCS-only data.

Yes, I think the benefit not having the weird flashing is good. Thanks for the clarification!

pllim · 2024-01-12T15:01:13Z

jdaviz/core/astrowidgets_api.py

-            i_top = get_top_layer_index(self)
-            image = self.layers[i_top].layer
+            if self.state.reference_data is not None:
+                image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference)


I don't understand this change. Zoom should be based on the visible layer, not the hidden reference data.

Zoom should be based on the visible layer

Why? If they're linked, is this distinction meaningful?

Because I want the zoom level of the image under view, not zoom level of the reference data.

Ah, no, it is completely wrong. I forgot to link by WCS when I commented 8 mins ago. When I link by WCS, the zoom_level is wrong. This change needs to be reverted.

To give you a sense of what I expect, in the ImvizDitheredExample notebook, a "fit" zoom would be roughly 0.2. A imviz.default_viewer.zoom_level of 1 would be like this:

It should not matter if I am linking by WCS or not.

pllim · 2024-01-12T15:02:24Z

jdaviz/core/template_mixin.py

+            `True` will filter only the WCS-only layers, `False` will
+            give the non-WCS-only layers.
+        """
+        def is_wcs_only(data):


I feel like this re-implemented layer_is_wcs_only that you also implemented in Imviz helper.

This needs to be a separate function that you can turn on and off as a filter for LayerSelect.

pllim · 2024-01-12T15:03:56Z

jdaviz/core/template_mixin.py

+            self.add_filter(is_wcs_only)
+
+    @property
+    def only_wcs_layers(self):


How is this different from get_wcs_only_layer_labels? Please clarify in a docstring.

pllim · 2024-01-12T15:05:49Z

jdaviz/core/tools.py

+                viewer.center_on(sky_cen)
+
+            else:
+                with delay_callback(viewer.state, *self.match_keys):


Some comments to explain the intention of this logic block would ease future debugging, in case this math does not work for all use cases.

bmorris3 · 2024-01-12T18:38:07Z

@pllim 5c3d48b reverts the change to the center_on call, which allows me to revert the expected test results successfully.

pllim · 2024-01-12T19:03:35Z

It is unfortunate the remote data job is failing for a different reason, so we cannot completely convince ourselves via CI until after rebase.

pllim

Also, any chance we can turn Cami's use case that started all this into a test?

I already forgot her workflow. 😆

pllim

When you run the ImvizDitheredExample notebook (on this branch without rebasing on top of main), linked by WCS, 2 viewers, zoomed in a little -- Do you see the second viewer flicker when you linked pan/zoom the first one? I see it flicker a few times before settling.

Also, once I switch the tool from linked to normal pan/zoom (or box zoom), I can no longer switch back, as in when I right-click on the tool, there is no linked option anymore.

Neither of these are a problem on main.

pllim · 2024-01-12T21:45:01Z

jdaviz/core/astrowidgets_api.py

-            i_top = get_top_layer_index(self)
-            image = self.layers[i_top].layer
+            if self.state.reference_data is not None:
+                image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference)


i_ref is not used.

Suggested change

image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference)

image, _ = get_reference_image_data(self.jdaviz_app, self.reference)

pllim · 2024-01-12T21:45:51Z

jdaviz/core/astrowidgets_api.py

-            i_top = get_top_layer_index(self)
-            image = self.layers[i_top].layer
+            if self.state.reference_data is not None:
+                image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference)


Suggested change

image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference)

image, _ = get_reference_image_data(self.jdaviz_app, self.reference)

bmorris3 · 2024-01-16T21:36:30Z

@pllim I'm hopeful 3c5fb47 fixed up the data menu problems. Seems to work for me. 🤞🏻

jdaviz/core/astrowidgets_api.py

pllim · 2024-01-16T23:28:52Z

jdaviz/core/astrowidgets_api.py

-            image = self.layers[i_top].layer
+            if self.state.reference_data is not None:
+                image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference)
+            else:


Is this else ever used? I don't think we would encounter a viewer without reference data but has a top layer displayed.

pllim · 2024-01-16T23:32:46Z

jdaviz/core/tools.py

+                old_level = viewer.zoom_level
+                viewer.zoom_level = old_level * float(to_fov_sky / orig_fov_sky)
+                viewer.center_on(sky_cen)


This is the part I was referring to when I say you can roll your own calculations if we ever decide that Astrowidgets API cannot make the results here reasonable. cc @kecnry

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

pllim · 2024-01-16T23:48:01Z

@bmorris3 , alas, does not work for me. Maybe another dev can check if it is just me or it is really still a problem.

pllim · 2024-01-16T23:51:20Z

Could be just me because now even on the new machine, the weird data menu behavior that somehow only plagues me in the old machine is appearing again. 😭

kecnry · 2024-01-17T13:29:46Z

@pllim - that first screenshot looks like it was fixed by spacetelescope#2605 (the main image rotation PR probably just needs a rebase to get that in).

pllim

Why don't we just merge then, so rebase can happen? Thanks!

fix matched pan zoom

e2f36a5

github-actions bot added the imviz label Jan 4, 2024

kecnry reviewed Jan 5, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/orientation/orientation.py Outdated Show resolved Hide resolved

kecnry reviewed Jan 5, 2024

View reviewed changes

Update jdaviz/configs/imviz/plugins/orientation/orientation.py

f8ede98

bmorris3 commented Jan 5, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/orientation/orientation.py Outdated Show resolved Hide resolved

bmorris3 and others added 4 commits January 5, 2024 11:40

Update jdaviz/configs/imviz/plugins/orientation/orientation.py

89dfa63

Update jdaviz/configs/imviz/plugins/orientation/orientation.py

d6f5525

fixes from github suggestions

fedbdf1

review comments from Kyle

131a412

pllim reviewed Jan 12, 2024

View reviewed changes

fix centering on click test

5c3d48b

bmorris3 force-pushed the wcs-only-layers-matchpanzoom branch from e366643 to 5c3d48b Compare January 12, 2024 18:36

pllim reviewed Jan 12, 2024

View reviewed changes

bmorris3 added 2 commits January 12, 2024 14:52

reducing redundancy

ea13cea

add warning to astrowidgets_api zoom_level

b155027

pllim reviewed Jan 12, 2024

View reviewed changes

data menu whac-a-mole fixes

3c5fb47

pllim reviewed Jan 16, 2024

View reviewed changes

jdaviz/core/astrowidgets_api.py Outdated Show resolved Hide resolved

pllim reviewed Jan 16, 2024

View reviewed changes

Update jdaviz/core/astrowidgets_api.py

cd5aafc

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

pllim approved these changes Jan 17, 2024

View reviewed changes

bmorris3 merged commit 7c1a556 into wcs-only-layers Jan 17, 2024
9 of 12 checks passed

bmorris3 mentioned this pull request Jan 17, 2024

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

Merged

11 tasks

	image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference)
	image, _ = get_reference_image_data(self.jdaviz_app, self.reference)

Fix matched pan zoom #29

Fix matched pan zoom #29

Conversation

bmorris3 commented Jan 4, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

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

Choose a reason for hiding this comment

pllim 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

bmorris3 Jan 12, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

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

bmorris3 commented Jan 12, 2024

pllim commented Jan 12, 2024

pllim left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmorris3 commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Jan 16, 2024

pllim commented Jan 16, 2024

kecnry commented Jan 17, 2024

pllim left a comment

Choose a reason for hiding this comment

bmorris3 commented Jan 4, 2024 •

edited

Loading

bmorris3 Jan 12, 2024 •

edited

Loading