image viewer zoom-level/center #2649

kecnry · 2024-01-05T20:33:03Z

Description

This pull request moves the logic for translating between zoom limits (in x and y) and zoom level/center from the astrowidgets API into the viewer state and builds on #2604 to allow accessing and modifying these from the plot options plugin.

Screen.Recording.2024-01-11.at.12.51.20.PM.mov

NOTE: dynamic step size requires upstream support glue-viz/glue-jupyter#417

TODO:

initial population of zoom_level and zoom_center before any change to limits
rebase on top of Expose spectrum-viewer bounds in UI #2604 and add to plot options.
fix failing test (ensure same astrowidgets behavior as previously)
handle/prevent negative zoom-level (already have traceback, but should prevent from UI with validation error, preferably via glue mechanism)
resolve change in centering logic: https://github.com/spacetelescope/jdaviz/pull/2649/files#r1443351929
decide how to handle pixel vs world coordinates from users based on link type (especially with upcoming changes in image rotation)

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

pllim

You might wanna wait. I redid some of the math to account for Brett's rotation work on his branch.

kecnry · 2024-01-05T20:37:35Z

You might wanna wait.

I think that should be pretty easy to migrate here, no?

pllim · 2024-01-05T20:38:15Z

Yes. But you will have conflicts.

jdaviz/core/astrowidgets_api.py

kecnry · 2024-01-09T18:16:34Z

jdaviz/core/astrowidgets_api.py

-
-        with delay_callback(self.state, 'x_min', 'x_max'):
-            self.state.x_min = new_x_min - 0.5
-            self.state.x_max = new_x_max - 0.5


@pllim should this be + 0.5 instead (assuming the 0.5 is to account for the margin added around the data)? Otherwise, where is this coming from?

I don't quite remember either and #744 has no record of it. I think it has something to do with user input being pixel center but viewer wants pixel corner. Not 100% sure.

I was sure I worked out the math properly though using API + mouse cursor info. Does it not work anymore?

I was seeing wandering when changing the zoom-level and switching to + seemed to fix it, but maybe introduced that somewhere else. I'll go back to main and see if I can trigger the same there or not. Either way, doesn't seem to be the culprit for the failing test.

pllim · 2024-01-09T18:54:16Z

Just focusing on the first failure I see, looks like centering failed. You have array([-0.5, 9.5, -0.5, 9.5]) which means it is still centering on the image center, not the (0, 1) I told it to go to.

    def test_center_offset_pixel(self):
        self.viewer.center_on((0, 1))
>       assert_allclose((self.viewer.state.x_min, self.viewer.state.x_max,
                         self.viewer.state.y_min, self.viewer.state.y_max),
                        (-5, 5, -4, 6))

pllim · 2024-01-09T18:55:38Z

Try run the test case interactively on a notebook. Does it work there?

kecnry · 2024-01-09T19:19:10Z

Some of the failures here I have a hacky workaround locally, I just need to figure out the right way to do it, but there are a few others that still remain after that as well.

kecnry · 2024-01-11T14:47:31Z

jdaviz/configs/cubeviz/plugins/viewers.py

@@ -31,9 +32,12 @@ class CubevizImageView(JdavizViewerMixin, BqplotImageView):
                ]

    default_class = None
+    _state_cls = FreezableBqplotImageViewerState


this is was the reason for the cubeviz test failure - this forces cubeviz to use Imviz's reset_limits which behaves differently, even after accounting for different pixel components. We may eventually want to update reset_limits and/or the tests so that the image viewers in cubeviz and Imviz behave consistently when resetting limits, but that is out of scope here.

By Imviz's reset limits, do you mean this?

jdaviz/jdaviz/core/astrowidgets_api.py

Lines 195 to 198 in 00954e1

# Zoom on X and Y will auto-adjust.

if val == 'fit':

self.state.reset_limits()

return

I farmed it back out to glue-jupyter. Nothing fancy there.

No, I meant FreezableBqplotImageViewerState.reset_limits() which overrides glue's BqplotImageViewerState.reset_limits()

Looks like you implemented this 2 years ago in #1897

I know, and that so far has not been enabled in cubeviz.

And then that particular line was changed from a different logic by me later in #1928

I don't think there is any point to make Imviz and Cubeviz consistent. Imviz has WCS linking to worry about, Cubeviz does not.

kecnry · 2024-01-11T14:48:07Z

jdaviz/core/freezable_state.py

+                pixel_id_x = [comp for comp in pixel_ids if comp.label.endswith('[x]')][0]
+                pixel_id_y = [comp for comp in pixel_ids if comp.label.endswith('[y]')][0]

-                x_max = max(x_max, layer.layer.data[pixel_ids[1]].max() + 0.5)
-                y_max = max(y_max, layer.layer.data[pixel_ids[0]].max() + 0.5)
+                x_max = max(x_max, layer.layer.data[pixel_id_x].max() + 0.5)
+                y_max = max(y_max, layer.layer.data[pixel_id_y].max() + 0.5)


this technically isn't necessary, but will help future-proof if we ever do enable this custom reset_limits for any cube viewer.

But now it depends on the label name. Is the label always guaranteed to have x/y in the name?

that seems to be more general/reliable than assuming the first and second index 🤷‍♂️ I can revert for now though and deal with this if/when we try to make the image viewers in configs more consistent.

pllim · 2024-01-11T16:07:19Z

This is the only error I am seeing:

AttributeError: type object 'BqplotImageViewerState' has no attribute 'zoom_center_x'

kecnry · 2024-01-11T16:22:00Z

That is the mosviz error I am still working on addressing.

If you remove the if-statement within FreezableBqplotImageViewerState.reset_limits() then you'll see the cubeviz tests fail because of the different behavior between that and the upstream reset_limits(). I fixed it for now by just still sending cubeviz to the upstream implementation since it feels out of scope here to change the zoom behavior itself that would require a change to the tests.

codecov · 2024-01-11T17:39:12Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (189276d) 90.81% compared to head (e380a96) 90.83%.
Report is 18 commits behind head on main.

Files	Patch %	Lines
jdaviz/core/freezable_state.py	77.38%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2649      +/-   ##
==========================================
+ Coverage   90.81%   90.83%   +0.02%     
==========================================
  Files         162      163       +1     
  Lines       20962    21502     +540     
==========================================
+ Hits        19036    19532     +496     
- Misses       1926     1970      +44

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

javerbukh

Zooming and centering work as expected, even when viewers are linked. Plot options also handles different combinations of multiselect well, even when an image and spectrum viewer are selected. Nice work!

kecnry · 2024-02-02T14:28:38Z

@javerbukh - since your approval, this has been updated to expose zoom-radius instead of zoom-level (with different translation logic), but the UI remains essentially unchanged. Feel free to dismiss your review though if you see anything in the latest two commits that you think warrants a fresh look.

bmorris3

Something is amiss in Imviz w/o WCS linking. Changing zoom radius in the plugin causes jumping in the viewer, and does not change the zoom radius.

Screen.Recording.2024-02-02.at.09.45.20.mov

bmorris3 · 2024-02-02T14:52:07Z

When linked by WCS, zoom radius step size of "1" (degree?) is too big to be useful, and should be restricted to positive values.

In some other Plot Options context (stretch histogram maybe?), we adjust the up/down arrow step size depending on the content of the viewer. I think we need that here.

Also, we need some kind of unit on the zoom center (x, y) and radius. When WCS-linked, it's degrees for x and y, and maybe also radius. I'd suggest that zoom radius should be displayed in units of arcsec/arcmin.

Screen.Recording.2024-02-02.at.09.48.22.mov

jdaviz/core/freezable_state.py

pllim

Since changes to Astrowidgets API so far is minimal, I do not have concern on the design. When you have made more progress, the user docs definitely would need to be updated. Also does not hurt to update the example notebook(s) to demonstrate this new way of zooming.

Thanks!

* NOTE: widget will not respect step until upstream PR is merged/pinned, but can be accessed (for testing purposes) with plot_options._obj.zoom_step

kecnry · 2024-02-02T20:39:03Z

Something is amiss in Imviz w/o WCS linking. Changing zoom radius in the plugin causes jumping in the viewer, and does not change the zoom radius.

I think the zoom radius was working, it just isn't obvious with the stepsize. Dynamic step-size is now implemented, but you'll need glue-viz/glue-jupyter#417 in order to see it in action (without that upstream PR, the stepsize will fallback on 1).

Jumping is as fixed as I can get it 😬

When linked by WCS, zoom radius step size of "1" (degree?) is too big to be useful, and should be restricted to positive values.
In some other Plot Options context (stretch histogram maybe?), we adjust the up/down arrow step size depending on the content of the viewer. I think we need that here.

Re step size: see response above.

Re negative radius: limiting the value through glue is difficult, so I now just gracefully handle a negative radius 🤷‍♂️

Also, we need some kind of unit on the zoom center (x, y) and radius. When WCS-linked, it's degrees for x and y, and maybe also radius. I'd suggest that zoom radius should be displayed in units of arcsec/arcmin.

For now I just expose these as degrees to avoid unit-conversion. I can see the argument for making zoom-radius a "finer" unit, but suspect we'll want to just adhere to display units if/when we implement them for the spatial direction on images.

jdaviz/core/freezable_state.py

javerbukh

Re-checked the code and tested the functionality and all still looks good. Others have mentioned the jumping but I can understand that that may not be fixable at the moment. As long as we have a ticket to track that this should be good to merge, thanks!

cshanahan1

tested it again, looks good to me

kecnry added this to the 3.9 milestone Jan 5, 2024

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

pllim reviewed Jan 5, 2024

View reviewed changes

kecnry force-pushed the image-viewer-center-state branch from a9c4418 to 37e50f6 Compare January 5, 2024 20:42

kecnry commented Jan 5, 2024

View reviewed changes

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

kecnry mentioned this pull request Jan 8, 2024

Expose spectrum-viewer bounds in UI #2604

Merged

kecnry force-pushed the image-viewer-center-state branch 2 times, most recently from 60b249b to 5bc43a6 Compare January 8, 2024 18:46

kecnry commented Jan 9, 2024

View reviewed changes

kecnry force-pushed the image-viewer-center-state branch from 20f8d44 to 121fc7a Compare January 10, 2024 17:17

kecnry commented Jan 11, 2024

View reviewed changes

kecnry force-pushed the image-viewer-center-state branch 2 times, most recently from dc2678d to 5a4cdeb Compare January 11, 2024 15:14

kecnry marked this pull request as ready for review January 11, 2024 17:49

kecnry requested review from rosteen, javerbukh, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners January 11, 2024 17:49

javerbukh approved these changes Jan 11, 2024

View reviewed changes

expose zoom-radius instead of zoom-level in state/plot options

5468fb1

kecnry force-pushed the image-viewer-center-state branch from a5b2ce7 to 5468fb1 Compare February 1, 2024 19:53

bmorris3 reviewed Feb 2, 2024

View reviewed changes

pllim reviewed Feb 2, 2024

View reviewed changes

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

pllim reviewed Feb 2, 2024

View reviewed changes

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

pllim reviewed Feb 2, 2024

View reviewed changes

dynamic zoom step-size and units

89d3bc0

* NOTE: widget will not respect step until upstream PR is merged/pinned, but can be accessed (for testing purposes) with plot_options._obj.zoom_step

kecnry force-pushed the image-viewer-center-state branch from a5e853c to 89d3bc0 Compare February 2, 2024 20:30

gracefully handle negative zoom radius

dcaa2e4

kecnry added 2 commits February 5, 2024 08:42

smarter viewer logic when setting zoom step

903a12a

revert center_on logic

ddf3c0d

kecnry marked this pull request as ready for review February 5, 2024 14:25

kecnry force-pushed the image-viewer-center-state branch from dbb3e33 to e6c2fc5 Compare February 5, 2024 18:07

pllim reviewed Feb 5, 2024

View reviewed changes

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

optimize world_to_pixel_values calls

e380a96

kecnry force-pushed the image-viewer-center-state branch from e6c2fc5 to e380a96 Compare February 5, 2024 20:06

javerbukh approved these changes Feb 5, 2024

View reviewed changes

cshanahan1 approved these changes Feb 5, 2024

View reviewed changes

kecnry enabled auto-merge (squash) February 5, 2024 21:47

kecnry merged commit e3cce1e into spacetelescope:main Feb 5, 2024
14 of 15 checks passed

kecnry deleted the image-viewer-center-state branch February 6, 2024 13:12

gibsongreen pushed a commit to gibsongreen/jdaviz that referenced this pull request Feb 12, 2024

image viewer zoom-level/center (spacetelescope#2649)

aa8200f

pllim mentioned this pull request Feb 26, 2024

tests failing with jdaviz main spacetelescope/lcviz#92

Closed

rosteen mentioned this pull request Mar 6, 2024

Get dev tests to pass spacetelescope/lcviz#93

Closed

pllim mentioned this pull request Mar 11, 2024

_set_axes_lim called too many times when Imviz adjusts more than one limit at once #2746

Open

kecnry mentioned this pull request Mar 13, 2024

TST: See if percentile is breaking devdeps (try 2) spacetelescope/lcviz#96

Closed

kecnry mentioned this pull request Mar 28, 2024

Dev test fix take two kecnry/lcviz#8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image viewer zoom-level/center #2649

image viewer zoom-level/center #2649

kecnry commented Jan 5, 2024 •

edited

Loading

pllim left a comment

kecnry commented Jan 5, 2024

pllim commented Jan 5, 2024

kecnry Jan 9, 2024

pllim Jan 9, 2024

pllim Jan 9, 2024

kecnry Jan 9, 2024

pllim commented Jan 9, 2024

pllim commented Jan 9, 2024

kecnry commented Jan 9, 2024

kecnry Jan 11, 2024 •

edited

Loading

pllim Jan 11, 2024

kecnry Jan 11, 2024

pllim Jan 11, 2024

kecnry Jan 11, 2024

pllim Jan 11, 2024

pllim Jan 11, 2024

kecnry Jan 11, 2024

pllim Jan 11, 2024

kecnry Jan 11, 2024

pllim commented Jan 11, 2024

kecnry commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

javerbukh left a comment

kecnry commented Feb 2, 2024

bmorris3 left a comment

bmorris3 commented Feb 2, 2024 •

edited

Loading

pllim left a comment

kecnry commented Feb 2, 2024 •

edited

Loading

javerbukh left a comment

cshanahan1 left a comment

	# Zoom on X and Y will auto-adjust.
	if val == 'fit':
	self.state.reset_limits()
	return

image viewer zoom-level/center #2649

image viewer zoom-level/center #2649

Conversation

kecnry commented Jan 5, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Jan 5, 2024

pllim commented Jan 5, 2024

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 Jan 9, 2024

pllim commented Jan 9, 2024

kecnry commented Jan 9, 2024

kecnry Jan 11, 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

pllim commented Jan 11, 2024

kecnry commented Jan 11, 2024 • edited Loading

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

javerbukh left a comment

Choose a reason for hiding this comment

kecnry commented Feb 2, 2024

bmorris3 left a comment

Choose a reason for hiding this comment

bmorris3 commented Feb 2, 2024 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Feb 2, 2024 • edited Loading

javerbukh left a comment

Choose a reason for hiding this comment

cshanahan1 left a comment

Choose a reason for hiding this comment

kecnry commented Jan 5, 2024 •

edited

Loading

kecnry Jan 11, 2024 •

edited

Loading

kecnry commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

bmorris3 commented Feb 2, 2024 •

edited

Loading

kecnry commented Feb 2, 2024 •

edited

Loading