Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Colorbar on histogram #2517

Merged
merged 8 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ New Features

- User can now remove data from the app completely after removing it from viewers. [#2409, #2531]

- Colorbar now shown on top of the histogram in Plot Options for image viewers. [#2517]

Cubeviz
^^^^^^^

Expand All @@ -25,7 +27,6 @@ Imviz

- Aperture photometry (previously "Imviz Simple Aperture Photometry") now supports batch mode. [#2465]


- Expose sky regions in get_subsets. If 'include_sky_region' is True, a sky Region will be returned (in addition to a pixel Region) for spatial subsets with parent data that was a WCS. [#2496]

Mosviz
Expand Down
2 changes: 1 addition & 1 deletion docs/imviz/displayimages.rst
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ can be accessed with ``plot_options.stretch_function.choices``:

A histogram is displayed showing the distribution of pixel values, with
vertical lines representing the ``stretch_vmin`` and ``stretch_vmax``
values. A stretch "curve" can be plotted under the histogram to represent
values, and a colorbar on top. A stretch "curve" can be plotted under the histogram to represent
how pixel values are mapped to the colorbar. This feature can be toggled
on from the API with:

Expand Down
96 changes: 87 additions & 9 deletions jdaviz/configs/default/plugins/plot_options/plot_options.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
import os
import matplotlib
import numpy as np

from astropy.visualization import (
ManualInterval, ContrastBiasStretch, PercentileInterval
)
from echo import delay_callback
from traitlets import Any, Dict, Float, Bool, Int, List, Unicode, observe

from glue.core.subset_group import GroupedSubset
from glue.config import stretches
from glue.config import colormaps, stretches
from glue.viewers.scatter.state import ScatterViewerState
from glue.viewers.profile.state import ProfileViewerState, ProfileLayerState
from glue.viewers.image.state import ImageSubsetLayerState
from glue.viewers.scatter.state import ScatterLayerState as BqplotScatterLayerState
from glue.viewers.image.composite_array import COLOR_CONVERTER
from glue_jupyter.bqplot.image.state import BqplotImageLayerState
from glue_jupyter.common.toolbar_vuetify import read_icon

Expand Down Expand Up @@ -473,12 +476,14 @@
self.stretch_histogram._add_data('histogram', x=[0, 1])

self.stretch_histogram.add_line('vmin', x=[0, 0], y=[0, 1], ynorm=True, color='#c75d2c')
self.stretch_histogram.add_line('vmax', x=[0, 0], y=[0, 1], ynorm=True, color='#c75d2c')
self.stretch_histogram.figure.axes[0].label = 'pixel value'
self.stretch_histogram.figure.axes[0].num_ticks = 3
self.stretch_histogram.figure.axes[0].tick_format = '0.1e'
self.stretch_histogram.figure.axes[1].label = 'density'
self.stretch_histogram.figure.axes[1].num_ticks = 2
self.stretch_histogram.add_line('vmax', x=[0, 0], y=[0, 1], ynorm='vmin', color='#c75d2c')
self.stretch_histogram.add_scatter('colorbar', x=[], y=[], ynorm='vmin', marker='square', stroke_width=33) # noqa: E501
with self.stretch_histogram.figure.hold_sync():
self.stretch_histogram.figure.axes[0].label = 'pixel value'
self.stretch_histogram.figure.axes[0].num_ticks = 3
self.stretch_histogram.figure.axes[0].tick_format = '0.1e'
self.stretch_histogram.figure.axes[1].label = 'density'
self.stretch_histogram.figure.axes[1].num_ticks = 2
self.stretch_histogram_widget = f'IPY_MODEL_{self.stretch_histogram.model_id}'

self.subset_visible = PlotOptionsSyncState(self, self.viewer, self.layer, 'visible',
Expand Down Expand Up @@ -713,14 +718,83 @@
stretch_vstep = (hist_lims[1] - hist_lims[0]) / 100.
self.stretch_vstep = np.round(stretch_vstep, decimals=-int(np.log10(stretch_vstep))+1) # noqa

self.stretch_histogram.viewer.state.hist_x_min = hist_lims[0]
self.stretch_histogram.viewer.state.hist_x_max = hist_lims[1]
with delay_callback(self.stretch_histogram.viewer.state, 'hist_x_min', 'hist_x_max'):
self.stretch_histogram.viewer.state.hist_x_min = hist_lims[0]
self.stretch_histogram.viewer.state.hist_x_max = hist_lims[1]

self.stretch_histogram.figure.title = f"{len(sub_data)} pixels"

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

@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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@skip_if_no_updates_since_last_active()
def _update_stretch_histogram_colorbar(self, msg={}):
"""Renders a colorbar on top of the histogram."""
if not self._viewer_is_image_viewer() 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

Check warning on line 740 in jdaviz/configs/default/plugins/plot_options/plot_options.py

View check run for this annotation

Codecov / codecov/patch

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

Added line #L740 was not covered by tests

if self.multiselect:
with self.stretch_histogram.hold_sync():
self.stretch_histogram._marks["colorbar"].x = []
self.stretch_histogram._marks["colorbar"].y = []
return

Check warning on line 746 in jdaviz/configs/default/plugins/plot_options/plot_options.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/default/plugins/plot_options/plot_options.py#L743-L746

Added lines #L743 - L746 were not covered by tests

if len(self.layer.selected_obj):
layer = self.layer.selected_obj[0]
else:
# skip further updates if no data are available:
return

Check warning on line 752 in jdaviz/configs/default/plugins/plot_options/plot_options.py

View check run for this annotation

Codecov / codecov/patch

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

Added line #L752 was not covered by tests

if isinstance(layer.layer, GroupedSubset):
# don't update histogram for subsets:
return

Check warning on line 756 in jdaviz/configs/default/plugins/plot_options/plot_options.py

View check run for this annotation

Codecov / codecov/patch

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

Added line #L756 was not covered by tests

# Cannot use layer.state because it can be out-of-sync
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]
Comment on lines +759 to +763
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't self.stretch_function_value from the plugin already?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

if color_mode == 'Colormaps':
cmap = colormaps[self.image_colormap.text]
if hasattr(cmap, "get_bad"):
bad_color = cmap.get_bad().tolist()[:3]
layer_cmap = cmap.with_extremes(bad=bad_color + [self.image_opacity_value])
else:
layer_cmap = cmap

Check warning on line 780 in jdaviz/configs/default/plugins/plot_options/plot_options.py

View check run for this annotation

Codecov / codecov/patch

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

Added line #L780 was not covered by tests

# Compute colormapped image
plane = layer_cmap(data)

else: # Monochromatic
# Get color
color = COLOR_CONVERTER.to_rgba_array(self.image_color_value)[0]
plane = data[:, np.newaxis] * color
plane[:, 3] = 1

Check warning on line 789 in jdaviz/configs/default/plugins/plot_options/plot_options.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/default/plugins/plot_options/plot_options.py#L787-L789

Added lines #L787 - L789 were not covered by tests

plane = np.clip(plane, 0, 1, out=plane)
ipycolors = [matplotlib.colors.rgb2hex(p, keep_alpha=False) for p in plane]

self.stretch_histogram._marks["colorbar"].x = x
self.stretch_histogram._marks["colorbar"].y = y
self.stretch_histogram._marks["colorbar"].colors = ipycolors
kecnry marked this conversation as resolved.
Show resolved Hide resolved

@observe('is_active', 'stretch_vmin_value', 'stretch_vmax_value', 'layer_selected',
'stretch_hist_nbins', 'image_contrast_value', 'image_bias_value',
'stretch_curve_visible')
Expand All @@ -745,6 +819,10 @@
return

for layer in self.layer.selected_obj:
if isinstance(layer.layer, GroupedSubset):
# don't update histogram for subsets:
continue

Check warning on line 824 in jdaviz/configs/default/plugins/plot_options/plot_options.py

View check run for this annotation

Codecov / codecov/patch

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

Added line #L824 was not covered by tests

# clear old mark, if it exists:
mark_label = f'{mark_label_prefix}{layer.label}'
mark_exists = mark_label in self.stretch_histogram.marks
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import pytest
import numpy as np
from numpy import allclose
from astropy import units as u
from astropy.nddata import NDData
from numpy.testing import assert_allclose
from photutils.datasets import make_4gaussians_image
from astropy.nddata import NDData
from astropy import units as u

from jdaviz.configs.default.plugins.plot_options.plot_options import SplineStretch


Expand Down Expand Up @@ -77,12 +76,24 @@ def test_stretch_histogram(cubeviz_helper, spectrum1d_cube_with_uncerts):

assert po.stretch_histogram is not None

# check the colorbar
cb = po.stretch_histogram._marks["colorbar"]
assert_allclose(cb.x, po.stretch_histogram.figure.marks[0].x)
assert_allclose(cb.y, 1)
assert cb.colors == [ # Gray scale, linear
'#050505', '#0f0f0f', '#191919', '#232323', '#2e2e2e',
'#383838', '#424242', '#4c4c4c', '#575757', '#616161',
'#6b6b6b', '#757575', '#808080', '#8a8a8a', '#949494',
'#9e9e9e', '#a8a8a8', '#b3b3b3', '#bdbdbd', '#c7c7c7',
'#d1d1d1', '#dcdcdc', '#e6e6e6', '#f0f0f0', '#fafafa']

hist_lyr = po.stretch_histogram.layers['histogram']
flux_cube_sample = hist_lyr.layer.data['x']

# changing viewer should change results
po.viewer.selected = 'uncert-viewer'
assert not allclose(hist_lyr.layer.data['x'], flux_cube_sample)
with pytest.raises(AssertionError):
assert_allclose(hist_lyr.layer.data['x'], flux_cube_sample)

po.viewer.selected = 'flux-viewer'
assert_allclose(hist_lyr.layer.data['x'], flux_cube_sample)
Expand Down Expand Up @@ -202,8 +213,8 @@ def test_stretch_spline(imviz_helper):
expected_y = [0., 0.05, 0.3, 0.9, 1.]

# Validate if the generated knots match the expected knots
np.testing.assert_allclose(knots_x, expected_x)
np.testing.assert_allclose(knots_y, expected_y)
assert_allclose(knots_x, expected_x)
assert_allclose(knots_y, expected_y)

# Update the stretch options to new values and verify the knots update correctly
with po.as_active():
Expand All @@ -219,8 +230,8 @@ def test_stretch_spline(imviz_helper):
expected_y = [0., 0.05, 0.3, 0.9, 1.]

# Validate if the generated knots for updated settings match the expected values
np.testing.assert_allclose(knots_x, expected_x)
np.testing.assert_allclose(knots_y, expected_y)
assert_allclose(knots_x, expected_x)
assert_allclose(knots_y, expected_y)

# Disable the stretch curve and ensure no knots or stretches are visible
with po.as_active():
Expand Down
22 changes: 20 additions & 2 deletions jdaviz/core/template_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -3420,10 +3420,28 @@ def clear_all_marks(self):
self.clear_marks(*self.marks.keys())

def _add_mark(self, cls, label, xnorm=False, ynorm=False, **kwargs):
"""
Parameters
----------
xnorm : bool or str
If True, axes will be normalized. If a string of an existing mark, this mark will
share that same x-axis scale.
ynorm : bool or str
If True, axes will be normalized. If a string of an existing mark, this mark will
share that same y-axis scale.
"""
if label in self._marks:
raise ValueError(f"mark with label '{label}' already exists")
mark = cls(scales={'x': bqplot.LinearScale() if xnorm else self.figure.axes[0].scale,
'y': bqplot.LinearScale() if ynorm else self.figure.axes[1].scale},
scales = {}
for dim, norm in zip(('x', 'y'), (xnorm, ynorm)):
if isinstance(norm, str) and norm in self._marks.keys():
# point to an existing marks scales
scales[dim] = self._marks[norm].scales[dim]
elif norm:
scales[dim] = bqplot.LinearScale()
else:
scales[dim] = self.figure.axes[0].scale
mark = cls(scales=scales,
labels=[label],
**kwargs)
self.figure.marks = self.figure.marks + [mark]
Expand Down
Loading
Loading