Add API to rename subsets

CHANGES.rst

@@ -4,6 +4,8 @@
 New Features
 ------------
 
+- Added API for renaming subsets to Subset Tools plugin. [#3356]
+
 Cubeviz
 ^^^^^^^
 
@@ -89,9 +91,9 @@ New Features
 
 - Improve performance while importing multiple regions. [#3321]
 
-* API method to toggle API hints. [#3336]
+- API method to toggle API hints. [#3336]
 
-* Changing flux/SB display units no longer resets viewer zoom levels. [#3335]
+- Changing flux/SB display units no longer resets viewer zoom levels. [#3335]
 
 Cubeviz
 ^^^^^^^

jdaviz/app.py

@@ -41,7 +41,7 @@
 from jdaviz import style_registry
 from jdaviz.core.config import read_configuration, get_configuration
 from jdaviz.core.events import (LoadDataMessage, NewViewerMessage, AddDataMessage,
-                                SnackbarMessage, RemoveDataMessage,
+                                SnackbarMessage, RemoveDataMessage, SubsetRenameMessage,
                                 AddDataToViewerMessage, RemoveDataFromViewerMessage,
                                 ViewerAddedMessage, ViewerRemovedMessage,
                                 ViewerRenamedMessage, ChangeRefDataMessage,
@@ -319,6 +319,9 @@
         self.hub.subscribe(self, PluginPlotAddedMessage,
                            handler=self._on_plugin_plot_added)
 
+        # Convenient reference of all existing subset names
+        self._reserved_labels = set([])
+
         # Parse the yaml configuration file used to compose the front-end UI
         self.load_configuration(configuration)
 
@@ -371,10 +374,14 @@
         # Internal cache so we don't have to keep calling get_object for the same Data.
         # Key should be (data_label, statistic) and value the translated object.
         self._get_object_cache = {}
+
         self.hub.subscribe(self, SubsetUpdateMessage,
                            handler=self._on_subset_update_message)
+        # These both call _on_layers_changed
         self.hub.subscribe(self, SubsetDeleteMessage,
                            handler=self._on_subset_delete_message)
+        self.hub.subscribe(self, SubsetCreateMessage,
+                           handler=self._on_subset_create_message)
 
         # Store for associations between Data entries:
         self._data_associations = self._init_data_associations()
@@ -384,9 +391,6 @@
                            handler=self._on_add_data_message)
         self.hub.subscribe(self, RemoveDataMessage,
                            handler=self._on_layers_changed)
-        self.hub.subscribe(self, SubsetCreateMessage,
-                           handler=self._on_layers_changed)
-        # SubsetDeleteMessage will also call _on_layers_changed via _on_subset_delete_message
 
         # Emit messages when icons are updated
         self.state.add_callback('viewer_icons',
@@ -464,6 +468,13 @@
 
     def _on_subset_delete_message(self, msg):
         self._remove_live_plugin_results(trigger_subset=msg.subset)
+        if msg.subset.label in self._reserved_labels:
+            # This might already be gone in test teardowns
+            self._reserved_labels.remove(msg.subset.label)
+        self._on_layers_changed(msg)
+
+    def _on_subset_create_message(self, msg):
+        self._reserved_labels.add(msg.subset.label)
         self._on_layers_changed(msg)
 
     def _on_plugin_plot_added(self, msg):
@@ -584,6 +595,9 @@
             children_layers = self._get_assoc_data_children(layer_name)
 
         elif hasattr(msg, 'subset'):
+            # We don't need to reprocess the subset for every data collection entry
+            if msg.subset.data != self.data_collection[0]:
+                return
             layer_name = msg.subset.label
             is_wcs_only = False
             is_not_child = True
@@ -1988,6 +2002,99 @@
             viewer_item = self._viewer_item_by_id(ref_or_id)
         return viewer_item
 
+    def _check_valid_subset_label(self, subset_name, raise_if_invalid=True):
+        """Check that `subset_name` is a valid choice for a subset name. This
+        check is run when renaming subsets.
+
+        A valid subset name must not be the name of another subset in the data
+        collection (case insensitive? there cant be a subset 1 and a Subset 1.)
+        The name may match a subset in the data collection if it the current
+        active selection (i.e the renaming is not really a renaming, it is
+        just keeping the old name, which is valid).
+
+        Unlike dataset names, the attempted renaming of a subset to an existing
+        subset label will not append a number (e.g Subset 1 repeated because
+        Subset 1(1)). If the name exists, a warning will be raised and the
+        original subset name will be returned.
+
+        """
+
+        # get active selection, if there is one
+        if self.session.edit_subset_mode.edit_subset == []:
+            subset_selected = None
+        else:
+            subset_selected = self.session.edit_subset_mode.edit_subset[0].label
+
+        # remove the current selection label from the set of labels, because its ok
+        # if the new subset shares the name of the current selection (renaming to current name)
+        if subset_selected in self._reserved_labels:
+            self._reserved_labels.remove(subset_selected)
+
+        # now check `subset_name` against list of non-active current subset labels
+        # and warn and return if it is
+        if subset_name in self._reserved_labels:
+            if raise_if_invalid:
+                raise ValueError("Cannot rename subset to name of an existing subset"
+                                 f" or data item: ({subset_name}).")
+            return False
+
+        elif not subset_name.replace(" ", "").isalnum():
+            if raise_if_invalid:
+                raise ValueError("Subset labels must be purely alphanumeric")
+            return False
+
+        else:
+            split_label = subset_name.split(" ")
+            if split_label[0] == "Subset" and split_label[1].isdigit():
+                if raise_if_invalid:
+                    raise ValueError("The pattern 'Subset N' is reserved for "
+                                     "auto-generated labels")
+                return False
+
+        return True
+
+    def _rename_subset(self, old_label, new_label, subset_group=None, check_valid=True):
+        # Change the label of a subset, making sure it propagates to as many places as it can
+        # I don't think there's an easier way to get subset_group by label, it's just a tuple
+        if subset_group is None:
+            for s in self.data_collection.subset_groups:
+                if s.label == old_label:
+                    subset_group = s
+                    break
+            # If we couldn't find a matching subset group, raise an error
+            else:
+                raise ValueError(f"No subset named {old_label} to rename")
+
+        if check_valid:
+            if self._check_valid_subset_label(new_label):
+                subset_group.label = new_label
+        else:
+            subset_group.label = new_label
+
+        # Update layer icon
+        self.state.layer_icons[new_label] = self.state.layer_icons[old_label]
+        _ = self.state.layer_icons.pop(old_label)
+
+        # Updated extracted spectrum if applicable and update _update_live_plugin_results dict.
+        if self.config == 'cubeviz':
+            for d in self.data_collection:
+                if d.label.split("(")[-1].split(",")[0] == old_label:
+                    old_data_label = d.label
+                    new_data_label = d.label.replace(old_label, new_label)
+                    d.label = new_data_label
+                    self.state.layer_icons[new_data_label] = self.state.layer_icons[old_data_label]
+                    _ = self.state.layer_icons.pop(old_data_label)
+                    results_dict = d.meta['_update_live_plugin_results']
+                    results_dict['aperture'] = new_label
+                    results_dict['add_results']['label'] = new_data_label
+                    d.meta['_update_live_plugin_results'] = results_dict
+
+                    for data_item in self.state.data_items:
+                        if data_item['name'] == old_data_label:
+                            data_item['name'] = new_data_label
+
+        self.hub.broadcast(SubsetRenameMessage(subset_group, old_label, new_label, sender=self))
+
     def _reparent_subsets(self, old_parent, new_parent=None):
         '''
         Re-parent subsets that belong to the specified data
@@ -2380,6 +2487,7 @@
             self._link_new_data()
         data_item = self._create_data_item(msg.data)
         self.state.data_items.append(data_item)
+        self._reserved_labels.add(msg.data.label)
 
     def _clear_object_cache(self, data_label=None):
         if data_label is None:

jdaviz/configs/default/plugins/subset_tools/subset_tools.py

@@ -4,6 +4,7 @@
 
 from astropy.time import Time
 import astropy.units as u
+from functools import cached_property
 from glue.core.message import EditSubsetMessage, SubsetUpdateMessage
 from glue.core.edit_subset_mode import (AndMode, AndNotMode, OrMode,
                                         ReplaceMode, XorMode, NewMode)
@@ -166,8 +167,11 @@
 
     @property
     def user_api(self):
-        expose = ['subset', 'combination_mode', 'recenter_dataset', 'recenter',
-                  'get_center', 'set_center', 'import_region', 'get_regions']
+        expose = ['subset', 'combination_mode',
+                  'recenter_dataset', 'recenter',
+                  'get_center', 'set_center',
+                  'import_region', 'get_regions',
+                  'rename_selected', 'rename_subset']
         return PluginUserApi(self, expose)
 
     def get_regions(self, region_type=None, list_of_subset_labels=None,
@@ -237,10 +241,10 @@
                 subset_data = lyr.layer
                 subset_label = subset_data.label
 
-                # TODO: Remove this when Jdaviz support round-tripping, see
-                # https://github.com/spacetelescope/jdaviz/pull/721
-                if not subset_label.startswith('Subset'):
+                if isinstance(subset_data.subset_state, MaskSubsetState):
+                    # Ignore MaskSubsetState here
                     continue
+
                 try:
                     if self.app.config == "imviz" and to_sky:
                         region = roi_subset_state_to_region(subset_data.subset_state,
@@ -300,16 +304,16 @@
                 self.show_region_info = True
         self._update_combination_mode()
 
-    def _on_subset_update(self, *args):
-        self._sync_selected_from_state(*args)
+    def _on_subset_update(self, msg):
+        self._sync_selected_from_state()
         if 'Create New' in self.subset_selected:
             return
         subsets_avail = [sg.label for sg in self.app.data_collection.subset_groups]
         if self.subset_selected not in subsets_avail:
             # subset selection should re-default after processing the deleted subset,
             # for now we can safely ignore
             return
-        self._get_subset_definition(*args)
+        self._get_subset_definition()
         subset_to_update = self.session.edit_subset_mode.edit_subset[0]
         self.subset._update_subset(subset_to_update, attribute="type")
 
@@ -839,6 +843,42 @@
                 self.subset_definitions[index][i]['value'] = new_value
                 break
 
+    @cached_property
+    def selected_subset_group(self):
+        for subset_group in self.app.data_collection.subset_groups:
+            if subset_group.label == self.subset.selected:
+                return subset_group
+
+    def rename_subset(self, old_label, new_label):
+        """
+        Method to rename an existing subset
+
+        Parameters
+        ----------
+        old_label : str
+            The current label of the subset to be renamed.
+        new_label : str
+            The new label to apply to the selected subset.
+        """
+        self.app._rename_subset(old_label, new_label)
+        self._sync_available_from_state()
+
+    def rename_selected(self, new_label):
+        """
+        Method to rename the subset currently selected in the Subset Tools plugin.
+
+        Parameters
+        ----------
+        new_label : str
+            The new label to apply to the selected subset.
+        """
+        subset_group = self.selected_subset_group
+        if subset_group is None:
+            raise TypeError("current selection is not a subset")
+
+        self.app._rename_subset(self.subset.selected, new_label, subset_group=subset_group)
+        self._sync_available_from_state()
+
     def import_region(self, region, combination_mode=None, max_num_regions=None,
                       refdata_label=None, return_bad_regions=False, **kwargs):
         """

jdaviz/configs/default/plugins/subset_tools/tests/test_subset_tools.py

@@ -315,3 +315,27 @@ def test_get_regions_composite(cubeviz_helper, spectrum1d_cube):
     # make sure the same regions are returned by app.get_subsets
     get_subsets = cubeviz_helper.app.get_subsets()
     assert np.all([get_subsets[ss][0]['region'] == regions[ss] for ss in ss_labels])
+
+
+def test_check_valid_subset_label(imviz_helper):
+
+    # imviz instance with some data
+    data = NDData(np.ones((50, 50)) * u.nJy)
+    imviz_helper.load_data(data)
+
+    st = imviz_helper.plugins["Subset Tools"]
+
+    # apply three subsets, with their default names of `Subset 1`, `Subset 2`, and `Subset 3`
+    st.import_region(CircularROI(20, 20, 10))
+    st.subset = "Create New"
+    st.import_region(CircularROI(25, 25, 10))
+    st.subset = "Create New"
+    st.import_region(CircularROI(30, 30, 10))
+
+    # we should not be able to rename or add a subset named 'Subset 1'.
+    # Make sure this warns and returns accordingly.
+    with pytest.raises(ValueError, match="Cannot rename subset to name of an existing subset"):
+        st.rename_selected("Subset 1")
+
+    with pytest.raises(ValueError, match="The pattern 'Subset N' is reserved"):
+        st.rename_selected("Subset 5")

jdaviz/configs/imviz/plugins/orientation/orientation.py

@@ -8,6 +8,7 @@
 from glue.core.subset import Subset
 from glue.core.subset_group import GroupedSubset
 from glue.plugins.wcs_autolinking.wcs_autolinking import WCSLink, NoAffineApproximation
+from glue.viewers.image.state import ImageSubsetLayerState
 from traitlets import List, Unicode, Bool, Dict, observe
 
 from jdaviz.configs.imviz.wcs_utils import (
@@ -501,8 +502,7 @@ def _on_refdata_change(self, msg):
             # ensure that it is never visible:
             for layer in viewer.state.layers:
                 if (
-                    hasattr(layer.layer, 'label') and
-                    layer.layer.label.startswith("Subset") and
+                    isinstance(layer.layer, ImageSubsetLayerState) and
                     layer.layer.data.meta.get("_WCS_ONLY", False)
                 ):
                     layer.visible = False

jdaviz/core/events.py

@@ -2,7 +2,7 @@
 from glue.core.message import Message
 
 __all__ = ['NewViewerMessage', 'ViewerAddedMessage', 'ViewerRemovedMessage', 'LoadDataMessage',
-           'AddDataMessage', 'SnackbarMessage', 'RemoveDataMessage',
+           'AddDataMessage', 'SnackbarMessage', 'RemoveDataMessage', 'SubsetRenameMessage',
            'AddLineListMessage', 'RowLockMessage',
            'SliceSelectSliceMessage', 'SliceValueUpdatedMessage',
            'SliceToolStateMessage',
@@ -161,6 +161,27 @@
         return self._old
 
 
+class SubsetRenameMessage(Message):
+    def __init__(self, subset_group, old_label, new_label, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        self._subset_group = subset_group
+        self._old_label = old_label
+        self._new_label = new_label
+
+    @property
+    def subset_group(self):
+        return self._subset_group
+
+    @property
+    def old_label(self):
+        return self._old_label
+
+    @property
+    def new_label(self):
+        return self._new_label
+
+
 class SnackbarMessage(Message):
     def __init__(self, text, color=None, timeout=5000, loading=False,
                  *args, **kwargs):

jdaviz/core/helpers.py

@@ -924,11 +924,6 @@ def get_interactive_regions(self):
             subset_data = lyr.layer
             subset_label = subset_data.label
 
-            # TODO: Remove this when Jdaviz support round-tripping, see
-            # https://github.com/spacetelescope/jdaviz/pull/721
-            if not subset_label.startswith('Subset'):
-                continue
-
             try:
                 if self.app.config == "imviz" and to_sky:
                     region = roi_subset_state_to_region(subset_data.subset_state, to_sky=to_sky)