Adding colormap "curve" to Plot Options stretch histogram #2390

bmorris3 · 2023-08-22T15:49:46Z

Description

This PR is the first towards @PatrickOgle's feature request for Photoshop's "curves" tool. Curves shows a histogram of the pixel counts overlaid on a normalized representation of the colormap scaling. I've added a similar curve to the Plot Options histogram for image layers in Imviz or Cubeviz:

hist-stretch-func.mov

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 · 2023-08-22T16:55:34Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09% 🎉

Comparison is base (2e61f48) 90.52% compared to head (edf21f2) 90.62%.
Report is 2 commits behind head on main.

❗ Current head edf21f2 differs from pull request most recent head 27c0b59. Consider uploading reports for the commit 27c0b59 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2390      +/-   ##
==========================================
+ Coverage   90.52%   90.62%   +0.09%     
==========================================
  Files         159      159              
  Lines       18236    18217      -19     
==========================================
  Hits        16509    16509              
+ Misses       1727     1708      -19

Files Changed	Coverage Δ
...nfigs/default/plugins/plot_options/plot_options.py	`94.10% <100.00%> (+0.74%)`	⬆️

... and 12 files with indirect coverage changes

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

kecnry · 2023-08-22T17:23:23Z

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

        self._stretch_histogram_needs_update = False

+    @observe('stretch_vmin_value', 'stretch_vmax_value', 'layer_selected',
+             'stretch_function_value', 'stretch_hist_nbins',


I think this is redundant with the add_observe above (although we might want a comment here as well that we are listening to stretch_function_value.

Suggested change

'stretch_function_value', 'stretch_hist_nbins',

'stretch_hist_nbins',

Done, thanks.

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

kecnry · 2023-08-22T17:24:37Z

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

+        # Import here to prevent circular import (and not at the top of the method so the import
+        # check is avoided, whenever possible).
+        from jdaviz.configs.imviz.plugins.viewers import ImvizImageView
+        from jdaviz.configs.cubeviz.plugins.viewers import CubevizImageView
+
+        if (
+            not isinstance(self.viewer.selected_obj, (ImvizImageView, CubevizImageView)) or
+            not hasattr(self, 'stretch_histogram')
+        ):
+            # don't update histogram if selected viewer is not an image viewer,
+            # or the stretch histogram hasn't been initialized:
+            return


I don't love that this is duplicated logic, maybe we should put this in a re-usable method/property? Or even in ViewerSelect itself so we could do self.viewer.selected_is_image_viewer?

Moved 👍🏻

kecnry · 2023-08-22T17:26:25Z

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

+            mark_label = f'{mark_label_prefix}{layer.label}'
+            mark_exists = mark_label in self.stretch_histogram.marks


What is the benefit of tracking per-layer curves? Just to future-proof if we ever support showing the histogram for multiple layers? If we don't need that now, this can simplify greatly by creating the mark in the init with the rest of the other marks, always modify that existing entry, and not have to worry about re-ordering the marks.

That's the use case I have in mind. Curves for multiple layers might be helpful for RGB work (cc @PatrickOgle).

the whole histogram would need to be updated for that though... is it worth the overhead now if we're not sure how that will work yet?

If it's already implemented, what's the harm in keeping it?

I think we will want a more general solution that handles the histogram in the same way and there is no good way to actually test this implementation as-is and it adds quite a bit of extra code that we're not 100% sure if we'll ever use 🤷‍♂️

kecnry · 2023-08-22T17:27:13Z

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

+
+            # create the new/updated mark following the colormapping
+            # procedure in glue's CompositeArray:
+            layer_state = layer.state.as_dict()


does this provide any benefits over referring to layer.state.contrast etc below? My only thought would be that you ensure all state properties are accessed simultaneously 🤷‍♂️

No important differences. Switching to layer.state..

rosteen · 2023-08-23T17:15:03Z

I don't have a good intuitive sense for what I'm looking at here, but it seems useful for people who do. I think it might be better to have this be a toggle in the UI ("Show colormap curve" or something), which would add context for people who might not immediately know what this line on the histogram is.

pllim · 2023-08-23T17:19:26Z

The colormap curve hurts my head. It makes me think too much. Why can't we just have a colormap bar like everyone else?

kecnry · 2023-08-23T17:24:13Z

I think it might be better to have this be a toggle in the UI ("Show colormap curve" or something)

This could be a nice way to explain what it means, but it also would cost vertical space (you already have to scroll between the controls that modify the curve and the histogram itself, unless you pop it out), so that concerns me a little. I suspect people who want it would know what it is, and people who don't would just ignore it? 🤷‍♂️ This might be a good thing for user-testing and feedback though.

Why can't we just have a colormap bar like everyone else?

I agree for just trying to go from color -> value a colorbar is more intuitive and common, but the eventual goal/request here is to be able to also create and edit custom splines for RGB image creation to manually tune the balances for each layer/filter.

rosteen · 2023-08-25T17:24:39Z

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

+            curve_y = stretch(curve_y, out=curve_y, clip=False)
+            curve_y = layer_cmap(curve_y)[:, 0]
+
+            if not mark_exists:


Would it make more sense to have this after the following for loop, so we're not immediately checking a mark we just created that already has the right x and y?

Good idea, done!

bmorris3 · 2023-08-28T14:26:41Z

Added a toggle option for the stretch curve in the plot options histogram (thanks for the suggestion, @rosteen!):

hist-stretch-func-toggle.mov

rosteen

LGTM now, I like the toggle. Tested in the standalone and notebook, didn't manage to break it. I don't have strong opinions on the last ongoing implementation discussion so I'm approving.

kecnry · 2023-08-28T14:59:25Z

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

@@ -381,6 +393,13 @@ def state_attr_for_line_visible(state):
        self.stretch_vmax = PlotOptionsSyncState(self, self.viewer, self.layer, 'v_max',
                                                 'stretch_vmax_value', 'stretch_vmax_sync',
                                                 state_filter=is_image)
+
+        self.stretch_curve_visible = PlotOptionsSyncState(self, self.viewer, self.layer,


do we want to add this to the user API (and then API docs)?

bmorris3 · 2023-08-28T15:33:17Z

Dev failure is unrelated, and being addressed upstream here: astropy/astropy#15233

kecnry

I created a follow-up ticket to either finish multi-layer support (for the histogram and vertical marks as well) or to remove the unused logic here. 🐱.

Can you also please just add an entry for stretch_curve_visible in the class-level docstring before merge?

add stretch_curve_visible to plot options api, docs

bmorris3 added this to the 3.7 milestone Aug 22, 2023

bmorris3 added imviz cubeviz UI/UX😍 plugin Label for plugins common to multiple configurations labels Aug 22, 2023

bmorris3 marked this pull request as ready for review August 22, 2023 17:19

bmorris3 requested review from rosteen, javerbukh, pllim, kecnry, haticekaratay and cshanahan1 as code owners August 22, 2023 17:19

bmorris3 requested a review from PatrickOgle August 22, 2023 17:20

kecnry reviewed Aug 22, 2023

View reviewed changes

rosteen reviewed Aug 25, 2023

View reviewed changes

bmorris3 added 4 commits August 28, 2023 10:26

adding photoshop-style 'curves' feature to plot options hist

ddd9210

sync histogram stretch function

70af2f0

observe bias and contrast too

de15416

skip stretch histogram for irrelevant viewers

a908e1c

bmorris3 force-pushed the hist-stretch-func branch from edf21f2 to c65035a Compare August 28, 2023 14:26

bmorris3 force-pushed the hist-stretch-func branch from c65035a to 47a0a55 Compare August 28, 2023 14:28

rosteen approved these changes Aug 28, 2023

View reviewed changes

kecnry reviewed Aug 28, 2023

View reviewed changes

kecnry approved these changes Aug 28, 2023

View reviewed changes

address histogram stretch function review comments

27c0b59

add stretch_curve_visible to plot options api, docs

bmorris3 force-pushed the hist-stretch-func branch from 99b4329 to 27c0b59 Compare August 28, 2023 16:05

pllim merged commit c2cfd94 into spacetelescope:main Aug 28, 2023

kecnry mentioned this pull request Aug 30, 2023

improve stretch histogram performance (downsampling and decorator) #2408

Merged

9 tasks

bmorris3 mentioned this pull request Oct 26, 2023

Bugfix: correction to Plot Options stretch function curve #2537

Merged

9 tasks

kecnry mentioned this pull request Oct 27, 2023

consolidate stretch histogram code #2538

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding colormap "curve" to Plot Options stretch histogram #2390

Adding colormap "curve" to Plot Options stretch histogram #2390

bmorris3 commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

kecnry Aug 22, 2023

bmorris3 Aug 28, 2023 •

edited

Loading

kecnry Aug 22, 2023

bmorris3 Aug 28, 2023

kecnry Aug 22, 2023

bmorris3 Aug 23, 2023

kecnry Aug 23, 2023

bmorris3 Aug 28, 2023

kecnry Aug 28, 2023

kecnry Aug 22, 2023

bmorris3 Aug 28, 2023

rosteen commented Aug 23, 2023

pllim commented Aug 23, 2023

kecnry commented Aug 23, 2023

rosteen Aug 25, 2023

bmorris3 Aug 28, 2023

bmorris3 commented Aug 28, 2023

rosteen left a comment

kecnry Aug 28, 2023

bmorris3 Aug 28, 2023

bmorris3 commented Aug 28, 2023

kecnry left a comment

	'stretch_function_value', 'stretch_hist_nbins',
	'stretch_hist_nbins',

		mark_label = f'{mark_label_prefix}{layer.label}'
		mark_exists = mark_label in self.stretch_histogram.marks

Adding colormap "curve" to Plot Options stretch histogram #2390

Adding colormap "curve" to Plot Options stretch histogram #2390

Conversation

bmorris3 commented Aug 22, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

bmorris3 Aug 28, 2023 • 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

Choose a reason for hiding this comment

rosteen commented Aug 23, 2023

pllim commented Aug 23, 2023

kecnry commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmorris3 commented Aug 28, 2023

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

bmorris3 commented Aug 28, 2023

kecnry left a comment

Choose a reason for hiding this comment

bmorris3 commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

bmorris3 Aug 28, 2023 •

edited

Loading