Add stretch bounds tool to plot option histogram viewer #2513

rosteen · 2023-10-16T14:26:48Z

Opening as draft since I'll need a test and changelog entry.

codecov · 2023-10-16T14:47:33Z

Codecov Report

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

Files	Coverage Δ
...nfigs/default/plugins/plot_options/plot_options.py	`86.64% <100.00%> (+0.28%)`	⬆️
jdaviz/core/template_mixin.py	`91.25% <100.00%> (+0.01%)`	⬆️
jdaviz/core/tools.py	`86.42% <89.65%> (+0.34%)`	⬆️

... and 6 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

jdaviz/core/tools.py

Remove debugging prints Add test

kecnry · 2023-10-16T15:48:44Z

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

+ # Add the stretch bounds tool to the default Plot viewer.
+ self.stretch_histogram.tools_nested.append(["jdaviz:stretch_bounds"])
+ self.stretch_histogram.toolbar = NestedJupyterToolbar(self.stretch_histogram.viewer,
+ self.stretch_histogram.tools_nested,
+ [])


we might eventually want to generalize this as a method (or have the init take options for the tools), but this is probably ok for now if this is the only case we're modifying?

jdaviz/core/tools.py

rosteen · 2023-10-16T15:56:07Z

FYI Jenn said she probably won't be able to get to making an icon for this until next week.

rosteen · 2023-10-19T15:57:06Z

I changed this to update the closest bound to where the user clicks, and to enable clicking+dragging. Way more performant than I was worried it would be, video below:

Screen.Recording.2023-10-19.at.11.54.31.AM.mov

pllim · 2023-10-19T16:03:06Z

Curious... what happens when someone puts one line on top of another, how do you decide which line they are clicking on?

rosteen · 2023-10-19T16:09:02Z

Curious... what happens when someone puts one line on top of another, how do you decide which line they are clicking on?

It seems to Just Work (tm) actually, I don't think it will let you drag the VMax below the VMin, it switches between them as you drag past.

jdaviz/core/tools.py

jdaviz/core/tests/test_tools.py

kecnry

This really works very nicely once we get the throttling tuned right for large images. Do we want to change the color between inactive/active in this PR or consider that follow-up?

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

Co-authored-by: Kyle Conroy <[email protected]>

rosteen · 2023-10-19T17:52:43Z

This really works very nicely once we get the throttling tuned right for large images. Do we want to change the color between inactive/active in this PR or consider that follow-up?

I'll see if I can do the color quickly this afternoon.

rosteen · 2023-10-19T19:34:16Z

@kecnry Colors change now, not sure if this is the best way to do it but these lines weren't a subclass of any of our marks, so I just did it this way 🤷 .

kecnry · 2023-10-19T19:53:34Z

This also changes the color of the stretch function itself, which isn't ideal. We probably want that to always be blue now since that will only be "active"/"clickable" with another tool and only for splines (in the future). We might eventually want to generalize the slice mark to also get the "out-of-bounds" logic, but for now just a custom class or even just a private attribute to check (line.is_vmin_vmax=True and then `hasattr(line, 'is_vmin_vmax' or something like that) so that we can filter on just these two should do the trick.

rosteen · 2023-10-19T19:58:44Z

This also changes the color of the stretch function itself

I was thinking that, although you aren't directly changing that line, it does update when you change vmin and vmax, that it might make sense to have it change color as well. Basically "things that will be affected by using this tool change color." I'm open to changing it though.

Update jdaviz/configs/default/plugins/plot_options/plot_options.py Co-authored-by: Kyle Conroy <[email protected]> Codestyle

Add missing colon Codestyle

rosteen · 2023-10-19T21:53:55Z

Alright, I updated so that the stretch curve color doesn't get updated.

kecnry

There was no where to suggest it, so I just pushed a commit that changes the curve to the same "inactive" blue (always). Once that supports dragging as well, we can do the same color toggling as here.

This works great and is a super elegant solution! Curious to see if we need to fine-tune the throttling when using on super large images, but we can iterate on that easily going forward.

jdaviz/core/tests/test_tools.py

cshanahan1

The only small issue I ran into was that there is some funky behavior when you click a min/max line and drag it around, when it surpasses the previous maximum and becomes the new maximum. The second line will move even when not being dragged:

Screen.Recording.2023-10-20.at.12.55.26.PM.mov

EDIT: We had a discussion offline about changing the logic of this from 'inactive bar becomes active' to 'active bar becomes new min/max' which would improve the jumpiness when dragging the bar around, but that doesn't need to block this PR.

cshanahan1 · 2023-10-20T17:07:32Z

When you click the home button the bars disappear. Also you can see in this video, do vmin and vmax start overlapping and you have to 'shake' the second bound loose to have access to both bars?:

Screen.Recording.2023-10-20.at.1.05.24.PM.mov

kecnry · 2023-10-20T17:10:04Z

When you click the home button the bars disappear.

Home button is a known bug, see #2506

cshanahan1

Approving, but to summarize some offline discussion pertaining to my review:

My comment about the jumpiness of the bars can be fixed in a follow up but doesn't need to block this PR. It would entail changing the logic when vmin>vmax from the active bar switching, to the definition of the active bar switching (vmin becomes vmax, rather than the selection changing)
The home button problem is an existing issue
The overlapping bars issue i mentioned is related to Non-repeatable vmin vmax[BUG] #2526

Add stretch bounds tool to plot option histogram viewer

8ebe1dc

rosteen added feature Feature request plugin Label for plugins common to multiple configurations labels Oct 16, 2023

rosteen added this to the 3.8 milestone Oct 16, 2023

bmorris3 reviewed Oct 16, 2023

View reviewed changes

jdaviz/core/tools.py Outdated Show resolved Hide resolved

Add changelog, test

59431ab

Remove debugging prints Add test

kecnry reviewed Oct 16, 2023

View reviewed changes

Change to click+drag for stretch tool

7f562cf

Fix codestyle

205bde7

kecnry reviewed Oct 19, 2023

View reviewed changes

jdaviz/core/tools.py Outdated Show resolved Hide resolved

jdaviz/core/tests/test_tools.py Outdated Show resolved Hide resolved

kecnry reviewed Oct 19, 2023

View reviewed changes

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

Decrease throttle time and properly update last time

3863338

rosteen marked this pull request as ready for review October 19, 2023 17:48

rosteen requested review from javerbukh, pllim, haticekaratay and cshanahan1 as code owners October 19, 2023 17:48

Update jdaviz/core/tests/test_tools.py

cadd0ec

Co-authored-by: Kyle Conroy <[email protected]>

rosteen force-pushed the interactive-stretch-limits branch from f4dfd1b to cadd0ec Compare October 19, 2023 17:51

rosteen force-pushed the interactive-stretch-limits branch from f81c6ad to bbd0603 Compare October 19, 2023 19:33

Change color based on tool active state

f2fc309

Update jdaviz/configs/default/plugins/plot_options/plot_options.py Co-authored-by: Kyle Conroy <[email protected]> Codestyle

rosteen force-pushed the interactive-stretch-limits branch from bbd0603 to f2fc309 Compare October 19, 2023 19:59

Don't change color of stretch function curve

1c4cae0

Add missing colon Codestyle

kecnry approved these changes Oct 20, 2023

View reviewed changes

kecnry added the Ready for final review label Oct 20, 2023

kecnry reviewed Oct 20, 2023

View reviewed changes

jdaviz/core/tests/test_tools.py Outdated Show resolved Hide resolved

rosteen and others added 2 commits October 20, 2023 10:47

Add short sleep to test to avoid throttle

da425e0

stretch function curve as inactive blue

5eff644

rosteen force-pushed the interactive-stretch-limits branch from 04ce780 to 5eff644 Compare October 20, 2023 14:48

cshanahan1 reviewed Oct 20, 2023

View reviewed changes

cshanahan1 mentioned this pull request Oct 20, 2023

Non-repeatable vmin vmax[BUG] #2526

Open

cshanahan1 approved these changes Oct 20, 2023

View reviewed changes

kecnry merged commit d9cacaf into spacetelescope:main Oct 20, 2023
15 checks passed

kecnry mentioned this pull request Oct 27, 2023

consolidate stretch histogram code #2538

Merged

9 tasks

kecnry mentioned this pull request Nov 8, 2023

only include stretch tool in stretch histogram #2556

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stretch bounds tool to plot option histogram viewer #2513

Add stretch bounds tool to plot option histogram viewer #2513

rosteen commented Oct 16, 2023

codecov bot commented Oct 16, 2023 •

edited

Loading

kecnry Oct 16, 2023

rosteen commented Oct 16, 2023

rosteen commented Oct 19, 2023

pllim commented Oct 19, 2023

rosteen commented Oct 19, 2023

kecnry left a comment

rosteen commented Oct 19, 2023

rosteen commented Oct 19, 2023

kecnry commented Oct 19, 2023 •

edited

Loading

rosteen commented Oct 19, 2023

rosteen commented Oct 19, 2023

kecnry left a comment

cshanahan1 left a comment •

edited

Loading

cshanahan1 commented Oct 20, 2023 •

edited

Loading

kecnry commented Oct 20, 2023

cshanahan1 left a comment

Add stretch bounds tool to plot option histogram viewer #2513

Add stretch bounds tool to plot option histogram viewer #2513

Conversation

rosteen commented Oct 16, 2023

codecov bot commented Oct 16, 2023 • edited Loading

Codecov Report

kecnry Oct 16, 2023

Choose a reason for hiding this comment

rosteen commented Oct 16, 2023

rosteen commented Oct 19, 2023

pllim commented Oct 19, 2023

rosteen commented Oct 19, 2023

kecnry left a comment

Choose a reason for hiding this comment

rosteen commented Oct 19, 2023

rosteen commented Oct 19, 2023

kecnry commented Oct 19, 2023 • edited Loading

rosteen commented Oct 19, 2023

rosteen commented Oct 19, 2023

kecnry left a comment

Choose a reason for hiding this comment

cshanahan1 left a comment • edited Loading

Choose a reason for hiding this comment

cshanahan1 commented Oct 20, 2023 • edited Loading

kecnry commented Oct 20, 2023

cshanahan1 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2023 •

edited

Loading

kecnry commented Oct 19, 2023 •

edited

Loading

cshanahan1 left a comment •

edited

Loading

cshanahan1 commented Oct 20, 2023 •

edited

Loading