Improve coordinates display panel for spectrum viewer #1894

pllim · 2022-12-05T23:51:35Z

Description

This pull request is to implement the following feature request:

The display should show the pixel in x on the first line and spectral axis and flux on the second line with appropriate units.
The cursor should lock to the spectrum in x only.

TODO

Fix Cubeviz. It uses PaddedSpectrumWCS and not SpectralCoordinates.
Fix Mosviz.
Specviz2d? Need support specreduce 1.3 #1889
Generalize the new code?
Make it work on collapsed Subset
Visualization like https://tradingeconomics.com/commodity/lumber minus the dashed lines. See marker for specviz coordinate mouseover pllim/jdaviz#8
- Is marker behavior acceptable? Is it too confusing to have both marker and the cursor?
- Are different display formats acceptable when tool is toggled on and off?
Acceptable performance (especially using MANGA cube in Cubeviz)

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

codecov · 2022-12-21T23:38:35Z

Codecov Report

Base: 91.86% // Head: 91.93% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (18c9d34) compared to base (c7a8023).
Patch coverage: 93.87% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1894      +/-   ##
==========================================
+ Coverage   91.86%   91.93%   +0.07%     
==========================================
  Files         140      140              
  Lines       15069    15310     +241     
==========================================
+ Hits        13843    14076     +233     
- Misses       1226     1234       +8

Impacted Files	Coverage Δ
jdaviz/core/marks.py	`91.08% <80.00%> (-0.45%)`	⬇️
jdaviz/configs/specviz/plugins/viewers.py	`86.11% <85.29%> (+0.65%)`	⬆️
jdaviz/app.py	`94.41% <85.71%> (+0.47%)`	⬆️
...gins/gaussian_smooth/tests/test_gaussian_smooth.py	`100.00% <100.00%> (ø)`
...z/configs/imviz/plugins/coords_info/coords_info.py	`98.55% <100.00%> (+0.67%)`	⬆️
jdaviz/configs/imviz/tests/test_linking.py	`100.00% <100.00%> (ø)`
jdaviz/configs/mosviz/tests/test_data_loading.py	`100.00% <100.00%> (ø)`
jdaviz/configs/specviz/tests/test_helper.py	`100.00% <100.00%> (ø)`
jdaviz/configs/specviz2d/tests/test_parsers.py	`100.00% <100.00%> (ø)`
...default/plugins/gaussian_smooth/gaussian_smooth.py	`97.77% <0.00%> (-1.12%)`	⬇️
... and 1 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 · 2022-12-21T23:44:42Z

jdaviz/configs/default/plugins/gaussian_smooth/tests/test_gaussian_smooth.py

-    gs = GaussianSmooth(app=app)
-    gs.dataset_selected = 'test'
+    gs = cubeviz_helper.plugins['Gaussian Smooth']._obj
+    gs.dataset_selected = 'test[FLUX]'


There is slight behavior change when you use the proper route to load the data into Cubeviz. I think this is more correct as a test because this is how user will actually load it.

pllim · 2022-12-21T23:45:42Z

jdaviz/configs/default/plugins/gaussian_smooth/tests/test_gaussian_smooth.py


-@pytest.mark.filterwarnings("ignore::UserWarning")


I moved the warning check to the affected line. This way, we won't accidentally ignore warning that we're not supposed to ignore.

pllim · 2022-12-21T23:52:32Z

p.s. I actually think it is ok to merge this without the marker thingy, but I can also wait if everyone else wants the marker to be part of this PR.

kecnry · 2022-12-29T19:56:22Z

jdaviz/configs/specviz/plugins/viewers.py

+                    # TODO: Is there a way to cache this?
+                    sp = lyr.layer.get_object(
+                        cls=Spectrum1D, statistic=getattr(self.state, 'function', None))


can you just use the arrays in the Lines object instead? If that isn't accessible from the layer, we could instead loop through self.figure.marks (but would then need to filter out marks that are not corresponding to data entries, etc).

I can try using the marks if you tell me how to properly grab them. I get lost in all the spectrum viewer internals.

Will using marks be compatible with unit conversion (when it finally works) and what is the risk of it breaking if we refactor the display stuff inside spectrum viewer?

Also, would using marks work for Cubeviz and Mosviz?

You should be able to pull the units from the figure as well, but will also need to account for as_steps similar to how is done for the cubeviz slider

jdaviz/jdaviz/configs/cubeviz/helper.py

Lines 103 to 106 in 66adb92

x_all = sv.native_marks[0].x

if sv.state.layers[0].as_steps:

# then the marks have been doubled in length (each point duplicated)

x_all = x_all[::2]

Either way, it's probably useful to cache in the plugin and update whenever the underlying data changes, but that might require a significant refactor of the plugin code. Right now, cubeviz is very noticeably laggy, I suspect because of this line and the extra expense of re-collapsing and creating a Spectrum1D object at every mouse move event)

Screen.Recording.2022-12-30.at.8.44.26.AM.mov

In that case... I wonder if part of this can be refactored out so all the viz with spectrum viewer can reuse this same code. What do you think?

jdaviz/jdaviz/configs/cubeviz/helper.py

Line 89 in 66adb92

def select_wavelength(self, wavelength):

I think the logic needs to be quite different here (can't just assume the first marks entry in specviz, for example). But if there is any part that can be refactored to be shared, I'm all for that. But with the complication of as_steps, I'm beginning to think just caching might be the cleanest way to go.

The cleanest way to cache is probably deep upstream here though I have a feeling they won't want it because of extra memory usage...

https://github.com/glue-viz/glue/blob/2293019762136c8cda1ce67e9c89d5513013d7e7/glue/core/data_collection.py#L414

kecnry · 2022-12-29T20:15:34Z

The logic for selecting the closest spectrum doesn't behave as I'd expect. See the following example (which uses pllim#8 to display a marker at the currently exposed coordinates):

Screen.Recording.2022-12-29.at.3.13.37.PM.mov

pllim · 2022-12-29T22:00:49Z

The logic for selecting the closest spectrum doesn't behave as I'd expect

I think I know what is happening. The distance math is pretty basic radius calculation using raw wave and flux values. So in the video, your mouse position results in distance in Y dominating the calculations (flux value is much higher than wave) and my math does not check if the X is outside a spectrum's wave bounds. So I think the fix shouldn't be hard. Good catch!

pllim · 2022-12-29T22:35:57Z

Re: #1894 (comment)

~~Actually, I have trouble reproducing the problem you pointed out using dummy data. How are those spectra generated?~~

import numpy as np
from astropy import units as u
from specutils import Spectrum1D

from jdaviz import Specviz

wave_1 = np.linspace(6000, 7000, 10) * u.AA
flux_1 = ([1200] * wave_1.size) * u.nJy
sp_1 = Spectrum1D(flux=flux_1, spectral_axis=wave_1)

wave_2 = wave_1 + (800 * u.AA)
flux_2 = ([60] * wave_2.size) * u.nJy  # corrected
sp_2 = Spectrum1D(flux=flux_2, spectral_axis=wave_2)

specviz_helper = Specviz()

specviz_helper.load_spectrum(sp_1, data_label='left')
specviz_helper.load_spectrum(sp_2, data_label='right')

specviz_helper.show()

pllim · 2022-12-29T23:17:57Z

display a marker at the currently exposed coordinates

I find it quite disorienting to see the jumpy marker and the mouse moving at different rates when I do mouseover. I wonder if @Jenneh has ideas.

kecnry · 2022-12-30T15:02:02Z

I find it quite disorienting to see the jumpy marker and the mouse moving at different rates when I do mouseover

Does pllim#8 (comment) help at all (hiding the marker during zoom/subset actions)?

My vote would be to include the marker for now since I think it adds a lot of important context to the mouseover display and avoids confusion/misinterpretation, and if it becomes annoying to us or users we can consider styling tweaks (color, marker, markersize, etc) and/or a toggle somewhere (perhaps directly in the space occupied by the mouseover coordinates since there isn't anything to display there when not hovering over a plot - see #1713 for a similar concept).

kecnry · 2023-01-05T19:36:31Z

Display has been updated to show cursor coordinates on the top row and move the index to the spectral axis row, and changing the marker to a vertical rectangle. Any refactoring and ability to "lock" a layer will be deferred for follow-up efforts.

Screen.Recording.2023-01-05.at.2.37.37.PM.mov

rosteen

Looks good to me, the feature works and seems quite performant. I checked that it works for multiple spectra, including a collapsed spatial subset. The "closest spectrum" behavior seems intuitive for well separated spectra, although I can see how it might get annoying with overlapping spectra - but locking is a follow up.

The only quibble I have is that I would prefer the marker (the blue rectangle) to be thinner, but that's not a blocker.

bmorris3

This looks great! Leaving approval with a few comments.

bmorris3 · 2023-01-17T21:39:17Z

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

@@ -53,3 +83,7 @@ def set_coords(self, sky, unreliable_world=False, unreliable_pixel=False):
            self.world_dec_deg = world_dec_deg
            self.unreliable_world = unreliable_world
            self.unreliable_pixel = unreliable_pixel
+            if unreliable_world:


Isn't this statement always true? 😏

bmorris3 · 2023-01-17T21:44:40Z

jdaviz/configs/specviz/plugins/viewers.py

+                            if _class == Spectrum1D:
+                                layer_data = lyr.get_object(cls=_class, statistic=statistic)
+                            else:
+                                layer_data = lyr.get_object(cls=_class)


Will this logic ignore any masks associated with the spectrum, which could be accessible via get_subset_object rather than get_object?

I am not familiar with how mask is supposed to work in Jdaviz. What is the workflow I should try to answer this question?

Actually, now that I look at the diff again, these calls also predate the PR, so addressing this comment is out of scope here.

bmorris3 · 2023-01-17T21:45:41Z

jdaviz/configs/specviz/plugins/viewers.py


-                data_x = layer_state.layer.data.get_object().spectral_axis
-                data_y = layer_state.layer.data.get_object().flux.value
+                data_obj = lyr.data.get_object()


Should we be explicit that cls=Spectrum1D here, or is that guaranteed elsewhere? When we support translation to/from NDDataArray from astropy/astropy#14175, we should be prepared to be specific.

Here, I merely cached the get_object() output. The actual call predates this PR, so addressing this is out of scope here.

bmorris3 · 2023-01-17T21:51:10Z

jdaviz/configs/specviz/plugins/viewers.py

+                    if cache_key in self.jdaviz_app._get_object_cache:
+                        sp = self.jdaviz_app._get_object_cache[cache_key]
+                    else:
+                        sp = self.jdaviz_app.get_data_from_viewer('spectrum-viewer',


Could we change 'spectrum-viewer' to self.jdaviz_app._default_spectrum_viewer_reference_name?

Done. Good catch!

in all the configurations supported by Jdaviz.

when out of bounds.

spec1d mouseover. Also remove unnecessary re-computation in the internals. A new internal cache on app level is introduced.

* only used in spectrum profile viewers * currently ALWAYS on

(or default tool - slice in cubeviz, for example)

* including clearing cache when subset is changed and ignoring hidden layers

pllim · 2023-01-19T16:15:06Z

~~With what Kyle did here, the slice info is lost. For Cubeviz, it doesn't matter. I did like it for Specviz but I guess it is not that useful...~~ Oh wait, I see it now. I missed the parenthesis.

pllim · 2023-01-19T16:21:32Z

To me, would be nicer if the marker is semi-transparent or just a simple "x" but I should defer to @Jenneh on this in a follow-up effort after this PR is merged.

pllim

@kecnry did an awesome job polishing up this PR and adding much values to it while I was out. I am okay with the changes and I think I addressed remaining comments from @bmorris3 .

Given there are 2 approvals and work waiting on this, I am merging it. Thanks, all!

pllim added this to the 3.2 milestone Dec 5, 2022

github-actions bot added imviz specviz labels Dec 5, 2022

pllim added cubeviz mosviz specviz2d plugin Label for plugins common to multiple configurations and removed imviz labels Dec 5, 2022

This comment was marked as resolved.

Sign in to view

pllim force-pushed the show-me-the-axey branch 2 times, most recently from 3886384 to b1f465d Compare December 21, 2022 23:26

pllim commented Dec 21, 2022

View reviewed changes

pllim marked this pull request as ready for review December 21, 2022 23:53

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino, kecnry and bmorris3 as code owners December 21, 2022 23:53

kecnry reviewed Dec 29, 2022

View reviewed changes

kecnry mentioned this pull request Dec 29, 2022

marker for specviz coordinate mouseover pllim/jdaviz#8

Closed

1 task

This comment was marked as resolved.

Sign in to view

pllim force-pushed the show-me-the-axey branch from b1f465d to bae928f Compare December 29, 2022 23:08

kecnry force-pushed the show-me-the-axey branch 5 times, most recently from 9be2cf4 to f2600e9 Compare January 5, 2023 18:57

kecnry force-pushed the show-me-the-axey branch from 644e980 to 9d06095 Compare January 5, 2023 19:39

kecnry marked this pull request as ready for review January 5, 2023 19:52

rosteen approved these changes Jan 17, 2023

View reviewed changes

bmorris3 approved these changes Jan 17, 2023

View reviewed changes

bmorris3 reviewed Jan 17, 2023

View reviewed changes

bmorris3 mentioned this pull request Jan 18, 2023

Proposal for a toggle setting for mouseover marker pllim/jdaviz#9

Closed

9 tasks

pllim and others added 9 commits January 19, 2023 11:00

Improved spectrum info panel for spectrum viewers

686d0c2

in all the configurations supported by Jdaviz.

BUG: Fix spectrum mouseover

aa30511

when out of bounds.

PERF: Decrease lag for Cubeviz

a1bb696

spec1d mouseover. Also remove unnecessary re-computation in the internals. A new internal cache on app level is introduced.

basic implementation of mouseover marker

ac3871e

* only used in spectrum profile viewers * currently ALWAYS on

only show marker (and nearest spectrum details) if no tool selected

71625dc

(or default tool - slice in cubeviz, for example)

support for mouseover of spatial subsets in cubeviz

129820d

* including clearing cache when subset is changed and ignoring hidden layers

remilestone changelog to 3.3

c74be61

add cursor and pixel information to mouseover display

19fb2b2

mouseover marker as rectangle

ef6d3b7

Do not hardcode spectrum-viewer

18c9d34

pllim force-pushed the show-me-the-axey branch from 9d06095 to 18c9d34 Compare January 19, 2023 16:24

pllim commented Jan 19, 2023

View reviewed changes

pllim merged commit 8f8106b into spacetelescope:main Jan 19, 2023

pllim deleted the show-me-the-axey branch January 19, 2023 16:56

kecnry mentioned this pull request Jan 23, 2023

refactor mouseover coords info display #1976

Merged

9 tasks

bmorris3 mentioned this pull request Jan 30, 2023

ENH: Adding Plot Options toggle for mouseover info/marker #1986

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve coordinates display panel for spectrum viewer #1894

Improve coordinates display panel for spectrum viewer #1894

pllim commented Dec 5, 2022 •

edited

Loading

This comment was marked as resolved.

codecov bot commented Dec 21, 2022 •

edited

Loading

pllim Dec 21, 2022

pllim Dec 21, 2022

pllim commented Dec 21, 2022

kecnry Dec 29, 2022

pllim Dec 29, 2022

pllim Dec 29, 2022

kecnry Dec 30, 2022

pllim Dec 30, 2022

kecnry Dec 30, 2022

pllim Dec 30, 2022 •

edited

Loading

kecnry commented Dec 29, 2022

pllim commented Dec 29, 2022

pllim commented Dec 29, 2022 •

edited

Loading

This comment was marked as resolved.

pllim commented Dec 29, 2022

kecnry commented Dec 30, 2022

kecnry commented Jan 5, 2023 •

edited

Loading

rosteen left a comment

bmorris3 left a comment

bmorris3 Jan 17, 2023

bmorris3 Jan 17, 2023

pllim Jan 19, 2023

pllim Jan 19, 2023

bmorris3 Jan 17, 2023

pllim Jan 19, 2023

bmorris3 Jan 17, 2023

pllim Jan 19, 2023

pllim commented Jan 19, 2023 •

edited

Loading

pllim commented Jan 19, 2023

pllim left a comment

	x_all = sv.native_marks[0].x
	if sv.state.layers[0].as_steps:
	# then the marks have been doubled in length (each point duplicated)
	x_all = x_all[::2]


		@pytest.mark.filterwarnings("ignore::UserWarning")

Improve coordinates display panel for spectrum viewer #1894

Improve coordinates display panel for spectrum viewer #1894

Conversation

pllim commented Dec 5, 2022 • edited Loading

Description

TODO

Change log entry

Checklist for package maintainer(s)

This comment was marked as resolved.

codecov bot commented Dec 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Dec 21, 2022

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 Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

kecnry commented Dec 29, 2022

pllim commented Dec 29, 2022

pllim commented Dec 29, 2022 • edited Loading

This comment was marked as resolved.

pllim commented Dec 29, 2022

kecnry commented Dec 30, 2022

kecnry commented Jan 5, 2023 • edited Loading

rosteen left a comment

Choose a reason for hiding this comment

bmorris3 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

pllim commented Jan 19, 2023 • edited Loading

pllim commented Jan 19, 2023

pllim left a comment

Choose a reason for hiding this comment

pllim commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 21, 2022 •

edited

Loading

pllim Dec 30, 2022 •

edited

Loading

pllim commented Dec 29, 2022 •

edited

Loading

kecnry commented Jan 5, 2023 •

edited

Loading

pllim commented Jan 19, 2023 •

edited

Loading