View spectrum from single spaxel on hover #2647

rosteen · 2024-01-05T17:03:11Z

Now shows spectrum and auto-scales spectrum viewer when hovering with the single-spaxel subset tool activated, see demo:

Screen.Recording.2024-01-05.at.11.59.02.AM.mov

Opened as draft since I still need to add tests and update docs. Outstanding question: do we want to reset the spectrum viewer limits to the default when deactivating the tool/leaving the viewer area?

kecnry · 2024-01-05T17:07:42Z

Outstanding question: do we want to reset the spectrum viewer limits to the default when deactivating the tool/leaving the viewer area?

What if instead we just have it overlay with a normalized y-axis (the values would no longer match the left y-axis, but you would be able to see the shape of the spectrum at the pixel) 🤔 ?

rosteen · 2024-01-05T17:21:14Z

What if instead we just have it overlay with a normalized y-axis (the values would no longer match the left y-axis, but you would be able to see the shape of the spectrum at the pixel) 🤔 ?

Interesting idea, definitely has some pros and cons. Probably a question for the POs on their preference.

kecnry · 2024-01-05T17:24:47Z

Interesting idea, definitely has some pros and cons. Probably a question for the POs on their preference.

Agreed.

Either way... I definitely think that:

we probably want to respect user-set x-limits
we definitely want the spectrum-viewer to return to exactly as it was before any hovering (i.e. not end up resetting limits or anything like that just because of a hover event)

pllim

Is this smart enough to know when it is hovering over something that isn't a flux cube or does it not matter?

What if you have it active, and then do unit conversion? Is that possible yet?

rosteen · 2024-01-05T20:03:49Z

Is this smart enough to know when it is hovering over something that isn't a flux cube or does it not matter?

What if you have it active, and then do unit conversion? Is that possible yet?

Right now it just grabs the first visible layer in the viewer, I should add a check to make sure it's 3D at least. And unit conversion is not available in Cubeviz currently.

kecnry · 2024-01-05T20:48:07Z

Is this smart enough to know when it is hovering over something that isn't a flux cube or does it not matter?

Hmmm, that's a good point though, you could in theory have the uncertainty or mask cube in that viewer, and so maybe we should use the logic from #2646 to always use the flux-cube, since that is what creating the subset will end up doing anyways (I think).

Codestyle Codestyle

rosteen · 2024-01-08T14:47:31Z

* we probably want to respect user-set x-limits

* we definitely want the spectrum-viewer to return to exactly as it was before any hovering (i.e. not end up resetting limits or anything like that just because of a hover event)

This already respected the user-set x-limits, I just implemented reverting to the previous zoom on mouseleave events or tool deactivation (in case it's deactivated from the API without mouse leaving the viewer).

codecov · 2024-01-08T14:58:33Z

Codecov Report

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

Comparison is base (03edac6) 91.53% compared to head (2b2bd3e) 91.52%.
Report is 9 commits behind head on main.

Files	Patch %	Lines
jdaviz/configs/cubeviz/plugins/tools.py	87.50%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2647      +/-   ##
==========================================
- Coverage   91.53%   91.52%   -0.01%     
==========================================
  Files         161      161              
  Lines       20056    20196     +140     
==========================================
+ Hits        18358    18485     +127     
- Misses       1698     1711      +13

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

rosteen · 2024-01-09T17:44:27Z

This is what the other zoom option for the preview (overlaying it over current zoom) would look like. I was leaning toward zooming to the preview, but now I think this might be nicer so that you don't potentially have other spectra constantly moving around as the zoom changes. The downside is not being able to see what the flux values of the preview are.

Screen.Recording.2024-01-09.at.12.36.33.PM.mov

kecnry · 2024-01-16T14:09:02Z

jdaviz/configs/cubeviz/plugins/tools.py

+    def activate(self):
+        self.viewer.add_event_callback(self.on_mouse_move, events=['mousemove', 'mouseleave'])
+        if self._spectrum_viewer is None:
+            self._spectrum_viewer = self.viewer.jdaviz_helper.app.get_viewer('spectrum-viewer')


can we avoid hardcoding 'spectrum-viewer' here by using the helper attributes?

still think this might be worth doing to avoid future headache

Done, grabs the first profile viewer now.

kecnry · 2024-01-16T14:11:20Z

jdaviz/configs/cubeviz/plugins/tools.py

+        y = int(np.round(data['domain']['y']))
+
+        # Use first visible layer for now
+        cube_data = [layer.layer for layer in self.viewer.layers if layer.state.visible][0]


should this use similar logic as #2646 to ensure its pulling from the flux cube? What happens when there are no visible layers (I'm guessing a traceback... we might want to just skip that case gracefully)?

I changed this to return on an empty result, good catch.

The logic in 2646 might be useful, but I don't think it's worth holding this up for that to be merged. We could potentially do that if/when we change the axis handling.

ok, but the first visible layer now could still be a flat image, or the uncertainty or mask cubes, right?

I added a check to at least only use 3D data, avoiding a traceback if something like a collapsed image is displayed.

thinking about this more... I think this should listen to the layer selection in mouseover itself, which also might fix the bug with the uncertainty cube failing to display. That or we just ALWAYS pull the flux cube, regardless of the viewer and whether it is an active layer in that viewer or not 🤷

jdaviz/configs/cubeviz/plugins/tools.py

javerbukh

The spectrum changing is so responsive, nice work!

kecnry · 2024-01-16T15:30:48Z

jdaviz/configs/cubeviz/plugins/tools.py

+        if len(cube_data) == 0:
+            return
+        cube_data = cube_data[0]
+        spectrum = cube_data.get_object(statistic=None)


is it worth caching this?

I thought about getting this on activation but was worried about data changing while the tool is active, and since it seems performant I didn't think it was worth the overhead of adding a hub listener or something along those lines.

I think this might not be performant on large cubes (or anything where get_object is expensive), but haven't tested. I guess we can leave that for future improvement, since depending on the decision of how to handle layer selection, you may need to have a listener to invalidate the cache

rosteen · 2024-01-19T20:31:47Z

@kecnry This now tries to preview the same data as selected for the coords info hover, before falling back on any 3D data available if the selected layer isn't 3D. I also fixed the uncertainty preview. Mind giving this another look?

kecnry · 2024-01-22T17:57:30Z

jdaviz/configs/cubeviz/plugins/tools.py

+        else:
+            for layer in self.viewer.layers:
+                if layer.layer.label == coords_info_dataset and layer.visible:
+                    if isinstance(layer, BqplotImageSubsetLayerArtist):
+                        # cannot expose info for spatial subset layers
+                        continue
+                    cube_data = layer.layer
+                    break


can this rely on dataset.selected_obj (which will then get caching for free)? Or do you need some layer info that isn't accessible there?

That also then might let you mouseover the uncertainty viewer but access the mouseover from the flux cube (if selected manually in the mouseover cycler) which would be super cool!

I was just copying the logic from coords_info, using selected_obj seems to work with an extra check for glue Data vs Spectrum1D. I do worry that it will be confusing for users rather than a good feature that if they select the hover option in a viewer, they might be seeing the data from another viewer if it's selected at the top level.

hmmm true, I think most people don't even use the cycler though, so probably won't run into this except for advanced use-cases. Might be worth some user-testing or just see if anyone complains about confusion.

kecnry

Works well! Just a few comments above and I think this is good to go! (Although would be nice to get code coverage a little higher if that isn't too difficult - many of the uncovered lines are just different mouseout situations which probably don't need specific coverage)

…ewer specifically

* Working on adding spectrum-from-spaxel on hover * First working version of spectrum on hover * Handle NaNs properly * Update docs and changelog * Add simple test Codestyle Codestyle * Revert zoom after preview view * Address review comments * Only use 3D data * Fix preview spectrum for uncertainty cube * Use the same layer selection as coords_info if possible * Codestyle * Use selected_obj and get first profile viewer rather than spectrum-viewer specifically * Remove unused import * Improve test coverage

github-actions bot added cubeviz plugin Label for plugins common to multiple configurations labels Jan 5, 2024

rosteen added this to the 3.9 milestone Jan 5, 2024

pllim reviewed Jan 5, 2024

View reviewed changes

rosteen added 6 commits January 8, 2024 09:16

Working on adding spectrum-from-spaxel on hover

07b9799

First working version of spectrum on hover

956cffb

Handle NaNs properly

e448c6c

Update docs and changelog

5f9b843

Add simple test

ec03400

Codestyle Codestyle

Revert zoom after preview view

cb3b1ed

rosteen force-pushed the hover-spectrum branch from a410c78 to cb3b1ed Compare January 8, 2024 14:45

rosteen force-pushed the hover-spectrum branch from 628f38c to cb3b1ed Compare January 12, 2024 16:17

rosteen marked this pull request as ready for review January 12, 2024 16:21

rosteen requested review from javerbukh, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners January 12, 2024 16:21

kecnry reviewed Jan 16, 2024

View reviewed changes

Address review comments

1d4be47

rosteen mentioned this pull request Jan 16, 2024

Hover spectrum with extra axis #2661

Draft

javerbukh approved these changes Jan 16, 2024

View reviewed changes

kecnry added the Ready for final review label Jan 16, 2024

Only use 3D data

05bd078

kecnry reviewed Jan 16, 2024

View reviewed changes

rosteen added 3 commits January 19, 2024 14:54

Fix preview spectrum for uncertainty cube

29cd90c

Use the same layer selection as coords_info if possible

5145cc4

Codestyle

b652e16

kecnry reviewed Jan 22, 2024

View reviewed changes

rosteen added 3 commits January 22, 2024 14:19

Use selected_obj and get first profile viewer rather than spectrum-vi…

34d2c98

…ewer specifically

Remove unused import

bd81c8a

Improve test coverage

2b2bd3e

kecnry approved these changes Jan 22, 2024

View reviewed changes

rosteen merged commit 8edbdf0 into spacetelescope:main Jan 22, 2024
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View spectrum from single spaxel on hover #2647

View spectrum from single spaxel on hover #2647

rosteen commented Jan 5, 2024 •

edited

Loading

kecnry commented Jan 5, 2024

rosteen commented Jan 5, 2024

kecnry commented Jan 5, 2024

pllim left a comment

rosteen commented Jan 5, 2024

kecnry commented Jan 5, 2024

rosteen commented Jan 8, 2024

codecov bot commented Jan 8, 2024 •

edited

Loading

rosteen commented Jan 9, 2024 •

edited

Loading

kecnry Jan 16, 2024

kecnry Jan 22, 2024

rosteen Jan 22, 2024

kecnry Jan 16, 2024

rosteen Jan 16, 2024

rosteen Jan 16, 2024

kecnry Jan 16, 2024

rosteen Jan 16, 2024

kecnry Jan 16, 2024

javerbukh left a comment

kecnry Jan 16, 2024

rosteen Jan 16, 2024

kecnry Jan 16, 2024

rosteen commented Jan 19, 2024

kecnry Jan 22, 2024

kecnry Jan 22, 2024

rosteen Jan 22, 2024

kecnry Jan 22, 2024

kecnry left a comment

View spectrum from single spaxel on hover #2647

View spectrum from single spaxel on hover #2647

Conversation

rosteen commented Jan 5, 2024 • edited Loading

kecnry commented Jan 5, 2024

rosteen commented Jan 5, 2024

kecnry commented Jan 5, 2024

pllim left a comment

Choose a reason for hiding this comment

rosteen commented Jan 5, 2024

kecnry commented Jan 5, 2024

rosteen commented Jan 8, 2024

codecov bot commented Jan 8, 2024 • edited Loading

Codecov Report

rosteen commented Jan 9, 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

javerbukh 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

rosteen commented Jan 19, 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

kecnry left a comment

Choose a reason for hiding this comment

rosteen commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 8, 2024 •

edited

Loading

rosteen commented Jan 9, 2024 •

edited

Loading