From 553d71787ad84cdd678dab0e4509c7efe82cb267 Mon Sep 17 00:00:00 2001 From: Jesse Averbukh Date: Fri, 5 May 2023 08:41:15 -0400 Subject: [PATCH 1/2] Allow editing of composite subsets in the subset plugin Updating composites work for spatial; in progress for spectral Use is_editable bool to disable centering composite subsets in imviz Fix failing test Change variable name and add comments Update comments Replace instances of is_editable Fix ADD and XOR cases, composite subset editing works from plugin Minor fixes Prevent user from setting subsets to invalid values Add changes --- CHANGES.rst | 2 + jdaviz/app.py | 79 +++++++++----- .../plugins/subset_plugin/subset_plugin.py | 88 ++++++++++----- .../plugins/subset_plugin/subset_plugin.vue | 5 +- jdaviz/configs/specviz/tests/test_helper.py | 4 +- jdaviz/tests/test_subsets.py | 101 ++++++++++++++---- 6 files changed, 200 insertions(+), 79 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 39434707cf..958977e913 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -16,6 +16,8 @@ New Features - Vertical (y-range) zoom tool for all spectrum and spectrum-2d viewers. [#2206] +- Allow Subset Plugin to edit composite subsets. [#2182] + Cubeviz ^^^^^^^ diff --git a/jdaviz/app.py b/jdaviz/app.py index 2b99d125f7..d2d3d9c217 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -909,8 +909,6 @@ def get_subsets(self, subset_name=None, spectral_only=False, # Remove duplicate spectral regions if is_spectral and isinstance(subset_region, SpectralRegion): subset_region = self._remove_duplicate_bounds(subset_region) - elif is_spectral: - subset_region = self._remove_duplicate_bounds_in_dict(subset_region) if spectral_only and is_spectral: if object_only and not simplify_spectral: @@ -942,22 +940,6 @@ def get_subsets(self, subset_name=None, spectral_only=False, else: return all_subsets - def _remove_duplicate_bounds_in_dict(self, subset_region): - new_subset_region = [] - for elem in subset_region: - if not new_subset_region: - new_subset_region.append(elem) - continue - unique = True - for elem2 in new_subset_region: - if (elem['region'].lower == elem2['region'].lower and - elem['region'].upper == elem2['region'].upper and - elem['glue_state'] == elem2['glue_state']): - unique = False - if unique: - new_subset_region.append(elem) - return new_subset_region - def _is_subset_spectral(self, subset_region): if isinstance(subset_region, SpectralRegion): return True @@ -1060,7 +1042,6 @@ def get_sub_regions(self, subset_state, simplify_spectral=True): return new_spec else: if isinstance(two, list): - # two[0]['glue_state'] = subset_state.state2.__class__.__name__ two[0]['glue_state'] = "AndNotState" # Return two first so that we preserve the chronology of how # subset regions are applied. @@ -1068,15 +1049,27 @@ def get_sub_regions(self, subset_state, simplify_spectral=True): elif subset_state.op is operator.and_: # This covers the AND subset mode - # Example of how this works: - # a = SpectralRegion(4 * u.um, 7 * u.um) - # b = SpectralRegion(5 * u.um, 6 * u.um) + # Example of how this works with "one" being the AND region + # and "two" being two Range subsets connected by an OR state: + # one = SpectralRegion(4.5 * u.um, 7.5 * u.um) + # two = SpectralRegion(4 * u.um, 5 * u.um) + SpectralRegion(7 * u.um, 8 * u.um) # - # b.invert(a.lower, a.upper) + # oppo = two.invert(one.lower, one.upper) + # Spectral Region, 1 sub-regions: + # (5.0 um, 7.0 um) + # + # oppo.invert(one.lower, one.upper) # Spectral Region, 2 sub-regions: - # (4.0 um, 5.0 um) (6.0 um, 7.0 um) + # (4.5 um, 5.0 um) (7.0 um, 7.5 um) if isinstance(two, SpectralRegion): - return two.invert(one.lower, one.upper) + # Taking an AND state of an empty region is allowed + # but there is no way for SpectralRegion to display that information. + # Instead, we raise a ValueError + if one.upper.value < two.lower.value or one.lower.value > two.upper.value: + raise ValueError("AND mode should overlap with existing subset") + oppo = two.invert(one.lower, one.upper) + + return oppo.invert(one.lower, one.upper) else: return two + one elif subset_state.op is operator.or_: @@ -1089,10 +1082,38 @@ def get_sub_regions(self, subset_state, simplify_spectral=True): elif two: return two elif subset_state.op is operator.xor: - # This covers the XOR case which is currently not working - return None - else: - return None + # This covers the ADD subset mode + + # Example of how this works, with "one" acting + # as the XOR region and "two" as two ranges joined + # by an OR: + # a = SpectralRegion(4 * u.um, 5 * u.um) + # b = SpectralRegion(6 * u.um, 9 * u.um) + # + # one = SpectralRegion(4.5 * u.um, 12 * u.um) + # two = a + b + + # two.invert(one.lower, one.upper) + # Spectral Region, 2 sub-regions: + # (5.0 um, 6.0 um) (9.0 um, 12.0 um) + + # one.invert(two.lower, two.upper) + # Spectral Region, 1 sub-regions: + # (4.0 um, 4.5 um) + + # two.invert(one.lower, one.upper) + one.invert(two.lower, two.upper) + # Spectral Region, 3 sub-regions: + # (4.0 um, 4.5 um) (5.0 um, 6.0 um) (9.0 um, 12.0 um) + + if isinstance(two, SpectralRegion): + if one.lower > two.lower: + # If one.lower is less than two.lower, it will be included + # in the two.invert() call. Otherwise, we can add it like this. + return (two.invert(one.lower, one.upper) + + one.invert(two.lower, two.upper)) + return two.invert(one.lower, one.upper) + else: + return two + one else: # This gets triggered in the InvertState case where state1 # is an object and state2 is None diff --git a/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py b/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py index 3af7db91f5..7cae959aa7 100644 --- a/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py +++ b/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py @@ -39,7 +39,7 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin): subplugins_opened = Any().tag(sync=True) - is_editable = Bool(False).tag(sync=True) + is_centerable = Bool(False).tag(sync=True) def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -96,7 +96,7 @@ def _sync_selected_from_ui(self, change): self.subset_definitions = [] self.subset_types = [] self.glue_state_types = [] - self.is_editable = False + self.is_centerable = False if not hasattr(self, 'subset_select'): # during initial init, this can trigger before the component is initialized @@ -132,9 +132,9 @@ def _unpack_get_subsets_for_ui(self): if not subset_information: return if len(subset_information) == 1: - self.is_editable = True + self.is_centerable = True else: - self.is_editable = False + self.is_centerable = False for spec in subset_information: subset_definition = [] @@ -200,35 +200,75 @@ def _get_subset_definition(self, *args): self._unpack_get_subsets_for_ui() def vue_update_subset(self, *args): + status, reason = self._check_input() + if not status: + self.hub.broadcast(SnackbarMessage(reason, color='error', sender=self)) + return + for index, sub in enumerate(self.subset_definitions): + if len(self.subset_states) <= index: + return sub_states = self.subset_states[index] for d_att in sub: if d_att["att"] == 'theta': # Humans use degrees but glue uses radians d_val = np.radians(d_att["value"]) else: d_val = float(d_att["value"]) + if float(d_att["orig"]) != d_val: if self.subset_types[index] == "Range": setattr(sub_states, d_att["att"], d_val) else: setattr(sub_states.roi, d_att["att"], d_val) - try: - # TODO: This commented out section is "more correct" because it - # adds the changed subregion to the data_collection.subset_groups - # tree. However, it still needs improvement and the section below - # allows updating similar to `main`. - # self.session.edit_subset_mode._combine_data(sub_states, - # override_mode=SUBSET_MODES[self.glue_state_types[index]]) - self.session.edit_subset_mode._combine_data(sub_states, override_mode=ReplaceMode) - except Exception as err: # pragma: no cover - self.hub.broadcast(SnackbarMessage( - f"Failed to update Subset: {repr(err)}", color='error', sender=self)) + try: + dc = self.data_collection + subsets = dc.subset_groups + self.session.edit_subset_mode._combine_data( + subsets[[x.label for x in subsets].index(self.subset_selected)].subset_state, + override_mode=ReplaceMode) + except Exception as err: # pragma: no cover + self.hub.broadcast(SnackbarMessage( + f"Failed to update Subset: {repr(err)}", color='error', sender=self)) + + def _check_input(self): + status = True + reason = "" + for index, sub in enumerate(self.subset_definitions): + lo = hi = xmin = xmax = ymin = ymax = None + for d_att in sub: + if d_att["att"] == "lo": + lo = d_att["value"] + elif d_att["att"] == "hi": + hi = d_att["value"] + elif d_att["att"] == "radius" and d_att["value"] <= 0: + status = False + reason = "Failed to update Subset: radius must be a positive scalar" + break + elif d_att["att"] == "xmin": + xmin = d_att["value"] + elif d_att["att"] == "xmax": + xmax = d_att["value"] + elif d_att["att"] == "ymin": + ymin = d_att["value"] + elif d_att["att"] == "ymax": + ymax = d_att["value"] + + if lo and hi and hi <= lo: + status = False + reason = "Failed to update Subset: lower bound must be less than upper bound" + break + elif xmin and xmax and ymin and ymax and (xmax - xmin <= 0 or ymax - ymin <= 0): + status = False + reason = "Failed to update Subset: width and length must be positive scalars" + break + + return status, reason def vue_recenter_subset(self, *args): - # Composite region cannot be edited. This only works for Imviz. - if not self.is_editable or self.config != 'imviz': # no-op + # Composite region cannot be centered. This only works for Imviz. + if not self.is_centerable or self.config != 'imviz': # no-op raise NotImplementedError( - f'Cannot recenter: is_editable={self.is_editable}, config={self.config}') + f'Cannot recenter: is_centerable={self.is_centerable}, config={self.config}') from photutils.aperture import ApertureStats from jdaviz.core.region_translators import regions2aperture, _get_region_from_spatial_subset @@ -265,7 +305,7 @@ def get_center(self): cen : number, tuple of numbers, or `None` The center of the Subset in ``x`` or ``(x, y)``, depending on the Subset type, if applicable. - If Subset is not editable, this returns `None`. + If Subset is not centerable, this returns `None`. Raises ------ @@ -273,8 +313,8 @@ def get_center(self): Subset type is not supported. """ - # Composite region cannot be edited. - if not self.is_editable: # no-op + # Composite region cannot be centered. + if not self.is_centerable: # no-op return subset_state = self.subset_select.selected_subset_state @@ -303,7 +343,7 @@ def get_center(self): def set_center(self, new_cen, update=False): """Set the desired center for the selected Subset, if applicable. - If Subset is not editable, nothing is done. + If Subset is not centerable, nothing is done. Parameters ---------- @@ -322,8 +362,8 @@ def set_center(self, new_cen, update=False): Subset type is not supported. """ - # Composite region cannot be edited, so just grab first element. - if not self.is_editable: # no-op + # Composite region cannot be centered, so just grab first element. + if not self.is_centerable: # no-op return subset_state = self.subset_select.selected_subset_state diff --git a/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue b/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue index 354c337611..286adbce32 100644 --- a/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue +++ b/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue @@ -23,7 +23,7 @@ - + @@ -58,13 +58,12 @@ :label="item.name" v-model.number="item.value" type="number" - :disabled="!is_editable" > - Update + Update diff --git a/jdaviz/configs/specviz/tests/test_helper.py b/jdaviz/configs/specviz/tests/test_helper.py index 82543695ff..12603091ff 100644 --- a/jdaviz/configs/specviz/tests/test_helper.py +++ b/jdaviz/configs/specviz/tests/test_helper.py @@ -170,7 +170,7 @@ def test_get_spectral_regions_does_not_raise_value_error(self): def test_get_spectral_regions_composite_region(self): spectrum_viewer = self.spec_app.app.get_viewer("spectrum-viewer") - self.spec_app.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(6000, 6400)) + self.spec_app.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(6000, 7400)) spectrum_viewer.session.edit_subset_mode._mode = AndNotMode @@ -187,7 +187,7 @@ def test_get_spectral_regions_composite_region(self): assert_quantity_allclose(spec_region['Subset 1'].subregions[0][0].value, 7300., atol=1e-5) assert_quantity_allclose(spec_region['Subset 1'].subregions[0][1].value, - 7800., atol=1e-5) + 7400., atol=1e-5) def test_get_spectral_regions_composite_region_multiple_and_nots(self): spectrum_viewer = self.spec_app.app.get_viewer("spectrum-viewer") diff --git a/jdaviz/tests/test_subsets.py b/jdaviz/tests/test_subsets.py index 972231c691..57098e739a 100644 --- a/jdaviz/tests/test_subsets.py +++ b/jdaviz/tests/test_subsets.py @@ -5,7 +5,7 @@ from glue.core import Data from glue.core.roi import CircularROI, EllipticalROI, RectangularROI, XRangeROI -from glue.core.edit_subset_mode import AndMode, AndNotMode, OrMode +from glue.core.edit_subset_mode import AndMode, AndNotMode, OrMode, XorMode from regions import PixCoord, CirclePixelRegion, RectanglePixelRegion, EllipsePixelRegion from numpy.testing import assert_allclose @@ -38,7 +38,7 @@ def test_region_from_subset_2d(cubeviz_helper): assert subset_plugin.subset_selected == "Subset 1" assert subset_plugin.subset_types == ["EllipticalROI"] - assert subset_plugin.is_editable + assert subset_plugin.is_centerable for key in ("orig", "value"): assert subset_plugin._get_value_from_subset_definition(0, "X Center", key) == 1 assert subset_plugin._get_value_from_subset_definition(0, "Y Center", key) == 3.5 @@ -76,7 +76,7 @@ def test_region_from_subset_3d(cubeviz_helper): assert subset_plugin.subset_selected == "Subset 1" assert subset_plugin.subset_types == ["RectangularROI"] - assert subset_plugin.is_editable + assert subset_plugin.is_centerable assert subset_plugin.get_center() == (2.25, 1.55) for key in ("orig", "value"): assert subset_plugin._get_value_from_subset_definition(0, "Xmin", key) == 1 @@ -127,7 +127,7 @@ def test_region_from_subset_3d(cubeviz_helper): cubeviz_helper.app.get_viewer('flux-viewer').apply_roi(CircularROI(xc=3, yc=4, radius=2.4)) assert subset_plugin.subset_selected == "Subset 2" assert subset_plugin.subset_types == ["CircularROI"] - assert subset_plugin.is_editable + assert subset_plugin.is_centerable for key in ("orig", "value"): assert subset_plugin._get_value_from_subset_definition(0, "X Center", key) == 3 assert subset_plugin._get_value_from_subset_definition(0, "Y Center", key) == 4 @@ -154,7 +154,7 @@ def test_region_from_subset_profile(cubeviz_helper, spectral_cube_wcs): assert subset_plugin.subset_selected == "Subset 1" assert subset_plugin.subset_types == ["Range"] - assert subset_plugin.is_editable + assert subset_plugin.is_centerable assert_allclose(subset_plugin.get_center(), 10.25) for key in ("orig", "value"): assert subset_plugin._get_value_from_subset_definition(0, "Lower bound", key) == 5 @@ -285,31 +285,26 @@ def test_disjoint_spectral_subset(cubeviz_helper, spectral_cube_wcs): assert subset_plugin.subset_selected == "Subset 1" assert subset_plugin.subset_types == ["Range", "Range"] assert subset_plugin.glue_state_types == ["RangeSubsetState", "OrState"] - # assert not subset_plugin.is_editable + + # Make sure that certain things are not possible because we are + # dealing with a composite spectral subset + subset_plugin.set_center(99, update=True) # This is no-op assert subset_plugin.get_center() is None - # TODO: Should this be changed to something else? - # subset_plugin.set_center(99, update=True) # This is no-op + for key in ("orig", "value"): assert subset_plugin._get_value_from_subset_definition(1, "Lower bound", key) == 30 assert subset_plugin._get_value_from_subset_definition(1, "Upper bound", key) == 35 assert subset_plugin._get_value_from_subset_definition(0, "Lower bound", key) == 5 assert subset_plugin._get_value_from_subset_definition(0, "Upper bound", key) == 15.5 - # This should not be possible via GUI but here we change - # something to make sure no-op is really no-op. - subset_plugin._set_value_in_subset_definition(0, "Lower bound", "value", 25) - # subset_plugin.vue_update_subset() - # "value" here does not matter. It is going to get overwritten next time Subset is processed. - assert subset_plugin._get_value_from_subset_definition(0, "Lower bound", "value") == 25 - assert subset_plugin._get_value_from_subset_definition(0, "Lower bound", "orig") == 5 + # We will now update one of the bounds of the composite subset + subset_plugin._set_value_in_subset_definition(1, "Lower bound", "value", 25) + subset_plugin.vue_update_subset() + assert subset_plugin._get_value_from_subset_definition(1, "Lower bound", "value") == 25 + assert subset_plugin._get_value_from_subset_definition(1, "Lower bound", "orig") == 25 reg = cubeviz_helper.app.get_subsets('Subset 1') - assert_quantity_allclose(reg[1].lower, 30.0*u.Hz) # Still the old value - - # See, never happened. - subset_plugin.subset_selected = "Create New" - subset_plugin.subset_selected = "Subset 1" - assert subset_plugin._get_value_from_subset_definition(0, "Lower bound", "value") == 5 + assert_quantity_allclose(reg[1].lower, 25.0*u.Hz) # It is now the updated value def test_composite_region_from_subset_3d(cubeviz_helper): @@ -410,6 +405,23 @@ def test_composite_region_with_consecutive_and_not_states(cubeviz_helper): assert subset_plugin.subset_types == ['CircularROI', 'RectangularROI', 'EllipticalROI'] assert subset_plugin.glue_state_types == ['AndState', 'AndNotState', 'AndNotState'] + # This should be prevented since radius must be positive + subset_plugin._set_value_in_subset_definition(0, "Radius", "value", 0) + subset_plugin.vue_update_subset() + # assert subset_plugin._get_value_from_subset_definition(0, "Radius", "value") == 5 + + # This should also be prevented since a rectangle must have positive width + # and length + subset_plugin._set_value_in_subset_definition(1, "Xmin", "value", 0) + subset_plugin._set_value_in_subset_definition(1, "Xmax", "value", 0) + subset_plugin.vue_update_subset() + + # Make sure changes were not propagated + reg = cubeviz_helper.app.get_subsets("Subset 1") + assert reg[0]['subset_state'].roi.radius == 5 + assert reg[1]['subset_state'].roi.xmin == 25 + assert reg[1]['subset_state'].roi.xmax == 30 + def test_composite_region_with_imviz(imviz_helper, image_2d_wcs): arr = np.ones((10, 10)) @@ -492,3 +504,50 @@ def test_composite_region_from_subset_2d(specviz_helper, spectrum1d): assert subset_plugin.subset_selected == "Subset 1" assert subset_plugin.subset_types == ['Range', 'Range', 'Range', 'Range'] assert subset_plugin.glue_state_types == ['AndState', 'AndNotState', 'OrState', 'AndState'] + + +def test_edit_composite_spectral_subset(specviz_helper, spectrum1d): + specviz_helper.load_spectrum(spectrum1d) + viewer = specviz_helper.app.get_viewer(specviz_helper._default_spectrum_viewer_reference_name) + + viewer.apply_roi(XRangeROI(6200, 6800)) + specviz_helper.app.session.edit_subset_mode.mode = OrMode + viewer.apply_roi(XRangeROI(7200, 7600)) + + specviz_helper.app.session.edit_subset_mode.mode = XorMode + viewer.apply_roi(XRangeROI(6200, 7600)) + + reg = specviz_helper.app.get_subsets("Subset 1") + assert reg.lower.value == 6800 and reg.upper.value == 7200 + + subset_plugin = specviz_helper.app.get_tray_item_from_name('g-subset-plugin') + + # We will now update one of the bounds of the composite subset + subset_plugin._set_value_in_subset_definition(0, "Lower bound", "value", 6000) + subset_plugin.vue_update_subset() + + # Since we updated one of the Range objects and it's lower bound + # is now lower than the XOR region bound, the region from 6000 to + # 6200 should now be visible in the viewer. + reg = specviz_helper.app.get_subsets("Subset 1") + assert reg[0].lower.value == 6000 and reg[0].upper.value == 6200 + assert reg[1].lower.value == 6800 and reg[1].upper.value == 7200 + + # This makes it so that only spectral regions within this bound + # are visible, so the API should reflect that. + specviz_helper.app.session.edit_subset_mode.mode = AndMode + viewer.apply_roi(XRangeROI(6600, 7400)) + + reg = specviz_helper.app.get_subsets("Subset 1") + assert reg.lower.value == 6800 and reg[0].upper.value == 7200 + + # This should be prevented by the _check_inputs method + subset_plugin._set_value_in_subset_definition(0, "Lower bound", "value", 8000) + subset_plugin.vue_update_subset() + reg2 = specviz_helper.app.get_subsets("Subset 1") + assert reg.lower.value == reg2.lower.value + assert reg.upper.value == reg2.upper.value + + viewer.apply_roi(XRangeROI(7800, 8000)) + with pytest.raises(ValueError, match="AND mode should overlap with existing subset"): + specviz_helper.app.get_subsets("Subset 1") From c4f6f8530710f79bf06381d8bfe3606273949da5 Mon Sep 17 00:00:00 2001 From: Jesse Averbukh Date: Tue, 16 May 2023 12:05:54 -0400 Subject: [PATCH 2/2] Remove commented line --- jdaviz/tests/test_subsets.py | 1 - 1 file changed, 1 deletion(-) diff --git a/jdaviz/tests/test_subsets.py b/jdaviz/tests/test_subsets.py index 57098e739a..76e8ef3b1d 100644 --- a/jdaviz/tests/test_subsets.py +++ b/jdaviz/tests/test_subsets.py @@ -408,7 +408,6 @@ def test_composite_region_with_consecutive_and_not_states(cubeviz_helper): # This should be prevented since radius must be positive subset_plugin._set_value_in_subset_definition(0, "Radius", "value", 0) subset_plugin.vue_update_subset() - # assert subset_plugin._get_value_from_subset_definition(0, "Radius", "value") == 5 # This should also be prevented since a rectangle must have positive width # and length