diff --git a/CHANGES.rst b/CHANGES.rst index 615f13b51e..99b9e9b8c7 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -79,6 +79,8 @@ Cubeviz - Increased spectral slider performance considerably. [#1550] +- Fixed the spectral subset highlighting of spatial subsets in the profile viewer. [#1528] + Imviz ^^^^^ diff --git a/jdaviz/configs/cubeviz/helper.py b/jdaviz/configs/cubeviz/helper.py index dc05ade181..d5ed135c9d 100644 --- a/jdaviz/configs/cubeviz/helper.py +++ b/jdaviz/configs/cubeviz/helper.py @@ -80,9 +80,7 @@ def select_wavelength(self, wavelength): if not isinstance(wavelength, (int, float)): raise TypeError("wavelength must be a float or int") # Retrieve the x slices from the spectrum viewer's marks - x_all = [m for m in self.app.get_viewer('spectrum-viewer').figure.marks - if m.__class__.__name__ in ['Lines', 'LinesGL'] - ][0].x + x_all = self.app.get_viewer('spectrum-viewer').native_marks[0].x index = np.argmin(abs(x_all - wavelength)) return self.select_slice(int(index)) diff --git a/jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py b/jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py index 31c3c2fc8e..44e7ab7d93 100644 --- a/jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py +++ b/jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py @@ -2,7 +2,7 @@ @pytest.mark.filterwarnings('ignore:No observer defined on WCS') -def test_moment_calculation(cubeviz_helper, spectrum1d_cube, tmpdir): +def test_data_selection(cubeviz_helper, spectrum1d_cube, tmpdir): app = cubeviz_helper.app # NOTE: these are the same underlying data. This works fine for the current scope # of the tests (to make sure checking/unchecking operations change the data exposed diff --git a/jdaviz/configs/cubeviz/plugins/viewers.py b/jdaviz/configs/cubeviz/plugins/viewers.py index 6281ffafa3..5da5ddcfcd 100644 --- a/jdaviz/configs/cubeviz/plugins/viewers.py +++ b/jdaviz/configs/cubeviz/plugins/viewers.py @@ -1,9 +1,10 @@ import numpy as np from glue.core import BaseData +from glue.core.subset import RoiSubsetState, RangeSubsetState from glue_jupyter.bqplot.image import BqplotImageView from jdaviz.core.registries import viewer_registry -from jdaviz.core.marks import SliceIndicatorMarks +from jdaviz.core.marks import SliceIndicatorMarks, ShadowSpatialSpectral from jdaviz.configs.default.plugins.viewers import JdavizViewerMixin from jdaviz.configs.cubeviz.helper import layer_is_cube_image_data from jdaviz.configs.imviz.helper import data_has_valid_wcs @@ -197,6 +198,63 @@ def __init__(self, *args, **kwargs): # default_tool_priority=['jdaviz:selectslice'] super().__init__(*args, **kwargs) + def _get_spatial_subset_layers(self): + return [ls for ls in self.state.layers + if isinstance(getattr(ls.layer, 'subset_state', None), RoiSubsetState)] + + def _get_spectral_subset_layers(self): + return [ls for ls in self.state.layers + if isinstance(getattr(ls.layer, 'subset_state', None), RangeSubsetState)] + + def _get_marks_for_layers(self, layers): + layers_list = list(self.state.layers) + # here we'll assume that all custom marks are subclasses of Lines/GL but don't directly + # use Lines/LinesGL (so an isinstance check won't be sufficient here) + layer_marks = self.native_marks + # and now we'll assume that the marks are in the same order as the layers, this should + # be the case as long as the order isn't manually resorted + return [layer_marks[layers_list.index(layer)] for layer in layers] + + def _on_subset_delete(self, msg): + # delete any ShadowSpatialSpectral mark for which either of the spectral or spatial marks + # no longer exists + spectral_marks = self._get_marks_for_layers(self._get_spectral_subset_layers()) + spatial_marks = self._get_marks_for_layers(self._get_spatial_subset_layers()) + self.figure.marks = [m for m in self.figure.marks + if not (isinstance(m, ShadowSpatialSpectral) and + (m.spatial_spectrum_mark in spatial_marks or + m.spectral_subset_mark in spectral_marks))] + + def _expected_subset_layer_default(self, layer_state): + """ + This gets called whenever the layer of an expected new subset is added, we want to set the + default for the linewidth depending on whether it is spatial or spectral, and handle + creating any necessary marks for spatial-spectral subset intersections. + """ + super()._expected_subset_layer_default(layer_state) + + this_mark = self._get_marks_for_layers([layer_state])[0] + new_marks = [] + + if isinstance(layer_state.layer.subset_state, RoiSubsetState): + layer_state.linewidth = 1 + + # need to add marks for every intersection between THIS spatial subset and ALL spectral + spectral_marks = self._get_marks_for_layers(self._get_spectral_subset_layers()) + for spectral_mark in spectral_marks: + new_marks += [ShadowSpatialSpectral(this_mark, spectral_mark)] + + else: + layer_state.linewidth = 3 + + # need to add marks for every intersection between THIS spectral subset and ALL spatial + spatial_marks = self._get_marks_for_layers(self._get_spatial_subset_layers()) + for spatial_mark in spatial_marks: + new_marks += [ShadowSpatialSpectral(spatial_mark, this_mark)] + + # NOTE: += or append won't pick up on change + self.figure.marks = self.figure.marks + new_marks + @property def slice_indicator(self): for mark in self.figure.marks: diff --git a/jdaviz/configs/default/plugins/viewers.py b/jdaviz/configs/default/plugins/viewers.py index 2c8e45b938..3fb5fe7836 100644 --- a/jdaviz/configs/default/plugins/viewers.py +++ b/jdaviz/configs/default/plugins/viewers.py @@ -25,6 +25,20 @@ def __init__(self, *args, **kwargs): # NOTE: anything here most likely won't be called by viewers because of inheritance order super().__init__(*args, **kwargs) + @property + def native_marks(self): + """ + Return all marks that are Lines/LinesGL objects (and not subclasses) + """ + return [m for m in self.figure.marks if m.__class__.__name__ in ['Lines', 'LinesGL']] + + @property + def custom_marks(self): + """ + Return all marks that are not Lines/LinesGL objects (but can be subclasses) + """ + return [m for m in self.figure.marks if m.__class__.__name__ not in ['Lines', 'LinesGL']] + def _subscribe_to_layers_update(self): # subscribe to new layers self._expected_subset_layers = [] diff --git a/jdaviz/configs/specviz/plugins/viewers.py b/jdaviz/configs/specviz/plugins/viewers.py index 5347a556d4..e0bdc5ce19 100644 --- a/jdaviz/configs/specviz/plugins/viewers.py +++ b/jdaviz/configs/specviz/plugins/viewers.py @@ -2,7 +2,7 @@ import warnings from glue.core import BaseData -from glue.core.subset import Subset, RoiSubsetState +from glue.core.subset import Subset from glue.config import data_translator from glue_jupyter.bqplot.profile import BqplotProfileView from glue.core.exceptions import IncompatibleAttribute @@ -64,12 +64,10 @@ def __init__(self, *args, **kwargs): # Change collapse function to sum self.state.function = 'sum' - def _expected_subset_layer_default(self, layer): - super()._expected_subset_layer_default(layer) - if isinstance(layer.layer.subset_state, RoiSubsetState): - layer.linewidth = 1 - else: - layer.linewidth = 3 + def _expected_subset_layer_default(self, layer_state): + super()._expected_subset_layer_default(layer_state) + + layer_state.linewidth = 3 def data(self, cls=None): # Grab the user's chosen statistic for collapsing data diff --git a/jdaviz/core/helpers.py b/jdaviz/core/helpers.py index f183c3857d..3d87680248 100644 --- a/jdaviz/core/helpers.py +++ b/jdaviz/core/helpers.py @@ -12,7 +12,7 @@ import numpy as np import astropy.units as u from glue.core import HubListener -from glue.core.message import SubsetCreateMessage +from glue.core.message import SubsetCreateMessage, SubsetDeleteMessage from jdaviz.app import Application @@ -59,6 +59,8 @@ def __init__(self, app=None, verbosity='warning', history_verbosity='info'): self.app.hub.subscribe(self, SubsetCreateMessage, handler=lambda msg: self._propagate_callback_to_viewers('_on_subset_create', msg)) # noqa + self.app.hub.subscribe(self, SubsetDeleteMessage, + handler=lambda msg: self._propagate_callback_to_viewers('_on_subset_delete', msg)) # noqa def _propagate_callback_to_viewers(self, method, msg): # viewers don't have access to the app/hub to subscribe to messages, so we'll diff --git a/jdaviz/core/marks.py b/jdaviz/core/marks.py index abfc621e95..bae9166d2b 100644 --- a/jdaviz/core/marks.py +++ b/jdaviz/core/marks.py @@ -3,6 +3,7 @@ from astropy import units as u from bqplot import LinearScale from bqplot.marks import Lines, Label, Scatter +from copy import deepcopy from glue.core import HubListener from specutils import Spectrum1D @@ -326,19 +327,32 @@ class ShadowMixin: Can manually override ``_on_shadowing_changed`` for more advanced logic cases. """ + def _get_id(self, mark): + return getattr(mark, '_model_id', None) + def _setup_shadowing(self, shadowing, sync_traits=[], other_traits=[]): - self._shadowing = shadowing - self._sync_traits = sync_traits + """ + sync_traits: traits to set now, and mirror any changes to shadowing in the future + other_trait: traits to set now, but not mirror in the future + """ + if not hasattr(self, '_shadowing'): + self._shadowing = {} + self._sync_traits = {} + shadowing_id = self._get_id(shadowing) + self._shadowing[shadowing_id] = shadowing + self._sync_traits[shadowing_id] = sync_traits # sync initial values for attr in sync_traits + other_traits: - self._on_shadowing_changed({'name': attr, 'new': getattr(shadowing, attr)}) + self._on_shadowing_changed({'name': attr, + 'new': getattr(shadowing, attr), + 'owner': shadowing}) # subscribe to future changes shadowing.observe(self._on_shadowing_changed) def _on_shadowing_changed(self, change): - if change['name'] in self._sync_traits: + if change['name'] in self._sync_traits.get(self._get_id(change.get('owner')), []): setattr(self, change['name'], change['new']) return @@ -359,13 +373,48 @@ def __init__(self, shadowing, shadow_width=1, **kwargs): ['scales', 'x', 'y', 'visible', 'line_style', 'marker'], ['stroke_width', 'marker_size']) + +class ShadowSpatialSpectral(Lines, HubListener, ShadowMixin): + """ + Shadow the mark of a spatial subset collapsed spectrum, with the mask from a spectral subset, + and the styling from the spatial subset. + """ + def __init__(self, spatial_spectrum_mark, spectral_subset_mark): + # spatial_spectrum_mark: Lines mark corresponding to the spatially-collapsed spectrum + # from a spatial subset + # spectral_subset_mark: Lines mark on the FULL cube corresponding to the glue-highlight + # of the spectral subset + super().__init__(scales=spatial_spectrum_mark.scales, marker=None) + + self._spatial_mark_id = self._get_id(spatial_spectrum_mark) + self._setup_shadowing(spatial_spectrum_mark, + ['scales', 'y', 'visible', 'line_style'], + ['x']) + + self._spectral_mark_id = self._get_id(spectral_subset_mark) + self._setup_shadowing(spectral_subset_mark, + ['stroke_width', 'x', 'y', 'visible', 'opacities', 'colors']) + + @property + def spatial_spectrum_mark(self): + return self._shadowing[self._spatial_mark_id] + + @property + def spectral_subset_mark(self): + return self._shadowing[self._spectral_mark_id] + def _on_shadowing_changed(self, change): - super()._on_shadowing_changed(change) - if change['name'] in ['stroke_width', 'marker_size']: - # apply the same, but increased by the shadow width - setattr(self, change['name'], - change['new'] + self._shadow_width if change['new'] else 0) - return + if hasattr(self, '_spectral_mark_id'): + if change['name'] == 'y': + # force a copy or else we'll overwrite the mask to the spatial mark! + change['new'] = deepcopy(self.spatial_spectrum_mark.y) + change['new'][np.isnan(self.spectral_subset_mark.y)] = np.nan + + elif change['name'] == 'visible': + # only show if BOTH shadowing marks are set to visible + change['new'] = self.spectral_subset_mark.visible and self.spatial_spectrum_mark.visible # noqa + + return super()._on_shadowing_changed(change) class ShadowLabelFixedY(Label, ShadowMixin): diff --git a/jdaviz/tests/test_subsets.py b/jdaviz/tests/test_subsets.py index 5dabd1ff7d..c0ec22ad54 100644 --- a/jdaviz/tests/test_subsets.py +++ b/jdaviz/tests/test_subsets.py @@ -8,6 +8,8 @@ from regions import EllipsePixelRegion, RectanglePixelRegion from specutils import SpectralRegion +from jdaviz.core.marks import ShadowSpatialSpectral + def test_region_from_subset_2d(cubeviz_helper): data = Data(flux=np.ones((128, 128)), label='Test 2D Flux') @@ -162,13 +164,19 @@ def test_region_spectral_spatial(cubeviz_helper, spectral_cube_wcs): cubeviz_helper.app.add_data_to_viewer('spectrum-viewer', 'Test Flux') cubeviz_helper.app.add_data_to_viewer('flux-viewer', 'Test Flux') - cubeviz_helper.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(5, 15.5)) + spectrum_viewer = cubeviz_helper.app.get_viewer("spectrum-viewer") + spectrum_viewer.apply_roi(XRangeROI(5, 15.5)) + + # should be no spatial-spectral intersection marks yet + assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 0 # noqa flux_viewer = cubeviz_helper.app.get_viewer("flux-viewer") # We set the active tool here to trigger a reset of the Subset state to "Create New" flux_viewer.toolbar.active_tool = flux_viewer.toolbar.tools['bqplot:rectangle'] flux_viewer.apply_roi(RectangularROI(1, 3.5, -0.2, 3.3)) + assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 1 # noqa + subsets = cubeviz_helper.app.get_subsets_from_viewer('spectrum-viewer', subset_type='spectral') reg = subsets.get('Subset 1') @@ -188,6 +196,14 @@ def test_region_spectral_spatial(cubeviz_helper, spectral_cube_wcs): assert_allclose(reg.width, 2.5) assert_allclose(reg.height, 3.5) + # add another spectral subset to ensure the spatial-spectral intersection marks are created as + # expected + # reset the tool to force a new selection instead of the default "replace" + spectrum_viewer.toolbar.active_tool = spectrum_viewer.toolbar.tools['jdaviz:panzoom'] + spectrum_viewer.toolbar.active_tool = spectrum_viewer.toolbar.tools['bqplot:xrange'] + spectrum_viewer.apply_roi(XRangeROI(3, 16)) + assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 2 # noqa + def test_disjoint_spectral_subset(cubeviz_helper, spectral_cube_wcs): subset_plugin = cubeviz_helper.app.get_tray_item_from_name('g-subset-plugin')