Add app method to simplify spectral subset (#2237)

* Add app method to simplify spectral subset

Add simplify button to subset plugin

* Fix xor bug exposed by simplify button

* Fix style checks

* Remove print statements

* Add tooltip to simplify button

* Make button appear only if the subset can be simplified

* Make simplify button appear only when applicable

* Apply suggestions from code review

Co-authored-by: P. L. Lim <[email protected]>

* Update changes file

---------

Co-authored-by: P. L. Lim <[email protected]>

CHANGES.rst

@@ -6,6 +6,9 @@ New Features
 
 - Introduce jdaviz.open to automatically detect the appropriate config and load data [#2221]
 
+- Add Simplify button to subset plugin to make composite spectral subsets more user
+  friendly. [#2237]
+
 Cubeviz
 ^^^^^^^
 

jdaviz/app.py

@@ -1106,12 +1106,32 @@ def get_sub_regions(self, subset_state, simplify_spectral=True):
                     #   (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)
+                        new_region = None
+                        temp_region = None
+                        for subregion in two:
+                            # Add all subregions that do not intersect with XOR region
+                            # to a new SpectralRegion object
+                            if subregion.lower > one.upper or subregion.upper < one.lower:
+                                if not new_region:
+                                    new_region = subregion
+                                else:
+                                    new_region += subregion
+                            # All other subregions are added to temp_region
+                            else:
+                                if not temp_region:
+                                    temp_region = subregion
+                                else:
+                                    temp_region += subregion
+                        # This is the main application of XOR to other regions
+                        if not new_region:
+                            new_region = temp_region.invert(one.lower, one.upper)
+                        else:
+                            new_region = new_region + temp_region.invert(one.lower, one.upper)
+                        # This adds the edge regions that are otherwise not included
+                        if not (one.lower == temp_region.lower and one.upper == temp_region.upper):
+                            new_region = new_region + one.invert(temp_region.lower,
+                                                                 temp_region.upper)
+                        return new_region
                     else:
                         return two + one
             else:
@@ -1127,6 +1147,42 @@ def get_sub_regions(self, subset_state, simplify_spectral=True):
             elif isinstance(subset_state, RangeSubsetState):
                 return self._get_range_subset_bounds(subset_state, simplify_spectral)
 
+    def simplify_spectral_subset(self, subset_name, att, overwrite=False):
+        """
+        Convert a composite spectral subset consisting of And, AndNot, Or, Replace, and Xor
+        into one consisting of just Range and Or state objects.
+
+        Parameters
+        ----------
+        subset_name : str
+            Name of subset to simplify.
+        att : str
+            Attribute that the subset uses to apply to data.
+        overwrite : bool
+            Whether to update the current subset with the simplified state or apply it
+            to a new subset.
+        """
+        spectral_region = self.get_subsets(subset_name, spectral_only=True)
+        new_state = None
+        # Reverse through sub regions so that they are added back
+        # in the order of lowest values to highest
+        for index in range(len(spectral_region) - 1, -1, -1):
+            convert_to_range = RangeSubsetState(spectral_region[index].lower.value,
+                                                spectral_region[index].upper.value,
+                                                att)
+            if new_state is None:
+                new_state = convert_to_range
+            else:
+                new_state = new_state | convert_to_range
+
+        dc = self.data_collection
+        if not overwrite:
+            dc.new_subset_group(subset_state=new_state)
+        else:
+            old_subset = [subsets for subsets in dc.subset_groups
+                          if subsets.label == subset_name][0]
+            old_subset.subset_state = new_state
+
     def add_data(self, data, data_label=None, notify_done=True):
         """
         Add data to the Glue ``DataCollection``.

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

@@ -46,6 +46,7 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
     subplugins_opened = Any().tag(sync=True)
 
     is_centerable = Bool(False).tag(sync=True)
+    can_simplify = Bool(False).tag(sync=True)
 
     icon_replace = Unicode(read_icon(os.path.join(icon_path("glue_replace", icon_format="svg")), 'svg+xml')).tag(sync=True)  # noqa
     icon_or = Unicode(read_icon(os.path.join(icon_path("glue_or", icon_format="svg")), 'svg+xml')).tag(sync=True)  # noqa
@@ -199,6 +200,15 @@ def _unpack_get_subsets_for_ui(self):
                 self.glue_state_types = self.glue_state_types + [glue_state]
                 self.subset_states = self.subset_states + [subset_state]
 
+        simplifiable_states = set(['AndState', 'XorState', 'AndNotState'])
+        # Check if the subset has more than one subregion, is a range subset type, and
+        # uses one of the states that can be simplified.
+        if (len(self.subset_states) > 1 and isinstance(self.subset_states[0], RangeSubsetState)
+                and len(simplifiable_states - set(self.glue_state_types)) < 3):
+            self.can_simplify = True
+        else:
+            self.can_simplify = False
+
     def _get_subset_definition(self, *args):
         """
         Retrieve the parameters defining the selected subset, for example the
@@ -211,6 +221,16 @@ def _get_subset_definition(self, *args):
 
         self._unpack_get_subsets_for_ui()
 
+    def vue_simplify_subset(self, *args):
+        if len(self.subset_states) < 2:
+            self.hub.broadcast(SnackbarMessage("Cannot simplify spectral subset "
+                                               "of length less than 2", color='warning',
+                                               sender=self))
+            return
+        att = self.subset_states[0].att
+        self.app.simplify_spectral_subset(subset_name=self.subset_selected, att=att,
+                                          overwrite=True)
+
     def vue_update_subset(self, *args):
         status, reason = self._check_input()
         if not status:

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue

@@ -88,6 +88,9 @@
     </div>
 
       <v-row justify="end" no-gutters>
+        <j-tooltip v-if="can_simplify" tooltipcontent="Convert composite subset to use only add mode to connect subregions">
+          <v-btn color="primary" text @click="simplify_subset">Simplify</v-btn>
+        </j-tooltip>
         <v-btn color="primary" text @click="update_subset">Update</v-btn>
       </v-row>
   </j-tray-plugin>

jdaviz/tests/test_subsets.py

@@ -504,6 +504,9 @@ def test_composite_region_from_subset_2d(specviz_helper, spectrum1d):
     assert subset_plugin.subset_types == ['Range', 'Range', 'Range', 'Range']
     assert subset_plugin.glue_state_types == ['AndState', 'AndNotState', 'OrState', 'AndState']
 
+    subset_plugin.vue_simplify_subset()
+    assert subset_plugin.glue_state_types == ["RangeSubsetState", "OrState"]
+
 
 def test_edit_composite_spectral_subset(specviz_helper, spectrum1d):
     specviz_helper.load_spectrum(spectrum1d)
@@ -550,3 +553,23 @@ def test_edit_composite_spectral_subset(specviz_helper, spectrum1d):
     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")
+
+
+def test_edit_composite_spectral_with_xor(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(6400, 6600))
+    specviz_helper.app.session.edit_subset_mode.mode = OrMode
+    viewer.apply_roi(XRangeROI(7200, 7400))
+
+    viewer.apply_roi(XRangeROI(7600, 7800))
+
+    specviz_helper.app.session.edit_subset_mode.mode = XorMode
+    viewer.apply_roi(XRangeROI(6700, 7700))
+    reg = specviz_helper.app.get_subsets("Subset 1")
+
+    assert reg[0].lower.value == 6400 and reg[0].upper.value == 6600
+    assert reg[1].lower.value == 6700 and reg[1].upper.value == 7200
+    assert reg[2].lower.value == 7400 and reg[2].upper.value == 7600
+    assert reg[3].lower.value == 7700 and reg[3].upper.value == 7800