consolidate stretch histogram code #2538

kecnry · 2023-10-27T16:21:04Z

Description

This pull request:

consolidates code introduced in Introduce a new SplineStretch class #2525, Colorbar on histogram #2517, Add stretch bounds tool to plot option histogram viewer #2513, and Adding colormap "curve" to Plot Options stretch histogram #2390 to be in a single loop and avoid code duplication.
consolidates the UI itself by moving redundant input options (vmin, vmax, etc - now that these are editable interactively in the plugin plot) into an expandable menu.
removes infrastructure code added in Adding colormap "curve" to Plot Options stretch histogram #2390 to support multiple layers in the stretch histogram as that is no longer planned, and if we do ever want to introduce it, would require more general code and design with these new capabilities built on top.
scales the curve to 0.9 to avoid overlapping the colorbar
shows the stretch curve by default

No changelog entry needed as none of these changes have seen a release yet.

Note: this is expected to have a merge conflict with #2537, but should be fairly simple to address.

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

* and remove multilayer support

pllim · 2023-10-27T16:25:46Z

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

@@ -726,23 +739,24 @@ def _update_stretch_histogram(self, msg={}):

 # update the n_bins since this may be a new layer
 self._histogram_nbins_changed()
+ # update the curve/colorbar


I don't think colorbar and stretch curve listen to exactly the same events? For example, colorbar needs to match the bins on histogram but not necessarily the curve?

I know, we can skip sections based on the event if performance is an issue/concern, but I think this is cleaner than having duplicated code and multiple callbacks (in unpredictable order) for the ones that are shared.

If there are blocks you think we should skip under certain scenarios, please suggest any changes (msg.get('name') should give the name of the traitlet causing the method to be triggered)!

codecov · 2023-10-27T16:40:22Z

Codecov Report

Attention: 6 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.64% <100.00%> (+0.21%)`	⬆️
...nfigs/default/plugins/plot_options/plot_options.py	`92.00% <86.66%> (+0.66%)`	⬆️

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

pllim · 2023-10-27T17:21:28Z

I saw things but they are not related:

At some point, the colorbar scatter plot got messed up as if I reduced the number of bins but I didn't; however I am unable to reproduce this as I was doing a bunch of things and I don't remember what or the order.

pllim · 2023-10-27T17:24:28Z

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

 'stretch_function_value', 'stretch_vmin_value', 'stretch_vmax_value',
- 'stretch_hist_nbins', 'stretch_hist_zoom_limits')


Are you sure we don't need 'stretch_hist_zoom_limits'?

'is_active', 'layer_selected', 'viewer_selected', 'stretch_hist_zoom_limits' all trigger _update_stretch_histogram, which in turn will call this after updating the histogram bins themselves.

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

pllim · 2023-10-27T17:29:34Z

jdaviz/core/template_mixin.py

-# but may be added in the future. If it is not in the registry, we'll add it now. If/once the
-# min-pin of glue-jupyter includes this in the registry, we can safely remove this block.
+# but may be added in the future. If it is not in the registry, we'll add it now.
+# Once glue-jupyter with https://github.com/glue-viz/glue-jupyter/pull/402 is pinned,


If you know the version to pin to, please also document it here. Or open issue so we don't forget. Thanks!

I do not know the version yet (likely the next minor release, which would be 0.20.0, but can't be sure... I guess anything after 0.19.0 since its now merged) and forgetting won't hurt - we can remove this at any point after its sure to be available upstream.

pllim

Overall, this is good clean-up.

I also saw this thing where the vmin/vmax lines won't show when I update their values in Spline mode. But again, I cannot reproduce it. 🤷

kecnry · 2023-10-27T18:46:35Z

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

+ # NOTE: Index 0 in marks is assumed to be the bin centers.
+ x = self.stretch_histogram.figure.marks[0].x
+ y = np.ones_like(x)
+
+ # Copied from the __call__ internals of glue/viewers/image/composite_array.py
+ data = interval(x)
+ data = contrast_bias(data, out=data)
+ data = stretch(data, out=data)


I wonder if we can consolidate this with the similar logic for the stretch curve... the stretch curve uses a linspace between vmin/vmax whereas this uses the histogram bins and needs to extend beyond vmin/vmax. (@pllim @bmorris3 - what if we did a linspace between the lower and upper bin and use the same sampling for both?

colorbar needs to render beyond the vertical lines, it needs to span the whole visible histogram. I think the lines can move around, but not the histogram bars. Or am I not understanding how all these stuff is supposed to work together?

ok, let's merge for now since @haticekaratay and @javerbukh both need to build on top of this and we can discuss and do this as follow-up if we think it can be simplified. Thanks!

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

haticekaratay

Looks good to me! Thanks for your work!

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

kecnry added 6 commits October 25, 2023 14:41

consolidate stretch curve and colorbar code

d63bf40

* and remove multilayer support

hide stretch histogram in multiselect, advanced options to accordion

209a134

update comment when its safe to remove temp registry block

7a06273

show stretch curve by default

ab7f034

ensure colorbar updated at start

78a20ca

update tests

a3d0cae

kecnry added the no-changelog-entry-needed changelog bot directive label Oct 27, 2023

kecnry added this to the 3.8 milestone Oct 27, 2023

pllim reviewed Oct 27, 2023

View reviewed changes

kecnry marked this pull request as ready for review October 27, 2023 16:34

kecnry requested review from rosteen, javerbukh, bmorris3, haticekaratay and cshanahan1 as code owners October 27, 2023 16:34

pllim reviewed Oct 27, 2023

View reviewed changes

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

remove out-of-date comment

8957ea4

pllim reviewed Oct 27, 2023

View reviewed changes

pllim approved these changes Oct 27, 2023

View reviewed changes

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

kecnry commented Oct 27, 2023

View reviewed changes

haticekaratay reviewed Oct 27, 2023

View reviewed changes

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

kecnry mentioned this pull request Oct 27, 2023

UI: improve margin in expansion panels in plugins #2540

Merged

9 tasks

haticekaratay approved these changes Oct 27, 2023

View reviewed changes

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

kecnry merged commit 7855702 into spacetelescope:main Oct 27, 2023
18 of 19 checks passed

kecnry deleted the stretch-hist-consolidate branch October 27, 2023 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consolidate stretch histogram code #2538

consolidate stretch histogram code #2538

kecnry commented Oct 27, 2023 •

edited

Loading

pllim Oct 27, 2023

kecnry Oct 27, 2023 •

edited

Loading

kecnry Oct 27, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading

pllim commented Oct 27, 2023

pllim Oct 27, 2023

kecnry Oct 27, 2023 •

edited

Loading

pllim Oct 27, 2023

kecnry Oct 27, 2023

pllim left a comment

kecnry Oct 27, 2023

pllim Oct 27, 2023

kecnry Oct 27, 2023

haticekaratay left a comment

		'stretch_function_value', 'stretch_vmin_value', 'stretch_vmax_value',
		'stretch_hist_nbins', 'stretch_hist_zoom_limits')

consolidate stretch histogram code #2538

consolidate stretch histogram code #2538

Conversation

kecnry commented Oct 27, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim Oct 27, 2023

Choose a reason for hiding this comment

kecnry Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

kecnry Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 27, 2023 • edited Loading

Codecov Report

pllim commented Oct 27, 2023

pllim Oct 27, 2023

Choose a reason for hiding this comment

kecnry Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

pllim Oct 27, 2023

Choose a reason for hiding this comment

kecnry Oct 27, 2023

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

kecnry Oct 27, 2023

Choose a reason for hiding this comment

pllim Oct 27, 2023

Choose a reason for hiding this comment

kecnry Oct 27, 2023

Choose a reason for hiding this comment

haticekaratay left a comment

Choose a reason for hiding this comment

kecnry commented Oct 27, 2023 •

edited

Loading

kecnry Oct 27, 2023 •

edited

Loading

kecnry Oct 27, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading

kecnry Oct 27, 2023 •

edited

Loading