BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits() #3249

pllim · 2024-10-23T20:04:07Z

Description

BUG: Fix spectrum and spaxel resetting X and Y limits.

TST: Add regression test that fails on main without this patch.

FEAT: Add viewer.get_limits() method for convenience and refactor some code to use it. Also refactor some code to use existing viewer.set_limits() method. Since these are not "public API," there is no need for change log for this part.

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. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

TST: Add regression test that fails on main without this patch. FEAT: Add viewer.get_limits() method for convenience and refactor some code to use it. Also refactor some code to use existing set_limits.

pllim · 2024-10-23T21:51:02Z

jdaviz/configs/cubeviz/plugins/tests/test_tools.py

 from regions import RectanglePixelRegion


-@pytest.mark.filterwarnings('ignore:No observer defined on WCS')


This warning no longer exist, so we can remove the ignore.

pllim · 2024-10-23T21:51:39Z

jdaviz/configs/cubeviz/plugins/tests/test_tools.py

 from regions import RectanglePixelRegion


-@pytest.mark.filterwarnings('ignore:No observer defined on WCS')
-def test_spectrum_at_spaxel(cubeviz_helper, spectrum1d_cube_with_uncerts):
+def test_spectrum_at_spaxel_no_alt(cubeviz_helper, spectrum1d_cube_with_uncerts):


Have to add "_no_alt" to the name, otherwise -k test_spectrum_at_spaxel selects all the other tests with the same prefix as well.

pllim · 2024-10-23T21:52:27Z

jdaviz/configs/cubeviz/plugins/tests/test_tools.py

@@ -47,6 +52,9 @@ def test_spectrum_at_spaxel(cubeviz_helper, spectrum1d_cube_with_uncerts):
        {'event': 'mouseleave', 'domain': {'x': x, 'y': y}, 'altKey': False})
    assert flux_viewer.toolbar.active_tool._mark.visible is False

+    # Check that X is still zoomed but Y is reset.
+    assert_allclose(spectrum_viewer.get_limits(), (4.623e-07, 4.6232e-07, 28, 92))


On main without this patch, this test would fail as such:

# Check that X is still zoomed but Y is reset. > assert_allclose(spectrum_viewer.get_limits(), (4.623e-07, 4.6232e-07, 28, 92)) jdaviz/configs/cubeviz/plugins/tests/test_tools.py:56: E AssertionError: E Not equal to tolerance rtol=1e-07, atol=0 E E Mismatched elements: 2 / 4 (50%) E Max absolute difference among violations: 4.00276968e-11 E Max relative difference among violations: 8.65800676e-05 E ACTUAL: array([4.6228e-07, 4.6236e-07, 2.8000e+01, 9.2000e+01]) E DESIRED: array([4.6230e-07, 4.6232e-07, 2.8000e+01, 9.2000e+01])

I updated the test to also account for Y limits, but you get the idea.

pllim · 2024-10-23T21:53:50Z

jdaviz/configs/default/plugins/tools.py

@@ -34,8 +31,7 @@ def activate(self):
            self._mark = PluginLine(self._profile_viewer, visible=False)
            self._profile_viewer.figure.marks = self._profile_viewer.figure.marks + [self._mark, ]
        # Store these so we can revert to previous user-set zoom after preview view
-        pv_state = self._profile_viewer.state
-        self._previous_bounds = [pv_state.x_min, pv_state.x_max, pv_state.y_min, pv_state.y_max]
+        self._previous_bounds = self._profile_viewer.get_limits()


I thought about having self._previous_bounds only keeping Y bounds but then decided not to, in case downstream somehow needs the X bounds too.

jdaviz/configs/default/plugins/tools.py

pllim · 2024-10-23T21:55:21Z

jdaviz/configs/default/plugins/viewers.py

@@ -166,6 +166,17 @@ def set_limits(self, x_min=None, x_max=None, y_min=None, y_max=None):
            if y_max is not None:
                self.state.y_max = y_max

+    def get_limits(self):


I find this very convenient, so might as well add it in this PR. Otherwise, I will never get to it.

codecov · 2024-10-23T22:12:14Z

Codecov Report

Attention: Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.63%. Comparing base (b47c128) to head (182ca3b).
Report is 115 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/configs/default/plugins/viewers.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3249   +/-   ##
=======================================
  Coverage   88.63%   88.63%           
=======================================
  Files         125      125           
  Lines       18779    18784    +5     
=======================================
+ Hits        16644    16649    +5     
  Misses       2135     2135

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

that would fail without patching Y zoom also Also keep Y zoom

pllim · 2024-10-24T16:14:24Z

jdaviz/configs/cubeviz/plugins/tools.py

+            self._profile_viewer.state.remove_callback(k, self.on_limits_change)
+        super().deactivate()
+
+    def on_limits_change(self, *args):


I purposely implement this only for Cubeviz and not its parent class because I have no idea what downstream where is using the parent class and want to keep my scope to fix Cubeviz only.

kecnry

just two small questions to consider - but don't think they need to block merge. Looks good to me and appreciate the code cleanup (even if it made parsing the diff a bit of a pain 😜 )

jdaviz/configs/cubeviz/plugins/tools.py

kecnry · 2024-10-25T18:28:45Z

jdaviz/configs/cubeviz/plugins/tools.py

+        finally:
+            self._is_moving = False


is it possible to send a limits change in before a mouseleave? I guess maybe from API commands submitted in a still running cell or something (or would those just block events anyways)?

Anything is possible via API, but in practice, will a user actually do that?

if not, then we probably don't have to reset to False until the mouseleave event, right?

Better be safe than sorry? It might be in a dirty state because I cannot guarantee otherwise.

because we gotta move it, move it.

javerbukh

LGTM, thanks!

pllim · 2024-10-28T18:46:04Z

Thanks, all!

…ng limits; FEAT: Add viewer.get_limits()

…9-on-v4.0.x Backport PR #3249 on branch v4.0.x (BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits())

…imits() (#3249) * BUG: Fix spectrum and spaxel resetting X limits. TST: Add regression test that fails on main without this patch. FEAT: Add viewer.get_limits() method for convenience and refactor some code to use it. Also refactor some code to use existing set_limits. * Fix cubeviz error and add test that would fail without patching Y zoom also Also keep Y zoom * Move _is_moving because we gotta move it, move it.

pllim added bug Something isn't working 💤backport-v4.0.x on-merge: backport to v4.0.x labels Oct 23, 2024

pllim added this to the 4.0.1 milestone Oct 23, 2024

github-actions bot added cubeviz mosviz imviz plugin Label for plugins common to multiple configurations labels Oct 23, 2024

pllim force-pushed the spaxel-zoom-fix branch from 8cc0474 to 5481e43 Compare October 23, 2024 20:05

BUG: Fix spectrum and spaxel resetting X limits.

6f6b8f6

TST: Add regression test that fails on main without this patch. FEAT: Add viewer.get_limits() method for convenience and refactor some code to use it. Also refactor some code to use existing set_limits.

pllim force-pushed the spaxel-zoom-fix branch from 5481e43 to 6f6b8f6 Compare October 23, 2024 21:50

pllim commented Oct 23, 2024

View reviewed changes

pllim marked this pull request as ready for review October 23, 2024 22:08

pllim requested review from rosteen, javerbukh, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners October 23, 2024 22:08

Fix cubeviz error and add test

d969ca1

that would fail without patching Y zoom also Also keep Y zoom

pllim force-pushed the spaxel-zoom-fix branch from 73052af to d969ca1 Compare October 24, 2024 16:11

pllim commented Oct 24, 2024

View reviewed changes

pllim changed the title ~~BUG: Fix spectrum and spaxel resetting X limits; FEAT: Add viewer.get_limits()~~ BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits() Oct 24, 2024

kecnry approved these changes Oct 25, 2024

View reviewed changes

Move _is_moving

182ca3b

because we gotta move it, move it.

pllim added the Ready for final review label Oct 25, 2024

javerbukh approved these changes Oct 28, 2024

View reviewed changes

pllim merged commit e5ce007 into spacetelescope:main Oct 28, 2024
25 checks passed

pllim deleted the spaxel-zoom-fix branch October 28, 2024 18:45

meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Oct 28, 2024

Backport PR spacetelescope#3249: BUG: Fix spectrum and spaxel resetti…

21dc457

…ng limits; FEAT: Add viewer.get_limits()

meeseeksmachine mentioned this pull request Oct 28, 2024

Backport PR #3249 on branch v4.0.x (BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits()) #3262

Merged

pllim added a commit that referenced this pull request Oct 28, 2024

Merge pull request #3262 from meeseeksmachine/auto-backport-of-pr-324…

5921669

…9-on-v4.0.x Backport PR #3249 on branch v4.0.x (BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits())

pllim mentioned this pull request Dec 17, 2024

BUG: Not all viewers have get_limits #3361

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits() #3249

BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits() #3249

pllim commented Oct 23, 2024 •

edited

Loading

pllim Oct 23, 2024

pllim Oct 23, 2024

pllim Oct 23, 2024

pllim Oct 24, 2024

pllim Oct 23, 2024

pllim Oct 23, 2024

codecov bot commented Oct 23, 2024 •

edited

Loading

pllim Oct 24, 2024

kecnry left a comment

kecnry Oct 25, 2024

pllim Oct 25, 2024

kecnry Oct 25, 2024

pllim Oct 25, 2024

javerbukh left a comment

pllim commented Oct 28, 2024

		from regions import RectanglePixelRegion


		@pytest.mark.filterwarnings('ignore:No observer defined on WCS')

BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits() #3249

BUG: Fix spectrum and spaxel resetting limits; FEAT: Add viewer.get_limits() #3249

Conversation

pllim commented Oct 23, 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

codecov bot commented Oct 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

kecnry 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

Choose a reason for hiding this comment

javerbukh left a comment

Choose a reason for hiding this comment

pllim commented Oct 28, 2024

pllim commented Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading