From 663eac6d87ea43ffeb6b1c01c8ce54af8b5e7c05 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 28 Mar 2024 10:23:50 +0000 Subject: [PATCH 01/12] Revert "BUG: Always downsample histogram for Imviz in Plot Options (#2735)" This reverts commit fbef31f4c20716a29fc3f7ec96e371ee4b283a67. --- .../default/plugins/plot_options/plot_options.py | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/jdaviz/configs/default/plugins/plot_options/plot_options.py b/jdaviz/configs/default/plugins/plot_options/plot_options.py index 4dff3fbf69..66a53dc605 100644 --- a/jdaviz/configs/default/plugins/plot_options/plot_options.py +++ b/jdaviz/configs/default/plugins/plot_options/plot_options.py @@ -973,12 +973,8 @@ def _update_stretch_histogram(self, msg={}): x_max = x_limits.max() y_min = max(y_limits.min(), 0) y_max = y_limits.max() + arr = comp.data[y_min:y_max, x_min:x_max] - if self.config == "imviz": - # Downsample input data to about 400px (as per compass.vue) for performance. - xstep = max(1, round(arr.shape[1] / 400)) - ystep = max(1, round(arr.shape[0] / 400)) - arr = arr[::ystep, ::xstep] sub_data = arr.ravel() else: @@ -999,14 +995,8 @@ def _update_stretch_histogram(self, msg={}): sub_data = comp.data[inds].ravel() else: - if self.config == "imviz": - # Downsample input data to about 400px (as per compass.vue) for performance. - xstep = max(1, round(data.shape[1] / 400)) - ystep = max(1, round(data.shape[0] / 400)) - arr = comp[::ystep, ::xstep] - else: - # include all data, regardless of zoom limits - arr = comp.data + # include all data, regardless of zoom limits + arr = comp.data sub_data = arr.ravel() # filter out nans (or else bqplot will fail) From 0a6aea25f177002e39ea54b17122be7eb44859cd Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 28 Mar 2024 11:09:19 +0000 Subject: [PATCH 02/12] Use glue's ability to randomly sample values when computing histograms and image statistics, and remove code to handle NaN values that caused the whole array to be loaded into memory. --- .../plugins/plot_options/plot_options.py | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/jdaviz/configs/default/plugins/plot_options/plot_options.py b/jdaviz/configs/default/plugins/plot_options/plot_options.py index 66a53dc605..3e10c990ee 100644 --- a/jdaviz/configs/default/plugins/plot_options/plot_options.py +++ b/jdaviz/configs/default/plugins/plot_options/plot_options.py @@ -36,6 +36,8 @@ __all__ = ['PlotOptions'] +RANDOM_SUBSET_SIZE = 10_000 + def _register_random_cmap( cmap_name, @@ -999,15 +1001,21 @@ def _update_stretch_histogram(self, msg={}): arr = comp.data sub_data = arr.ravel() - # filter out nans (or else bqplot will fail) - if np.any(np.isnan(sub_data)): - sub_data = sub_data[~np.isnan(sub_data)] - + self.stretch_histogram.viewer.state.random_subset = RANDOM_SUBSET_SIZE self.stretch_histogram._update_data('histogram', x=sub_data) if len(sub_data) > 0: - interval = PercentileInterval(95) - hist_lims = interval.get_limits(sub_data) + + # Use glue to compute the statistics since this allows us to use + # a random subset of the data to compute the histogram + glue_data = self.stretch_histogram.app.data_collection['histogram'] + hist_lims = ( + glue_data.compute_statistic('percentile', glue_data.id['x'], + percentile=2.5, random_subset=RANDOM_SUBSET_SIZE), + glue_data.compute_statistic('percentile', glue_data.id['x'], + percentile=97.5, random_subset=RANDOM_SUBSET_SIZE) + ) + # set the stepsize for vmin/vmax to be approximately 1% of the range of the # histogram (within the percentile interval), rounded to 1-2 significant digits # to avoid random step sizes. This logic is somewhat arbitrary and can be safely From ee24ab99c5bef751512121a35d495aa37a30596e Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 28 Mar 2024 11:22:46 +0000 Subject: [PATCH 03/12] Remove unused import --- jdaviz/configs/default/plugins/plot_options/plot_options.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/jdaviz/configs/default/plugins/plot_options/plot_options.py b/jdaviz/configs/default/plugins/plot_options/plot_options.py index 3e10c990ee..7b525659ea 100644 --- a/jdaviz/configs/default/plugins/plot_options/plot_options.py +++ b/jdaviz/configs/default/plugins/plot_options/plot_options.py @@ -4,9 +4,8 @@ import matplotlib import numpy as np -from astropy.visualization import ( - ManualInterval, ContrastBiasStretch, PercentileInterval -) +from astropy.visualization import ManualInterval, ContrastBiasStretch + from echo import delay_callback from traitlets import Any, Dict, Float, Bool, Int, List, Unicode, observe From 394f6ac8c0906ec5f6a97e3020c54be461728556 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Tue, 9 Apr 2024 10:28:28 +0100 Subject: [PATCH 04/12] Use order='K' in ravel() to avoid copy --- jdaviz/configs/default/plugins/plot_options/plot_options.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jdaviz/configs/default/plugins/plot_options/plot_options.py b/jdaviz/configs/default/plugins/plot_options/plot_options.py index 7b525659ea..af36d5f31d 100644 --- a/jdaviz/configs/default/plugins/plot_options/plot_options.py +++ b/jdaviz/configs/default/plugins/plot_options/plot_options.py @@ -976,7 +976,7 @@ def _update_stretch_histogram(self, msg={}): y_max = y_limits.max() arr = comp.data[y_min:y_max, x_min:x_max] - sub_data = arr.ravel() + sub_data = arr.ravel(order="K") else: # spectrum-2d-viewer, for example. We'll assume the viewer @@ -993,12 +993,12 @@ def _update_stretch_histogram(self, msg={}): (y_data >= viewer.state.y_min) & (y_data <= viewer.state.y_max)) - sub_data = comp.data[inds].ravel() + sub_data = comp.data[inds].ravel(order="K") else: # include all data, regardless of zoom limits arr = comp.data - sub_data = arr.ravel() + sub_data = arr.ravel(order="K") self.stretch_histogram.viewer.state.random_subset = RANDOM_SUBSET_SIZE self.stretch_histogram._update_data('histogram', x=sub_data) From 16de08f36717e8eea7ec2d0e4648c3b1f7bd2e30 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Tue, 9 Apr 2024 12:08:01 +0100 Subject: [PATCH 05/12] Avoid calling ravel() since this causes a copy for cutouts --- .../configs/default/plugins/plot_options/plot_options.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/jdaviz/configs/default/plugins/plot_options/plot_options.py b/jdaviz/configs/default/plugins/plot_options/plot_options.py index af36d5f31d..4f348eb94e 100644 --- a/jdaviz/configs/default/plugins/plot_options/plot_options.py +++ b/jdaviz/configs/default/plugins/plot_options/plot_options.py @@ -975,8 +975,7 @@ def _update_stretch_histogram(self, msg={}): y_min = max(y_limits.min(), 0) y_max = y_limits.max() - arr = comp.data[y_min:y_max, x_min:x_max] - sub_data = arr.ravel(order="K") + sub_data = comp.data[y_min:y_max, x_min:x_max] else: # spectrum-2d-viewer, for example. We'll assume the viewer @@ -993,12 +992,11 @@ def _update_stretch_histogram(self, msg={}): (y_data >= viewer.state.y_min) & (y_data <= viewer.state.y_max)) - sub_data = comp.data[inds].ravel(order="K") + sub_data = comp.data[inds] else: # include all data, regardless of zoom limits - arr = comp.data - sub_data = arr.ravel(order="K") + sub_data = comp.data self.stretch_histogram.viewer.state.random_subset = RANDOM_SUBSET_SIZE self.stretch_histogram._update_data('histogram', x=sub_data) From 5da50e44f621542bf8a8253d0d333d2e2bf91c3e Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Mon, 15 Apr 2024 23:02:25 +0100 Subject: [PATCH 06/12] Add comment to explain percentile values Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --- jdaviz/configs/default/plugins/plot_options/plot_options.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jdaviz/configs/default/plugins/plot_options/plot_options.py b/jdaviz/configs/default/plugins/plot_options/plot_options.py index 4f348eb94e..393768209f 100644 --- a/jdaviz/configs/default/plugins/plot_options/plot_options.py +++ b/jdaviz/configs/default/plugins/plot_options/plot_options.py @@ -1004,7 +1004,8 @@ def _update_stretch_histogram(self, msg={}): if len(sub_data) > 0: # Use glue to compute the statistics since this allows us to use - # a random subset of the data to compute the histogram + # a random subset of the data to compute the histogram. + # The 2.5 and 97.5 hardcoded here is equivalent to PercentileInterval(95).get_limits(sub_data) glue_data = self.stretch_histogram.app.data_collection['histogram'] hist_lims = ( glue_data.compute_statistic('percentile', glue_data.id['x'], From b4f404938e7d853e81241ca3667a3638f7e33b36 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Tue, 16 Apr 2024 12:24:08 +0100 Subject: [PATCH 07/12] Fix codestyle --- jdaviz/configs/default/plugins/plot_options/plot_options.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jdaviz/configs/default/plugins/plot_options/plot_options.py b/jdaviz/configs/default/plugins/plot_options/plot_options.py index 393768209f..a8f9b3f005 100644 --- a/jdaviz/configs/default/plugins/plot_options/plot_options.py +++ b/jdaviz/configs/default/plugins/plot_options/plot_options.py @@ -1005,7 +1005,8 @@ def _update_stretch_histogram(self, msg={}): # Use glue to compute the statistics since this allows us to use # a random subset of the data to compute the histogram. - # The 2.5 and 97.5 hardcoded here is equivalent to PercentileInterval(95).get_limits(sub_data) + # The 2.5 and 97.5 hardcoded here is equivalent to + # PercentileInterval(95).get_limits(sub_data) glue_data = self.stretch_histogram.app.data_collection['histogram'] hist_lims = ( glue_data.compute_statistic('percentile', glue_data.id['x'], From 46d376f4502cb3817ed98c031a1d78c00d9c9ee8 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 25 Apr 2024 09:52:28 +0100 Subject: [PATCH 08/12] Fix issue when array len() match but .shape doesn't --- jdaviz/core/template_mixin.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jdaviz/core/template_mixin.py b/jdaviz/core/template_mixin.py index 4c91451656..8ac276fa09 100644 --- a/jdaviz/core/template_mixin.py +++ b/jdaviz/core/template_mixin.py @@ -4631,13 +4631,13 @@ def _update_data(self, label, reset_lims=False, **kwargs): data = self.app.data_collection[label] # if not provided, fallback on existing data - length_mismatch = False + shape_mismatch = False for component in self._viewer_components: kwargs.setdefault(component, data[component]) - if len(kwargs[component]) != len(data[component]): - length_mismatch = True + if kwargs[component].shape != data[component].shape: + shape_mismatch = True - if not length_mismatch: + if not shape_mismatch: # then we can update the existing entry components = {c.label: c for c in data.components} data.update_components({components[comp]: kwargs[comp] From 197789b7e3912a07cf0d94f2ac88d14addcae136 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 25 Apr 2024 09:56:42 +0100 Subject: [PATCH 09/12] Adjust reference values for test --- jdaviz/core/tests/test_tools.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/jdaviz/core/tests/test_tools.py b/jdaviz/core/tests/test_tools.py index 4390c03fed..912176a8ea 100644 --- a/jdaviz/core/tests/test_tools.py +++ b/jdaviz/core/tests/test_tools.py @@ -74,6 +74,11 @@ def test_stretch_bounds(imviz_helper): def test_stretch_bounds_and_spline(imviz_helper): + + # As the histogram randomly samples the array, we should make sure the + # values used here are reproducible + np.random.seed(42) + image_1 = NDData(make_4gaussians_image(), unit=u.nJy) imviz_helper.load_data(image_1) po = imviz_helper.plugins["Plot Options"] @@ -93,7 +98,7 @@ def test_stretch_bounds_and_spline(imviz_helper): knots_after_drag_move = ( [0.0, 0.1, 0.21712585033417825, 0.7, 1.0], - [0.0, 0.05, 0.2900993441358025, 0.9, 1.0], + [0.0, 0.05, 0.2852214046563617, 0.9, 1.0], ) stretch_tool.on_mouse_event(knot_move_msg) From b81482898e29cd0294abfb184e3198fd5ce72862 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 25 Apr 2024 09:59:05 +0100 Subject: [PATCH 10/12] Bumped minimum required version of glue-core and glue-jupyter --- pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index e48e909f8b..2e273e2855 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,8 +12,8 @@ dependencies = [ "traitlets>=5.0.5", "bqplot>=0.12.37", "bqplot-image-gl>=1.4.11", - "glue-core>=1.18.0", - "glue-jupyter>=0.20", + "glue-core>=1.20.0", + "glue-jupyter>=0.21.0", "echo>=0.5.0", "ipykernel>=6.19.4", "ipyvue>=1.6", From 30a4707665dc52d2e9d30424a68f0fd8125f3252 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 26 Apr 2024 12:05:57 +0100 Subject: [PATCH 11/12] Fix case where data is not a Numpy array --- jdaviz/core/template_mixin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jdaviz/core/template_mixin.py b/jdaviz/core/template_mixin.py index 8ac276fa09..67b2886e9e 100644 --- a/jdaviz/core/template_mixin.py +++ b/jdaviz/core/template_mixin.py @@ -4634,7 +4634,7 @@ def _update_data(self, label, reset_lims=False, **kwargs): shape_mismatch = False for component in self._viewer_components: kwargs.setdefault(component, data[component]) - if kwargs[component].shape != data[component].shape: + if np.asarray(kwargs[component]).shape != data[component].shape: shape_mismatch = True if not shape_mismatch: From 0bc72d14c3c63cfb2e0035affea7cca7679fad93 Mon Sep 17 00:00:00 2001 From: "P. L. Lim" <2090236+pllim@users.noreply.github.com> Date: Tue, 7 May 2024 15:55:27 -0400 Subject: [PATCH 12/12] Add change log --- CHANGES.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index c4b3970a08..a6f54a3925 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -127,6 +127,9 @@ Other Changes and Additions Bug Fixes --------- +- Histogram in Plot Options now uses random sampling to better + represent the data without sacrificing performance. [#2771] + Cubeviz ^^^^^^^