Expose spectrum-viewer bounds in UI #2604

rosteen · 2023-12-11T19:18:24Z

This allows the user to update the bounds of the spectrum viewer from the Plot Options plugin. I erred on the conservative side for now, and only have this active in single-select with spectrum-viewer chosen, or in multi-select if only spectrum-viewer is chosen.

Still needs tests, and it probably actually needs to handle the case of multiple non-image viewers already - I was thinking that everything in Jdaviz only has a single spectrum-viewer, but @kecnry does lcviz have multiple profile viewers already that we would want to update with this?

codecov · 2023-12-11T19:31:56Z

Codecov Report

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

Comparison is base (88a5ed1) 91.52% compared to head (b31e268) 91.53%.

Files	Patch %	Lines
...nfigs/default/plugins/plot_options/plot_options.py	92.98%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2604      +/-   ##
==========================================
+ Coverage   91.52%   91.53%   +0.01%     
==========================================
  Files         161      161              
  Lines       19995    20056      +61     
==========================================
+ Hits        18300    18359      +59     
- Misses       1695     1697       +2

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

kecnry · 2023-12-11T20:22:22Z

lcviz doesn't have profile viewers, but it definitely would be nice for this to be able to work there. The only thing that I think needs to be disabled for now is image viewers (because we need to respect aspect ratio, which probably means setting the center and zoom level, or deciding to recompute limits that encompass the user-provided bounds, etc).

jdaviz/configs/default/plugins/plot_options/plot_options.vue

camipacifici · 2023-12-11T20:38:53Z

I just tried this in Cubeviz and it works really well!! Thank you!!

kecnry · 2023-12-11T21:51:09Z

@rosteen - could we use something like #2606 to handle mixed state (by having an overlay over the entire widget you created if mixed state is detected in any of the inputs)?

rosteen · 2023-12-19T21:16:04Z

@camipacifici Would you try this again, please? Due to some updates it now live updates the viewer bounds as you edit the fields, I want to check that that behavior is ok for this.

camipacifici · 2023-12-19T21:21:07Z

I like it! Responds well. It's ok for me.

cshanahan1

This looks good so far, just adding some comments now and i'll continue to test and review:

a 'reset' button might be nice to have in the plugin. i know the home button will do the same thing but i think it would be nice to also have it here.
display less decimal points?
are there any consequences to having an empty entry for a boundary? you can delete one of the bounds entirely and if you click off it it doesn't snap back to its previous value or zero. this doesn't seem to matter but im not sure if the viewer limits are being used elsewhere and this could cause issues

kecnry · 2023-12-28T21:01:59Z

jdaviz/configs/default/plugins/plot_options/plot_options.py

@@ -753,14 +753,19 @@ def _update_viewer_bound_steps(self, msg={}):

        viewer = self.viewer.selected_obj[0] if self.viewer_multiselect else self.viewer.selected_obj # noqa
        if not isinstance(viewer.state, ImageViewerState):
+            # We round these values to show, e.g., 7.15 instead of 7.1499999


this also limits the ability to zoom in to small wavelength ranges... I'm not sure that is worth it

maybe decimals can be based on the current range instead?

Changing this to depend on the difference between min and max rather than just max makes sense to me.

That also prevents decreasing v_max to be less than v_min, which is nice.

rosteen · 2023-12-28T21:31:37Z

Note that using glue-float-field to handle empty fields now requires glue-viz/glue-jupyter#417, since currently glue-float-field doesn't respect input step values.

kecnry · 2023-12-29T20:12:50Z

jdaviz/configs/default/plugins/plot_options/plot_options.py

-                bound_step = (viewer.state.x_max) / 100.
-                decimals = -int(np.log10(bound_step))+1
+                bound_step = (viewer.state.x_max - viewer.state.x_min) / 100.
+                decimals = -int(np.log10(bound_step)) + 1


otherwise this can result in an OverflowError if bound_step == 0 (not sure what makes sense for the fallback, and will need to apply this to the case for y as well).

Suggested change

decimals = -int(np.log10(bound_step)) + 1

decimals = -int(np.log10(bound_step)) + 1 if bound_step > 0 else 6

How about just wrapping bound_step in abs() instead? I think we want that anyway now that I think about it - or are you thinking about a reverse axis case where the up arrow should decrease the value?

We still need to handle the bound_step==0 case in which log10(0) will resolve to -inf and raise an error when casting to an integer.

Honestly we should probably prevent min=max in general somewhere else, but for now I added this check.

kecnry · 2024-01-02T15:11:16Z

jdaviz/configs/default/plugins/plot_options/plot_options.py

+        # This button is currently only exposed if only the spectrum viewer is selected
+        viewer = self.app.get_viewer('spectrum-viewer')


can we use the selected viewer reference directly instead of hardcoding for spectrum-viewer?

kecnry · 2024-01-02T15:12:07Z

jdaviz/configs/default/plugins/plot_options/plot_options.vue

@@ -39,6 +39,64 @@
      :hint="viewer_multiselect ? 'Select viewers to set options simultaneously' : 'Select the viewer to set options.'"
    />

+    <v-row v-if="viewer_selected === 'spectrum-viewer' || (viewer_selected[0] === 'spectrum-viewer' && viewer_selected.length === 1)">


this logic prevents this from being used in lcviz (and will conflict with any future support for renaming viewers). I think we should be able to rely on the info in the sync traitlets to determine whether this is applicable or not.

I had worked on generalizing this and then reverted to just the initial scope of spectrum-viewer - I remember it was partially working for LCViz but something was messed up, don't remember exactly why I abandoned it. I can try that again now that some other things are resolved.

does the logic have to be in python with an extra traitlet or can we just use the existing logic in the PlotOptionsSyncState by doing v-if="viewer_x_min_sync.in_subscribed_states"?

I don't really know the details of what subscribed states is doing, so: maybe? 😆

I'll give it a try.

Yeah, that doesn't stop the viewer bounds menu from showing up if you multiselect a spectrum viewer and an image viewer. So I think the way I have it now is necessary.

but I think that's ok and consistent with how everything else works in plot options. Do the viewer numbers appear in mutliselect (if not, that might avoid any confusion).

rosteen · 2024-01-02T21:01:39Z

@kecnry this now works in LCViz (as far as I tested anyway) and I also fixed a bug I noticed with the decimal rounding for large (>10) min/max separations.

cshanahan1

this looks good and works well for me!

the only minor suggestion i have is that floats like (1., 2.) aren't accepted and i think it is reasonable that they should be

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Forgot vue file

…andling

… numbers

rosteen · 2024-01-08T13:51:50Z

the only minor suggestion i have is that floats like (1., 2.) aren't accepted and i think it is reasonable that they should be

Hm, that seems to work ok for me:

kecnry

Let's get this in and we can iterate on any multiviewer support as part of #2649 on top of this. Thanks!

kecnry reviewed Dec 11, 2023

View reviewed changes

jdaviz/configs/default/plugins/plot_options/plot_options.vue Outdated Show resolved Hide resolved

rosteen added this to the 3.9 milestone Dec 12, 2023

rosteen added UI/UX😍 plugin Label for plugins common to multiple configurations labels Dec 12, 2023

rosteen force-pushed the specviz-viewer-bounds-ui branch 2 times, most recently from 2b717e0 to 0560f05 Compare December 19, 2023 19:46

rosteen marked this pull request as ready for review December 19, 2023 19:47

rosteen requested review from javerbukh, pllim, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners December 19, 2023 19:47

cshanahan1 reviewed Dec 27, 2023

View reviewed changes

kecnry reviewed Dec 28, 2023

View reviewed changes

kecnry reviewed Dec 29, 2023

View reviewed changes

kecnry reviewed Jan 2, 2024

View reviewed changes

cshanahan1 approved these changes Jan 5, 2024

View reviewed changes

kecnry mentioned this pull request Jan 5, 2024

image viewer zoom-level/center #2649

Merged

15 tasks

rosteen added 2 commits January 8, 2024 08:40

Working on section for bounds editing

6deaad2

More work

e6d6f56

rosteen and others added 19 commits January 8, 2024 08:40

Now works to update from UI for spectrum-viewer

6fcab6e

Fix codestyle

670efa3

Update jdaviz/configs/default/plugins/plot_options/plot_options.vue

c84f999

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Add changelog

07942e1

Working on handling multiselect

a7adb7e

More work on multi

438a3c1

Forgot vue file

Converting these to PlotOptionSynchedState objects with multiselect h…

12bf23b

…andling

Re-add unit suffix

25cb824

Dynamic viewer bounds based on max when viewer selected

b074a29

Codestyle

a1fffcd

Lost this in rebase

5fb8806

Use FloatHandleEmpty

abb7ccc

Resolve test failures

5968634

Also round displayed min/max values

5018d1b

Round on home zoom click as well

dc1a3ee

Fix empty field handling

de39bc1

Change to diff based steps

e95c620

Handle negative bounds diff and 0 diff

edc1e88

Handle non-spectrum non-image viewers, fix decimal rounding for large…

b31e268

… numbers

rosteen force-pushed the specviz-viewer-bounds-ui branch from 805aaa5 to b31e268 Compare January 8, 2024 13:42

kecnry approved these changes Jan 8, 2024

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose spectrum-viewer bounds in UI #2604

Expose spectrum-viewer bounds in UI #2604

rosteen commented Dec 11, 2023

codecov bot commented Dec 11, 2023 •

edited

Loading

kecnry commented Dec 11, 2023

camipacifici commented Dec 11, 2023

kecnry commented Dec 11, 2023

rosteen commented Dec 19, 2023

camipacifici commented Dec 19, 2023

cshanahan1 left a comment

kecnry Dec 28, 2023

kecnry Dec 28, 2023

rosteen Dec 28, 2023

rosteen Dec 28, 2023

rosteen commented Dec 28, 2023

kecnry Dec 29, 2023

rosteen Dec 29, 2023

kecnry Dec 29, 2023

rosteen Dec 29, 2023

kecnry Jan 2, 2024

kecnry Jan 2, 2024 •

edited

Loading

rosteen Jan 2, 2024

kecnry Jan 2, 2024

rosteen Jan 2, 2024

rosteen Jan 2, 2024

kecnry Jan 3, 2024

rosteen commented Jan 2, 2024

cshanahan1 left a comment

rosteen commented Jan 8, 2024

kecnry left a comment

	decimals = -int(np.log10(bound_step)) + 1
	decimals = -int(np.log10(bound_step)) + 1 if bound_step > 0 else 6

		# This button is currently only exposed if only the spectrum viewer is selected
		viewer = self.app.get_viewer('spectrum-viewer')

Expose spectrum-viewer bounds in UI #2604

Expose spectrum-viewer bounds in UI #2604

Conversation

rosteen commented Dec 11, 2023

codecov bot commented Dec 11, 2023 • edited Loading

Codecov Report

kecnry commented Dec 11, 2023

camipacifici commented Dec 11, 2023

kecnry commented Dec 11, 2023

rosteen commented Dec 19, 2023

camipacifici commented Dec 19, 2023

cshanahan1 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

rosteen commented Dec 28, 2023

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

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

rosteen commented Jan 2, 2024

cshanahan1 left a comment

Choose a reason for hiding this comment

rosteen commented Jan 8, 2024

kecnry left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2023 •

edited

Loading

kecnry Jan 2, 2024 •

edited

Loading