Colorbar on histogram #2517

pllim · 2023-10-17T22:14:49Z

Description

This pull request is a different approach to encode colorbar information on the Plot Options histogram. Previous attempt to use pure bqplot (#2514) failed. This one lifts code from glue-core to compute the colors using matplotlib and then throw them on the bqplot histogram directly.

Once we decided on the look we want (see below), I will transfer the code from the concept notebook added in this PR to the actual app code. Right now, this only works if you run the concept notebook. (I went with hacked Scatter plot on top of histogram.)

Known issues due to it listening to too many things at once, I think. Not sure how to make it cleaner:

Wonky after toggling multiselect on/off.
Wonky after selecting/deselecting subset.
Wonky after zooming in/out too fast while having it follow zoom.
Possibly unrelated bug of it crashing when destroying viewer. Plot Option breaks when viewer deleted while selected #2521

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

camipacifici · 2023-10-18T03:30:11Z

Although this looks pretty, I am afraid it might not work in all cases. Histograms can be very much non-uniform and part of the color coding will be hidden. Here an extreme example.

I will give it a try anyway tomorrow, thank you!

pllim · 2023-10-18T13:39:45Z

Histograms can be very much non-uniform and part of the color coding will be hidden.

But you have pan/zoom now. 😉

kecnry · 2023-10-18T13:50:57Z

Another thing to consider (if coloring lines is ever supported upstream): we could color the stretch curve itself (and perhaps have it always enabled)... but I don't think that switching that to scatter with the plan to support custom splines would be worth it.

pllim · 2023-10-18T17:19:46Z

I cannot figure out bqplot.Image but I managed to hack around bqplot.Scatter. Does this look better?

camipacifici · 2023-10-18T18:01:35Z

Promising!!
Can I test on the same concept notebook?

codecov · 2023-10-19T00:26:09Z

Codecov Report

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

Files	Coverage Δ
...lt/plugins/plot_options/tests/test_plot_options.py	`100.00% <100.00%> (ø)`
jdaviz/core/template_mixin.py	`91.42% <100.00%> (+0.03%)`	⬆️
...nfigs/default/plugins/plot_options/plot_options.py	`91.33% <78.94%> (-2.07%)`	⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

pllim · 2023-10-19T02:09:34Z

@camipacifici , the colorbar now should be available in Plot Options. The concept notebook is still useful if you want to quickly run through different Plot Options settings via API and see how it affects the colorbar, but not compulsory.

kecnry

I'm not convinced scatter is the solution we want, but at least it allows us to test out the rest of the logic and hopefully we can swap out whatever is used for plotting (either before merge or as a follow-up).

kecnry · 2023-10-19T12:18:22Z

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

+    @observe('is_active', 'image_color_mode_value', 'image_color_value', 'image_colormap_value',
+             'image_contrast_value', 'image_bias_value',
+             'stretch_function_value', 'stretch_vmin_value', 'stretch_vmax_value',
+             'stretch_hist_nbins', 'stretch_hist_zoom_limits')


I don't think this should be affected by stretch_hist_nbins or stretch_hist_zoom_limits, right?

You tell me. Colorbar needs to track changes in the histogram X-axis too. So unless these nbins also trigger one of the other things it is already listening, I need it. Not sure what stretch_hist_zoom_limits is doing.

Since x is currently sampled on the bins (see conversation below), we do need to keep stretch_hist_nbins so that the resolution of the colorbar is controlled by the same input. But I don't see any need for stretch_hist_zoom_limits (which just changes which data is included in the histogram, but shouldn't affect the bins themselves (unless perhaps that would break if some bins on the extremes are unused - worth testing first!)

Actually I think we still need it because the display histogram X-axis values change when you toggle follow zoom, so we also need to update the colorbar.

kecnry · 2023-10-19T12:19:22Z

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

+        with self.stretch_histogram.hold_sync():
+            color_mode = self.image_color_mode_value
+            interval = ManualInterval(self.stretch_vmin.value, self.stretch_vmax.value)
+            contrast_bias = ContrastBiasStretch(self.image_contrast_value, self.image_bias_value)
+            stretch = stretches.members[self.stretch_function_value]


with a lot of this logic being shared with the drawing of the stretch function, should those be in the same method? We could even consider having the same switch toggle both of them (and perhaps be on by default).

They are a bit different. You listen to layer.state but I cannot because the state updates lags behind so I need to grab from plugin settings.

Ok, so can the stretch function read from plugin settings as well to be consistent?

Isn't self.stretch_function_value from the plugin already?

Yes, sorry, I meant can't drawing the stretch function be done the same way as you are doing here so that the code can be consolidated?

Maybe but I don't want to complicate it more in this PR. We're already behind schedule.

Consolidating will make maintaining and building on top of this significantly easier though

I am still not sure if consolidation won't introduce new bugs. _update_stretch_curve loops through for layer in self.layer.selected_obj for reason I do not understand, while _update_stretch_histogram_colorbar does not because I disabled it in multi-select mode. So maybe when you are looping through layers, it makes sense to access layer.state instead of plugin values. But I cannot do that in the colorbar because for reason I do not understand again, layer.state lags behind in the colorbar (i.e., if I change from Gray to Viridis, colorbar only changes to Viridis after I change it back to Gray again).

decided to defer code consolidation and removing any multi-layer support to a follow-up effort.

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

pllim · 2023-10-24T22:31:07Z

Rebased on top of @haticekaratay 's work.

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

kecnry

Thanks! We'll consolidate the code with other recent PRs in a follow-up PR (hopefully soon 🤞 )

pllim · 2023-10-25T15:06:46Z

devdeps failure is unrelated

RTD failure is also unrelated but I should probably patch it sooner than later as a separate PR (#2533).

and add concept notebook with possible colorbar implementations [ci skip] [rtd skip]

because Kyle probably added this because I have no idea what it is doing but I see it in the old diff. Co-authored-by: Kyle Conroy <[email protected]>

and some cleanup in that same test module

pllim · 2023-10-25T15:44:01Z

Rebased again to resolve change log conflict.

javerbukh

Works as expected in all of my testing, nice work! Do we still want to keep the concept notebook?

pllim · 2023-10-25T18:08:46Z

concept notebook?

I found it useful to access the Plot Options programmatically with it and see the popped out colorbar changes in real time. But it is not necessary for this feature to work, so if you prefer to not have it, I can delete it from this PR. Please let me know.

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

pllim added this to the 3.8 milestone Oct 17, 2023

github-actions bot added the documentation Explanation of code and concepts label Oct 17, 2023

This comment was marked as resolved.

Sign in to view

pllim force-pushed the colors-of-the-hist-2 branch from 9fc2c03 to 1f8ba78 Compare October 19, 2023 00:01

pllim marked this pull request as ready for review October 19, 2023 02:34

pllim requested review from rosteen, javerbukh, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners October 19, 2023 02:34

kecnry reviewed Oct 19, 2023

View reviewed changes

pllim force-pushed the colors-of-the-hist-2 branch 3 times, most recently from 299fe92 to b8eb5d3 Compare October 24, 2023 22:04

kecnry reviewed Oct 25, 2023

View reviewed changes

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

kecnry approved these changes Oct 25, 2023

View reviewed changes

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

hold sync when update hist

e1b7703

and add concept notebook with possible colorbar implementations [ci skip] [rtd skip]

pllim and others added 7 commits October 25, 2023 11:42

Move POC code to app

e0870ac

Add change log, doc, and smoke test

674311f

keep vertical position of colorbar fixed

9399864

Add comment about marks[0] usage

a3a7a81

for loop should not return on subset

d1296b4

Add back rebase botch

b0ca497

because Kyle probably added this because I have no idea what it is doing but I see it in the old diff. Co-authored-by: Kyle Conroy <[email protected]>

Improve colorbar test

2753608

and some cleanup in that same test module

pllim force-pushed the colors-of-the-hist-2 branch from 3c2f401 to 2753608 Compare October 25, 2023 15:43

javerbukh approved these changes Oct 25, 2023

View reviewed changes

pllim enabled auto-merge (squash) October 25, 2023 18:39

pllim disabled auto-merge October 25, 2023 18:39

pllim merged commit 0e8981c into spacetelescope:main Oct 25, 2023
11 of 15 checks passed

pllim deleted the colors-of-the-hist-2 branch October 25, 2023 18:39

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

Colorbar on histogram #2517

Colorbar on histogram #2517

pllim commented Oct 17, 2023 •

edited

Loading

camipacifici commented Oct 18, 2023

pllim commented Oct 18, 2023

kecnry commented Oct 18, 2023

pllim commented Oct 18, 2023

camipacifici commented Oct 18, 2023

This comment was marked as resolved.

codecov bot commented Oct 19, 2023 •

edited

Loading

pllim commented Oct 19, 2023

kecnry left a comment

kecnry Oct 19, 2023

pllim Oct 19, 2023

kecnry Oct 19, 2023

pllim Oct 19, 2023

kecnry Oct 19, 2023

pllim Oct 19, 2023

kecnry Oct 19, 2023

pllim Oct 19, 2023

kecnry Oct 19, 2023

pllim Oct 19, 2023

kecnry Oct 19, 2023

pllim Oct 19, 2023

kecnry Oct 25, 2023

pllim commented Oct 24, 2023

kecnry left a comment

pllim commented Oct 25, 2023 •

edited

Loading

pllim commented Oct 25, 2023

javerbukh left a comment

pllim commented Oct 25, 2023

Colorbar on histogram #2517

Colorbar on histogram #2517

Conversation

pllim commented Oct 17, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

camipacifici commented Oct 18, 2023

pllim commented Oct 18, 2023

kecnry commented Oct 18, 2023

pllim commented Oct 18, 2023

camipacifici commented Oct 18, 2023

This comment was marked as resolved.

codecov bot commented Oct 19, 2023 • edited Loading

Codecov Report

pllim commented Oct 19, 2023

kecnry 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

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 Oct 24, 2023

kecnry left a comment

Choose a reason for hiding this comment

pllim commented Oct 25, 2023 • edited Loading

pllim commented Oct 25, 2023

javerbukh left a comment

Choose a reason for hiding this comment

pllim commented Oct 25, 2023

pllim commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 19, 2023 •

edited

Loading

pllim commented Oct 25, 2023 •

edited

Loading