Repository: spacetelescope/jdaviz

Issue:
Allow editing of composite subsets in Subset Plugin
**Reporter: Jesse Averbukh**

This is a continuation of the work in https://github.com/spacetelescope/jdaviz/pull/2157

 --- 

 DISCLAIMER: This issue was autocreated by the Jdaviz Issue Creation Bot on behalf of the reporter. If any information is incorrect, please contact [Duy Nguyen](https://github.com/duytnguyendtn)

Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/spacetelescope/jdaviz/pull/2182?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) Report
Patch coverage: **`90.00`**% and no project coverage change.
> Comparison is base [(`733d28d`)](https://app.codecov.io/gh/spacetelescope/jdaviz/commit/733d28d4564ab75928cf175a68f9a6794645bbf5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) 91.71% compared to head [(`d28a7dd`)](https://app.codecov.io/gh/spacetelescope/jdaviz/pull/2182?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) 91.72%.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main    #2182   +/-   ##
=======================================
  Coverage   91.71%   91.72%           
=======================================
  Files         147      147           
  Lines       16191    16196    +5     
=======================================
+ Hits        14850    14856    +6     
+ Misses       1341     1340    -1     
```


| [Impacted Files](https://app.codecov.io/gh/spacetelescope/jdaviz/pull/2182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | |
|---|---|---|
| [...igs/default/plugins/subset\_plugin/subset\_plugin.py](https://app.codecov.io/gh/spacetelescope/jdaviz/pull/2182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-amRhdml6L2NvbmZpZ3MvZGVmYXVsdC9wbHVnaW5zL3N1YnNldF9wbHVnaW4vc3Vic2V0X3BsdWdpbi5weQ==) | `97.20% <88.88%> (+0.08%)` | :arrow_up: |
| [jdaviz/app.py](https://app.codecov.io/gh/spacetelescope/jdaviz/pull/2182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-amRhdml6L2FwcC5weQ==) | `93.03% <100.00%> (+0.10%)` | :arrow_up: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/spacetelescope/jdaviz/pull/2182?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope).

pllim: So, to test this behavior with annulus, install the dev version of glue-astronomy, and then you can do something like this (either with the notebook patch I show here or slap that code on your own use case though you'll have to adjust parameter values as you see fit). For some reason, I need to first select the annulus subset from top-left drop-down for the Subset Tool plugin drop-down to update, but since what I am doing is a bit hacky, let's not worry about that for now.

```diff
--- a/notebooks/concepts/imviz_simple_aper_phot.ipynb
+++ b/notebooks/concepts/imviz_simple_aper_phot.ipynb
@@ -158,6 +158,22 @@
     "imviz.load_regions([my_aper, my_bg])"
    ]
   },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "c6e793a0",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "from glue_astronomy.translators.regions import _annulus_to_subset_state\n",
+    "from regions import CircleAnnulusPixelRegion\n",
+    "\n",
+    "data = imviz.app.data_collection[0]\n",
+    "ann_reg = CircleAnnulusPixelRegion(PixCoord(x=1002, y=1154), inner_radius=22, outer_radius=25)\n",
+    "ann_sbst = _annulus_to_subset_state(ann_reg, data)\n",
+    "imviz.app.data_collection.new_subset_group(subset_state=ann_sbst);"
+   ]
+  },
   {
```

Your edit seems to work for annulus! Though I have to be careful to just modify the radius and the correct one at that.

![Screenshot 2023-05-10 180725](https://github.com/spacetelescope/jdaviz/assets/2090236/5db582bf-341c-47ce-bba2-801061c8a022)

![Screenshot 2023-05-10 180704](https://github.com/spacetelescope/jdaviz/assets/2090236/31d2a894-f09c-4b01-a4e7-0e9c9ed58ea8)

If I am not careful, I can end up with shape like this, though you get what you ask for, so not technically a bug but might also annoy people if they want to use it for photometry because then they have to manually fix the parameters until it is a "true" circular annulus again.

![Screenshot 2023-05-10 181538](https://github.com/spacetelescope/jdaviz/assets/2090236/e0393b54-b2bd-4489-9ef9-a650dac9d2e5)

I can also make the inner radius larger than or equal to the outer radius. Glue does not complain. The subset disappears. And then re-appears when I make the inner radius smaller again.
javerbukh: @pllim I would prefer to not mess with `get_center`, `set_center`, etc. since - afaik - centering composite subsets is a \*checks wiki\* gopher or fox hole. Happy to discuss implementation details and get that into a ticket though!

Also, glad to see annulus editing works out of the box!
pllim: @javerbukh , then you will have to deal with the deprecation warnings when we unpin maxversion of glue. They are already deprecated.
camipacifici: Works very nicely, thank you!
A couple of things I noticed that may or may not be bugs:
- in cubeviz, i select a composite region in the flux viewer, then set the function to 'median', then select a spectral region. the spectral region highlights only the "full cube" collapsed spectrum, but not the collapsed spectrum from the composite spatial region.

- if i do `cubeviz.get_interactive_regions()` i hit a traceback. (in my notebook cubeviz=h)
AttributeError                            Traceback (most recent call last)
Cell In[4], line 1
----> 1 h.get_interactive_regions()

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/jdaviz/core/helpers.py:759, in ImageConfigHelper.get_interactive_regions(self)
    756     continue
    758 try:
--> 759     region = subset_data.data.get_selection_definition(
    760         subset_id=subset_label, format='astropy-regions')
    761 except (NotImplementedError, ValueError):
    762     failed_regs.add(subset_label)

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/glue/core/data.py:381, in BaseData.get_selection_definition(self, subset_id, format, **kwargs)
    377     subset = self.subsets[subset_id]
    379 handler = subset_state_translator.get_handler_for(format)
--> 381 return handler.to_object(subset, **kwargs)

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/glue_astronomy/translators/regions.py:175, in AstropyRegionsHandler.to_object(self, subset)
    172     return PointPixelRegion(PixCoord(*subset_state.get_xy(data, 1, 0)))
    174 elif isinstance(subset_state, AndState):
--> 175     if _is_annulus(subset_state):
    176         return CircleAnnulusPixelRegion(
    177             center=PixCoord(x=subset_state.state1.roi.xc, y=subset_state.state1.roi.yc),
    178             inner_radius=subset_state.state2.state1.roi.radius,
    179             outer_radius=subset_state.state1.roi.radius)
    180     else:

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/glue_astronomy/translators/regions.py:61, in _is_annulus(subset_state)
     56 def _is_annulus(subset_state):
     57     # subset_state.state1 = outer circle
     58     # subset_state.state2 = inner circle
     59     # subset_state.state2 is inverted, so we need its state1
     60     return ((not isinstance(subset_state.state1, InvertState)) and
---> 61             isinstance(subset_state.state1.roi, CircularROI) and
     62             isinstance(subset_state.state2, InvertState) and
     63             isinstance(subset_state.state2.state1.roi, CircularROI) and
     64             (subset_state.state1.roi.xc == subset_state.state2.state1.roi.xc) and
     65             (subset_state.state1.roi.yc == subset_state.state2.state1.roi.yc) and
     66             (subset_state.state1.roi.radius > subset_state.state2.state1.roi.radius))

AttributeError: 'OrState' object has no attribute 'roi'

- one of the regions in the composite region is a rectangle. if i set all xmin, xmax, ymin, ymax to zero and hit update a red banner tells me that i cannot do that, no traceback is issued, but the list of regions in the subset tools disappears. only the menu to select the subset and the `update` button are left.
pllim: > AttributeError: 'OrState' object has no attribute 'roi'

That is my bad. I added that check in https://github.com/glue-viz/glue-astronomy/pull/90 and didn't consider all the different things that can trigger the check. I fixed the logic in https://github.com/glue-viz/glue-astronomy/pull/92 but if you need this particular check fixed sooner, I can do a more targeted PR to glue-astronomy.
camipacifici: No need, thank you for finding the reason.
javerbukh: @camipacifici I haven't touched `get_interactive_regions` throughout this refactor since I'm not sure what use case that would have over `get_susbets`. If `get_subsets` covers all the use cases, can we deprecate that method?

@pllim Are you working for a fix for that bug or should I add more logic to this PR to avoid that traceback?
pllim: I lumped the bug fix into a new feature at https://github.com/glue-viz/glue-astronomy/pull/92 . Do you want me to separate it out?
javerbukh: @camipacifici I see the bug you mentioned about the spectral subset not highlighting the collapsed composite spatial subset in the spectrum viewer. I know @kecnry did some work on that functionality a while ago, is it worth trying to get working in this PR or as a follow up?

`cubeviz.get_interactive_regions()` does not seem to be able to handle composite subsets, so I would suggest using `app.get_subsets` as a replacement.

Setting all values to 0, or making the upper bound less than the lower bound, or setting a negative radius will all cause bugs/tracebacks. I could spend some time putting guardrails up so that users cannot do some of these things but...why would a user do them? Can they just...not? 😄 
kecnry: > I see the bug you mentioned about the spectral subset not highlighting the collapsed composite spatial subset in the spectrum viewer.

I can reproduce this in main.  It works if you create the spectral subset _before_ making the spatial subset a compound subset.  I suspect its a limitation of [this function in the code](https://github.com/spacetelescope/jdaviz/pull/1528/files#diff-87fc9fbc017d7ecd743c7868a9c8603d038a7c431ae52f596287a307ee82442dR201)... I'll investigate and create an issue if its not a trivial fix.  Either way, I think it can be considered out-of-scope for the sake of this PR.
camipacifici: > Can they just...not?

Well yes, but I ended up in that trap because I wanted to delete part of the compound subset and I thought I could just change it to all zeros for that ;)
javerbukh: Unfortunately, there is no way to delete individual subregions in a compound subset. You can go into replace mode and start over if you added a region you did not actually want. If that is a feature users want, it would have to be a separate ticket so we could determine where that can happen and what the UI looks like.
camipacifici: Understood. I agree to have the request for this new functionality in a new ticket.
I still think the tool could get out of the problem (like setting all the properties of the subset to zero) a little more gracefully.

pllim: But... needs a change log under new feature.
javerbukh: Guardrails added and changelog updated, will wait for CI to pass and then I'll merge. Thank you everyone!

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 39434707..958977e9 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 2b99d125..d2d3d9c2 100644
--- a/jdaviz/app.py
+++ b/jdaviz/app.py
@@ -909,8 +909,6 @@ class Application(VuetifyTemplate, HubListener):
             # 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 @@ class Application(VuetifyTemplate, HubListener):
         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 @@ class Application(VuetifyTemplate, HubListener):
                         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 @@ class Application(VuetifyTemplate, HubListener):
                 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 @@ class Application(VuetifyTemplate, HubListener):
                     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 3af7db91..7cae959a 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 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
         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 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
         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 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
         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 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
         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 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
             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 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
 
     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 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):
             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 354c3376..286adbce 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 @@
     </v-row>
 
     <!-- Sub-plugin for recentering of spatial subset (Imviz only) -->
-    <v-row v-if="config=='imviz' && is_editable">
+    <v-row v-if="config=='imviz' && is_centerable">
       <v-expansion-panels accordion v-model="subplugins_opened">
         <v-expansion-panel>
           <v-expansion-panel-header >
@@ -58,13 +58,12 @@
           :label="item.name"
           v-model.number="item.value"
           type="number"
-          :disabled="!is_editable"
         ></v-text-field>
       </v-row>
     </div>
 
       <v-row justify="end" no-gutters>
-        <v-btn color="primary" text @click="update_subset" :disabled="!is_editable">Update</v-btn>
+        <v-btn color="primary" text @click="update_subset">Update</v-btn>
       </v-row>
   </j-tray-plugin>
 </template>


Test patch:
diff --git a/jdaviz/configs/specviz/tests/test_helper.py b/jdaviz/configs/specviz/tests/test_helper.py
index 82543695..12603091 100644
--- a/jdaviz/configs/specviz/tests/test_helper.py
+++ b/jdaviz/configs/specviz/tests/test_helper.py
@@ -170,7 +170,7 @@ class TestSpecvizHelper:
     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 @@ class TestSpecvizHelper:
         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 972231c6..76e8ef3b 100644
--- a/jdaviz/tests/test_subsets.py
+++ b/jdaviz/tests/test_subsets.py
@@ -5,7 +5,7 @@ from astropy.tests.helper import assert_quantity_allclose
 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,22 @@ 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()
+
+    # 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 +503,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")