refactor mouseover coords info display #1976

kecnry · 2023-01-23T21:44:30Z

Description

This pull request refactors/renames the logic in the coordinates info display so that:

viewer callbacks are handled by the plugin itself rather than various viewers
traitlets for the coordinate info are named based on their position in the widget. Previously they were named based on their meaning in the original Imviz implementation, and then re-used for other viewers. This became quite confusing to develop. This change then also means all tests needed to be updated for the new formatting.

The behavior should essentially be the same as in #1894, except that the top line in specviz now reads "Cursor" instead of the previously hard-coded "Pixel".

Fix #1645

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/viewers.py

pllim

The logic route is more complicated now per mousemove. Any performance penalty?

kecnry · 2023-01-23T22:03:07Z

The logic route is more complicated now per mousemove. Any performance penalty?

I don't think it is - the callback goes directly to the method in the plugin to handle mousemove events. There is an if-statement there to check whether the viewer is an image or spectrum viewer... if that has any performance implications, we could move the if-statement to creating the callbacks instead but that just makes testing a little clunkier. There are a few other isinstance checks after that to determine the behavior (again, could be brought higher up if its a concern), but this also gets rid of if-statements for the event type since only mouse events (and not keypress or click events) are processed here.

But if you don't like the code move or find any performance decreases, we can also split that part out and just keep the changes to the traitlets.

kecnry · 2023-01-23T22:03:42Z

(I think this should pass tests, but will leave in draft until #1975 is resolved)

kecnry · 2023-01-24T18:25:03Z

If we go this route (moving event-logic into the plugin), this would also allow us to merge this functionality into the marker plugin (#1953), so the same codebase would show the mouseover display and allow logging that same information to an exportable table with a keypress.

As far as I know, we can't have a single plugin have vue files for both the tray and toolbar, so we'll need to decide where this code ultimately belongs and how to share it between the two plugins. I'm trying to use the same callback infrastructure there as in this PR, so hopefully moving that logic around should be straightforward 🤞

codecov · 2023-01-25T18:18:06Z

Codecov Report

Base: 91.95% // Head: 92.04% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (996c2e9) compared to base (434f644).
Patch coverage: 96.92% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1976      +/-   ##
==========================================
+ Coverage   91.95%   92.04%   +0.08%     
==========================================
  Files         140      140              
  Lines       15333    15148     -185     
==========================================
- Hits        14100    13943     -157     
+ Misses       1233     1205      -28

Impacted Files	Coverage Δ
jdaviz/configs/imviz/plugins/tools.py	`93.98% <ø> (-0.09%)`	⬇️
jdaviz/configs/mosviz/plugins/viewers.py	`88.88% <ø> (+1.21%)`	⬆️
...z/configs/imviz/plugins/coords_info/coords_info.py	`94.62% <94.06%> (-3.93%)`	⬇️
...eviz/plugins/moment_maps/tests/test_moment_maps.py	`100.00% <100.00%> (ø)`
...aviz/configs/cubeviz/plugins/tests/test_parsers.py	`100.00% <100.00%> (ø)`
jdaviz/configs/cubeviz/plugins/tests/test_tools.py	`100.00% <100.00%> (ø)`
jdaviz/configs/cubeviz/plugins/viewers.py	`100.00% <100.00%> (+8.16%)`	⬆️
...gins/gaussian_smooth/tests/test_gaussian_smooth.py	`100.00% <100.00%> (ø)`
jdaviz/configs/default/plugins/viewers.py	`96.94% <100.00%> (+0.17%)`	⬆️
jdaviz/configs/imviz/plugins/viewers.py	`91.13% <100.00%> (+2.30%)`	⬆️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pllim

I ran out of time, so this is just partial review. I will continue from test_linking.py when I come back.

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

pllim · 2023-01-26T22:23:49Z

jdaviz/configs/default/plugins/viewers.py

@@ -185,6 +186,17 @@ def _on_subset_create(self, msg):
        if msg.subset.label not in self._expected_subset_layers and msg.subset.label:
            self._expected_subset_layers.append(msg.subset.label)

+    @property
+    def active_image_layer(self):


This really only applies to Imviz, is it correct to have it so high up in JdavizViewerMixin?

It's applicable to any image viewer. This will just return None for a viewer without a valid image layer. But would you prefer either putting this in the AstrowidgetsImageViewerMixin or creating a new mixin for non-astrowidgets image-viewer-specific methods/properties?

Just leave it here then. This is not part of Astrowidgets API anyway. Thanks for the clarification.

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

pllim · 2023-01-26T22:30:43Z

jdaviz/configs/imviz/plugins/tools.py

@@ -122,10 +122,6 @@ def deactivate(self):
    def on_click(self, data):
        self.viewer.blink_once(reversed=data['event']=='contextmenu')  # noqa: E225

-        # Also update the coordinates display.
-        data['event'] = 'mousemove'


This was here so blink without moving cursor would update. Is this still the case?

Yes, this is handled by the _layers_changed callback method now

pllim · 2023-01-26T22:31:36Z

jdaviz/configs/imviz/plugins/viewers.py

            key_pressed = data['key']

            if key_pressed in ('b', 'B'):
                self.blink_once(reversed=key_pressed=='B')  # noqa: E225

-                # Also update the coordinates display.
-                data['event'] = 'mousemove'


Need to check if blink without moving cursor still correct.

(same as above - this should work, but please feel free to test and confirm or let me know if any case doesn't act as expected)

Looks like it works to me.

jdaviz/configs/imviz/tests/test_linking.py

rosteen

I'm ok with deferring efforts toward allowing customization, approved.

pllim · 2023-02-01T13:12:20Z

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

+                                        {'event': 'mousemove', 'domain': {'x': -1, 'y': 0}})
+    assert label_mouseover.as_text() == ('Pixel x=-1.0 y=00.0',  # Out of bounds
+                                         'World 13h41m45.5759s +27d00m12.3044s (ICRS)',
+                                         '205.4398995981 27.0034178810 (deg)')  # noqa


This RA and Dec should have changed because X is now -1. Only the value should not shown. The old test does not check for sky coordinates so I cannot tell you what it is supposed to be but you should double check. This does not look right.

A simple way to check is to create that same WCS object locally and then call its pixel_to_world method as per https://github.com/astropy/astropy-APEs/blob/main/APE14.rst#high-level-api

I went back to main and added debugging print statements to the original test and they gave these same values (that's not to say that they're necessarily correct). I'll need to think about the exact scenario here to see what would be expected, or I guess we could always file as a separate follow-up and only include as_text()[0] in the test coverage so that we don't suggest these values are validated as correct.

Hmm... But theoretically, this is what should happen. I wonder if I omitted the sky coordinates on purpose or this was an oversight. Past Self ™️ strikes again!

from astropy.wcs import WCS w = WCS({ 'WCSAXES': 3, 'CRPIX1': 38.0, 'CRPIX2': 38.0, 'CRPIX3': 1.0, 'CRVAL1': 205.4384, 'CRVAL2': 27.004754, 'CRVAL3': 4.890499866509344, 'CTYPE1': 'RA---TAN', 'CTYPE2': 'DEC--TAN', 'CTYPE3': 'WAVE', 'CUNIT1': 'deg', 'CUNIT2': 'deg', 'CUNIT3': 'um', 'CDELT1': 3.61111097865634E-05, 'CDELT2': 3.61111097865634E-05, 'CDELT3': 0.001000000047497451, # noqa 'PC1_1 ': -1.0, 'PC1_2 ': 0.0, 'PC1_3 ': 0, 'PC2_1 ': 0.0, 'PC2_2 ': 1.0, 'PC2_3 ': 0, 'PC3_1 ': 0, 'PC3_2 ': 0, 'PC3_3 ': 1, 'DISPAXIS': 2, 'VELOSYS': -2538.02, 'SPECSYS': 'BARYCENT', 'RADESYS': 'ICRS', 'EQUINOX': 2000.0, 'LONPOLE': 180.0, 'LATPOLE': 27.004754, 'MJDREFI': 0.0, 'MJDREFF': 0.0, 'DATE-OBS': '2014-03-30'})

>>> w.pixel_to_world(0, 0, 0) [<SkyCoord (ICRS): (ra, dec) in deg (205.4398996, 27.00341788)>, <SpectralCoord (target: <ICRS Coordinate: (ra, dec, distance) in (deg, deg, kpc) (205.4384, 27.004754, 1000.) (pm_ra_cosdec, pm_dec, radial_velocity) in (mas / yr, mas / yr, km / s) (0., 0., 0.)>) 4.89049987e-06 m>] >>> w.pixel_to_world(-1, 0, 0) [<SkyCoord (ICRS): (ra, dec) in deg (205.43994013, 27.00341788)>, <SpectralCoord (target: <ICRS Coordinate: (ra, dec, distance) in (deg, deg, kpc) (205.4384, 27.004754, 1000.) (pm_ra_cosdec, pm_dec, radial_velocity) in (mas / yr, mas / yr, km / s) (0., 0., 0.)>) 4.89049987e-06 m>]

So it turns out when you load that test case into Cubeviz, the flux cube WCS becomes a PaddedSpectrumWCS but the uncert cube retained the original FITS WCS. If you do this, you can recreate the weird results this test is seeing.

>>> cubeviz_helper.app.data_collection[1].coords.pixel_to_world(0, 0, 3) [<SpectralCoord (target: <ICRS Coordinate: (ra, dec, distance) in (deg, deg, kpc) (205.4384, 27.004754, 1000.) (pm_ra_cosdec, pm_dec, radial_velocity) in (mas / yr, mas / yr, km / s) (0., 0., 0.)>) 4.89049987e-06 m>, <SkyCoord (ICRS): (ra, dec) in deg (205.43977801, 27.00341788)>] >>> cubeviz_helper.app.data_collection[1].coords.pixel_to_world(-1, 0, 3) [<SpectralCoord (target: <ICRS Coordinate: (ra, dec, distance) in (deg, deg, kpc) (205.4384, 27.004754, 1000.) (pm_ra_cosdec, pm_dec, radial_velocity) in (mas / yr, mas / yr, km / s) (0., 0., 0.)>) 4.89149987e-06 m>, <SkyCoord (ICRS): (ra, dec) in deg (205.43977801, 27.00341788)>]

It makes no sense but (1) it is unrelated to this PR, and (2) it is not real data. So, my recommendation is that you retain the old test behavior by not comparing the sky coordinates at all. I'd rather we do that than to say in the test we expect non-sensible result.

I thought uncert would have the same WCS as flux cube... maybe @bmorris3 cube work would naturally fix this. 🤷

So, my recommendation is that you retain the old test behavior by not comparing the sky coordinates at all.

ok, good idea, I like that plan for now.

pllim

The tests LGTM now. Some minor comments.

I'll approve soon if quick manual testing don't break anything.

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

jdaviz/configs/cubeviz/plugins/viewers.py

pllim · 2023-02-07T18:30:51Z

jdaviz/configs/default/plugins/viewers.py

@@ -185,6 +186,17 @@ def _on_subset_create(self, msg):
        if msg.subset.label not in self._expected_subset_layers and msg.subset.label:
            self._expected_subset_layers.append(msg.subset.label)

+    @property
+    def active_image_layer(self):


Just leave it here then. This is not part of Astrowidgets API anyway. Thanks for the clarification.

jdaviz/configs/default/plugins/viewers.py

pllim · 2023-02-07T18:33:45Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

+        if isinstance(viewer,
+                      (SpecvizProfileView,
+                       ImvizImageView,
+                       CubevizImageView,
+                       MosvizImageView,
+                       MosvizProfile2DView)):


Is there a follow-up issue to make this plugin somewhat working for custom config?

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

pllim · 2023-02-07T18:44:05Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

+        self.row2_title = 'Wave'
+        self.row2_text = f'{closest_wave.value:10.5e} {closest_wave.unit.to_string()}'
+        if closest_wave.unit != u.pix:
+            self.row2_text += f' ({int(closest_i)} pix)'


I wonder if the int casting is still needed here now that we only display if it is within bounds of spectral axis.

pllim · 2023-02-07T20:12:51Z

This confused me. I moved the mouse over to the left viewer, which is supposed to be "A". I expected nothing gray out but I am seeing this. What is going on? Am I going crazy? "x" marks where my cursor was.

pllim · 2023-02-07T20:25:24Z

(The above screenshot was when using concepts/imviz_dithered_gwcs.ipynb in this repo.

pllim · 2023-02-07T21:42:40Z

Re: #1976 (comment)

I see same behavior on main (when the matched pan-zoom is triggered). I think it is because of this that is a headache for another day:

jdaviz/jdaviz/configs/imviz/plugins/tools.py

Line 40 in 3fc9a92

# Set the reference data in other viewers to be the same as the current viewer.

pllim · 2023-02-07T21:47:15Z

I think the "(est.)" is now gone from mouseover text, so https://jdaviz--1976.org.readthedocs.build/en/1976/imviz/displayimages.html#notes-on-gwcs needs to be updated to remove mention of "(est.)".

pllim · 2023-02-07T21:57:27Z

This one is definitely related (i.e., not happening in main).

Fire up concepts/imviz_compass_mpl.ipynb.
Run "Case 7".
Mouse over (0, 0) on "C". Then press "b" to blink through the images and back to "C". The sky coordinates should not be there. With this PR, if you mouse away and back, the sky coordinates disappear, but if you don't move the cursor and just press "b", it stays there (which is incorrect).

* refactored to bring mouseover logic into the plugin itself * traitlets for information are now based on their position in the UI rather than their meaning for imviz (which was then "stretched" to apply to other viewers)

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

kecnry · 2023-02-10T18:06:10Z

With this PR, if you mouse away and back, the sky coordinates disappear, but if you don't move the cursor and just press "b", it stays there (which is incorrect).

Fixed, good catch!

pllim

LGTM now. Thanks!

kecnry added the no-changelog-entry-needed changelog bot directive label Jan 23, 2023

kecnry added this to the 3.3 milestone Jan 23, 2023

github-actions bot added cubeviz imviz mosviz specviz specviz2d labels Jan 23, 2023

kecnry force-pushed the coords-info-refactor branch from 2c59039 to e5fa123 Compare January 23, 2023 21:50

pllim reviewed Jan 23, 2023

View reviewed changes

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

pllim reviewed Jan 23, 2023

View reviewed changes

kecnry force-pushed the coords-info-refactor branch from e5fa123 to 39ad22d Compare January 24, 2023 16:25

kecnry force-pushed the coords-info-refactor branch 5 times, most recently from bca282c to e415dd1 Compare January 25, 2023 17:22

kecnry marked this pull request as ready for review January 25, 2023 18:36

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino and bmorris3 as code owners January 25, 2023 18:36

pllim reviewed Jan 26, 2023

View reviewed changes

kecnry force-pushed the coords-info-refactor branch from 1e0c3cc to eb7409d Compare January 27, 2023 16:18

pllim reviewed Jan 27, 2023

View reviewed changes

jdaviz/configs/imviz/tests/test_linking.py Outdated Show resolved Hide resolved

pllim reviewed Jan 27, 2023

View reviewed changes

jdaviz/configs/imviz/tests/test_linking.py Outdated Show resolved Hide resolved

kecnry force-pushed the coords-info-refactor branch 4 times, most recently from e98c955 to 506f709 Compare January 30, 2023 20:16

rosteen approved these changes Jan 31, 2023

View reviewed changes

kecnry added the Ready for final review label Jan 31, 2023

pllim reviewed Feb 1, 2023

View reviewed changes

kecnry force-pushed the coords-info-refactor branch 2 times, most recently from 17d98d0 to d9e0a92 Compare February 6, 2023 13:54

pllim reviewed Feb 7, 2023

View reviewed changes

pllim mentioned this pull request Feb 7, 2023

BUG: Cubeviz parser gives wrong sky coordinates for non-flux cube under certain condition #1991

Closed

rosteen modified the milestones: 3.3, 3.4 Feb 9, 2023

pllim mentioned this pull request Feb 9, 2023

Fix Cubeviz coordinates panel with inaccurate sky coordinates sometimes #2002

Closed

13 tasks

kecnry and others added 4 commits February 10, 2023 12:50

refactor mouseover coords info display

87f0562

* refactored to bring mouseover logic into the plugin itself * traitlets for information are now based on their position in the UI rather than their meaning for imviz (which was then "stretched" to apply to other viewers)

Apply suggestions from code review

8769e1a

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

Apply suggestions from code review

e406748

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

clear world coords when appropriate on blink

996c2e9

kecnry force-pushed the coords-info-refactor branch from 158d3b9 to 996c2e9 Compare February 10, 2023 18:05

pllim approved these changes Feb 10, 2023

View reviewed changes

pllim merged commit dad439d into spacetelescope:main Feb 10, 2023

kecnry deleted the coords-info-refactor branch February 10, 2023 20:56

This was referenced Mar 3, 2023

Fix Cubeviz mouseover bugs, add WCS to moment map #2009

Merged

markers plugin #1953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor mouseover coords info display #1976

refactor mouseover coords info display #1976

kecnry commented Jan 23, 2023 •

edited by pllim

Loading

pllim left a comment

kecnry commented Jan 23, 2023

kecnry commented Jan 23, 2023

kecnry commented Jan 24, 2023

codecov bot commented Jan 25, 2023 •

edited

Loading

pllim left a comment

pllim Jan 26, 2023

kecnry Jan 27, 2023

pllim Feb 7, 2023

pllim Jan 26, 2023

kecnry Jan 27, 2023

pllim Jan 26, 2023

kecnry Jan 27, 2023

rosteen Jan 30, 2023

rosteen left a comment

pllim Feb 1, 2023

pllim Feb 1, 2023

kecnry Feb 1, 2023

pllim Feb 1, 2023

pllim Feb 3, 2023

kecnry Feb 6, 2023

pllim left a comment

pllim Feb 7, 2023

pllim Feb 7, 2023

pllim Feb 7, 2023

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

kecnry commented Feb 10, 2023

pllim left a comment

refactor mouseover coords info display #1976

refactor mouseover coords info display #1976

Conversation

kecnry commented Jan 23, 2023 • edited by pllim Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Jan 23, 2023

kecnry commented Jan 23, 2023

kecnry commented Jan 24, 2023

codecov bot commented Jan 25, 2023 • edited Loading

Codecov Report

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

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

rosteen 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

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

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

pllim commented Feb 7, 2023

kecnry commented Feb 10, 2023

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Jan 23, 2023 •

edited by pllim

Loading

codecov bot commented Jan 25, 2023 •

edited

Loading