Rehome subsets after removing data from viewer. (spacetelescope#2409)

* First crack at rehoming subsets

* allow deleting non-plugin data if not last entry

* Prevent data deletion for mosviz and cubeviz

* Unloadable equivalencies for sci, err, dq

* Changelog

Clarify Changelog

* Hackily add data back temporarily to transfer subset ownership

* Remove defunct visible flag

* Add subset warning to removal tooltip

* update test to include viewer id in remove event

* Update CHANGES.rst

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Fix typo

* Debugging subset rehoming for Imviz, currently seems to flip x and y

* I think this logic will need to go first to add WCS transform before removing original reference

* Now converts all four spatial subsets through WCS link to new parent data

Fix codestyle

Remove debugging prints, add changelog

* Add simple test for this

Fix codestyle in test

* Move subset reparenting logic to data deletion, handle TrueCircularRoi and multiple viewers

Fix codestyle

* Handle converting spectral units if necessary

* Force subset plugin to update when subsets are re-parented

* Split out reparenting into its own function so it can be used with reference data changes

* Refactor reparenting to act on subset_groups rather than viewer layers

Fix codestyle

Resolve changelog conflict

Remove old changelog entry

Remove debugging prints

* Relink if ref data is deleted

* Restore responsiveness (to plot options, etc) for Imviz layers after ref data deletion

* Exclude subsets from layers to potentially reload

* Check Imviz for refdata

* Fix overlapping data name/removal icon

* Add some screenshots and documentation about data deletion

Fix errors

One more codestyle fix

* Add test for data deletion in Specviz

Remove stray print

Add third image for relinking test

* Make Specviz/Imviz display docs more consistent

* Update jdaviz/configs/specviz/tests/test_helper.py

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Consolidate 1D spectrum creation in conftest

* Changelog

Clarify Changelog

Update CHANGES.rst

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Fix typo

* Debugging subset rehoming for Imviz, currently seems to flip x and y

* I think this logic will need to go first to add WCS transform before removing original reference

* Now converts all four spatial subsets through WCS link to new parent data

Fix codestyle

Remove debugging prints, add changelog

* Add simple test for this

Fix codestyle in test

* Move subset reparenting logic to data deletion, handle TrueCircularRoi and multiple viewers

* parent 9732bfe9d374f8f16e94983348d5d1b840eb4682
author Ricky O'Steen <rosteen@stsci.edu> 1697030633 -0400
committer Ricky O'Steen <rosteen@stsci.edu> 1697735725 -0400

Check Imviz for refdata

Fix errors

One more codestyle fix

Remove stray print

Add third image for relinking test

Make Specviz/Imviz display docs more consistent

Update jdaviz/configs/specviz/tests/test_helper.py

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Consolidate 1D spectrum creation in conftest

Apply suggestions for tests

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Also check xmax and ymax

Add more comments, use center() in other test

* Consolidate this with center() and move_to()

Commit suggestion

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Codestyle

* Apply suggestions from code review

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Add missing import

* Handle composite subsets

Codestyle

Add helpful note

Fix specviz case, remove debugging print

* Make sure data is unloaded from all viewers before deleting it

---------

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
Co-authored-by: Duy Nguyen <duytnguyendtn.open@gmail.com>
Co-authored-by: Duy Tuong Nguyen <dtn5ah@virginia.edu>
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

CHANGES.rst

@@ -9,6 +9,8 @@ New Features
 
 - Histogram plot in Plot Options now includes tool to set stretch vmin and vmax. [#2513]
 
+- User can now remove data from the app completely after removing it from viewers. [#2409]
+
 Cubeviz
 ^^^^^^^
 

docs/imviz/displayimages.rst

@@ -20,11 +20,27 @@ Selecting a Data Set
 
 Data can be selected and de-selected in each viewer's data menu, opened by clicking the
 |icon-viewer-data-select| button in the top left of the viewer. Here, you can click a
-checkbox next to the listed data to make the data visible (checked) or invisible (unchecked).
-The datasets available in each viewer are filtered
+checkbox to the left of the listed data to make the data visible (checked) or invisible
+(unchecked). The datasets available in each viewer are filtered
 to include only compatible data, so you may not see all loaded data in the menu for
 every viewer. For example, 1D spectra will not be available in the image viewers.
 
+In addition to selecting and de-selecting data to toggle its visibility in the viewer, you
+can also unload the data from the viewer completely by clicking the ``X`` to the right of the
+data label. Any data that still exists in Imviz but has been unloaded from the viewer
+is listed in a separate section that is hidden by default but can can be expanded by clicking
+on the section header:
+
+.. image:: img/imviz_removed_data.png
+
+This section can be hidden by clicking the section header again. Unloaded data will be available
+to re-load into the viewer (by clicking the ``+`` icon) or remove permanently from the app (by
+clicking the trashcan icon).
+
+.. warning::
+    Deleting the first image that was loaded into Imviz may be slow, as deleting this image
+    requires Imviz to re-link any remaining data together and redefine any existing subsets.
+
 .. _imviz_cursor_info:
 
 Cursor Information

docs/specviz/displaying.rst

@@ -33,13 +33,15 @@ Data can be selected and de-selected in each viewer's data menu, opened by click
 |icon-viewer-data-select| button in the top left of the viewer. Here, you can click a 
 checkbox next to the listed data to make the data visible (checked) or invisible (unchecked).
 
+.. image:: img/data_tab.png
+
 In addition to toggling the visibility of a data layer, the data can be unloaded from a viewer
-by clicking the "x" button on the right.  Data unloaded from the viewer will also be excluded
+by clicking the ``X`` button on the right.  Data unloaded from the viewer will also be excluded
 as options from dataset dropdown menus in the various plugins.  Unloaded data will be available
-to re-load into the viewer or remove permanently from the app from an expandable section in the
-data menu.
+to re-load into the viewer (by clicking the ``+`` icon) or remove permanently from the app (by
+clicking the trashcan icon) from an expandable section in the data menu:
 
-.. image:: img/data_tab.png
+.. image:: img/specviz_remove_data.png
 
 .. _specviz_cursor_info:
 

jdaviz/app.py

@@ -35,11 +35,13 @@
 from glue.core.state_objects import State
 from glue.core.subset import (Subset, RangeSubsetState, RoiSubsetState,
                               CompositeSubsetState, InvertState)
+from glue.core.roi import CircularROI, CircularAnnulusROI, EllipticalROI, RectangularROI
 from glue.core.units import unit_converter
 from glue_astronomy.spectral_coordinates import SpectralCoordinates
 from glue_astronomy.translators.regions import roi_subset_state_to_region
 from glue_jupyter.app import JupyterApplication
 from glue_jupyter.common.toolbar_vuetify import read_icon
+from glue_jupyter.bqplot.common.tools import TrueCircularROI
 from glue_jupyter.state_traitlets_helpers import GlueState
 from glue_jupyter.bqplot.profile import BqplotProfileView
 from ipyvuetify import VuetifyTemplate
@@ -1594,6 +1596,7 @@ def remove_data_from_viewer(self, viewer_reference, data_label):
         viewer.remove_data(data)
         viewer._layers_with_defaults_applied = [layer_info for layer_info in viewer._layers_with_defaults_applied  # noqa
                                                 if layer_info['data_label'] != data.label]  # noqa
+
         remove_data_message = RemoveDataMessage(data, viewer,
                                                 viewer_id=viewer_id,
                                                 sender=self)
@@ -1891,6 +1894,99 @@ def _get_viewer_item(self, ref_or_id):
             viewer_item = self._viewer_item_by_id(ref_or_id)
         return viewer_item
 
+    def _reparent_subsets(self, old_parent, new_parent=None):
+        '''
+        Re-parent subsets that belong to the specified data
+
+        Parameters
+        ----------
+        old_parent : glue.core.Data, str
+            The item from the data collection off of which to move the subset definitions.
+
+        new_parent : glue.core.Data, str
+            The item from the data collection to make the new parent. If None, the first
+            item in the data collection that doesn't match ``old_parent`` will be chosen.
+        '''
+        from astropy.wcs.utils import pixel_to_pixel
+
+        if isinstance(old_parent, str):
+            old_parent = self.data_collection(old_parent)
+
+        if isinstance(new_parent, str):
+            new_parent = self.data_collection(new_parent)
+        elif new_parent is None:
+            for data in self.data_collection:
+                if data is not old_parent:
+                    new_parent = data
+                    break
+
+        # Set subset attributes to match a remaining data collection member, using get_subsets to
+        # get components of composite subsets.
+        for key, subset_list in self.get_subsets(simplify_spectral=False).items():
+            # Get the subset group entry for later. Unfortunately can't just index on label.
+            [subset_group] = [sg for sg in self.data_collection.subset_groups if sg.label == key]
+
+            for subset in subset_list:
+                subset_state = subset['subset_state']
+                # Only reparent if needed
+                if subset_state.attributes[0].parent is old_parent:
+                    for att in ("att", "xatt", "yatt", "x_att", "y_att"):
+                        if hasattr(subset_state, att):
+                            subset_att = getattr(subset_state, att)
+                            data_components = new_parent.components
+                            if subset_att not in data_components:
+                                cid = [c for c in data_components if c.label == subset_att.label][0]
+                                setattr(subset_state, att, cid)
+
+                    # Translate bounds through WCS if needed
+                    if (self.config == "imviz" and
+                            self._jdaviz_helper.plugins["Links Control"].link_type == "WCS"):
+                        # Get the correct link to use for translation
+                        roi = subset_state.roi
+                        if type(roi) in (CircularROI, CircularAnnulusROI,
+                                         EllipticalROI, TrueCircularROI):
+                            old_xc, old_yc = subset_state.center()
+                            # Convert center
+                            x, y = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                  roi.xc, roi.yc)
+                            subset_state.move_to(x, y)
+
+                            for att in ("radius", "inner_radius", "outer_radius",
+                                        "radius_x", "radius_y"):
+                                # Hacky way to get new radii with point on edge of circle
+                                # Do we need to worry about using x for the radius conversion for
+                                # radius_y if there is distortion?
+                                r = getattr(roi, att, None)
+                                if r is not None:
+                                    dummy_x = old_xc + r
+                                    x2, y2 = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                            dummy_x, old_yc)
+                                    new_radius = np.abs(x2 - x)
+                                    setattr(roi, att, new_radius)
+
+                        elif type(roi) is RectangularROI:
+                            x_min, y_min = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                          roi.xmin, roi.ymin)
+                            x_max, y_max = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                          roi.xmax, roi.ymax)
+                            roi.xmin = x_min
+                            roi.xmax = x_max
+                            roi.ymin = y_min
+                            roi.ymax = y_max
+
+                    elif type(subset_group.subset_state) is RangeSubsetState:
+                        range_state = subset_group.subset_state
+                        cur_unit = old_parent.coords.spectral_axis.unit
+                        new_unit = new_parent.coords.spectral_axis.unit
+                        if cur_unit is not new_unit:
+                            range_state.lo, range_state.hi = cur_unit.to(new_unit, [range_state.lo,
+                                                                                    range_state.hi])
+
+            # Force subset plugin to update bounds and such
+            for subset in subset_group.subsets:
+                subset_message = SubsetUpdateMessage(sender=subset)
+                self.hub.broadcast(subset_message)
+
     def vue_destroy_viewer_item(self, cid):
         """
         Callback for when viewer area tabs are destroyed. Finds the viewer item
@@ -2022,7 +2118,42 @@ def set_data_visibility(self, viewer_reference, data_label, visible=True, replac
                 viewer.on_limits_change()  # Trigger compass redraw
 
     def vue_data_item_remove(self, event):
-        self.data_collection.remove(self.data_collection[event['item_name']])
+
+        data_label = event['item_name']
+        data = self.data_collection[data_label]
+        self._reparent_subsets(data)
+
+        # Make sure the data isn't loaded in any viewers
+        for viewer_id in self._viewer_store:
+            self.remove_data_from_viewer(viewer_id, data_label)
+
+        # Imviz has some extra logic below that can be skipped after data removal if we're not
+        # removing the reference data, so we check that here.
+        if self.config == "imviz":
+            imviz_refdata = False
+            ref_data, iref = self._jdaviz_helper.get_ref_data()
+            if data is ref_data:
+                imviz_refdata = True
+
+        self.data_collection.remove(self.data_collection[data_label])
+
+        # If there are two or more datasets left we need to link them back together after removing
+        # the reference data (which would leave 0 external_links).
+        if len(self.data_collection) > 1 and len(self.data_collection.external_links) == 0:
+            if self.config == "imviz" and imviz_refdata:
+                link_type = self._jdaviz_helper.plugins["Links Control"].link_type.selected.lower()
+                self._jdaviz_helper.link_data(link_type=link_type, error_on_fail=True)
+                # Hack to restore responsiveness to imviz layers
+                for viewer_ref in self.get_viewer_reference_names():
+                    viewer = self.get_viewer(viewer_ref)
+                    loaded_layers = [layer.layer.label for layer in viewer.layers if
+                                     "Subset" not in layer.layer.label]
+                    if len(loaded_layers):
+                        self.remove_data_from_viewer(viewer_ref, loaded_layers[-1])
+                        self.add_data_to_viewer(viewer_ref, loaded_layers[-1])
+            else:
+                for i in range(1, len(self.data_collection)):
+                    self._link_new_data(data_to_be_linked=i)
 
     def vue_close_snackbar_message(self, event):
         """

jdaviz/components/tooltip.vue

@@ -66,7 +66,7 @@ const tooltips = {
   'viewer-data-radio': 'Switch visibility to layers associated with this data entry',
   'viewer-data-enable': 'Load data entry into this viewer',
   'viewer-data-disable': 'Disable data within this viewer (will be hidden and unavailable from plugins until re-enabled)',
-  'viewer-data-delete': 'Remove data entry across entire app',
+  'viewer-data-delete': 'Remove data entry across entire app (might affect existing subsets)',
 
   'table-prev': 'Select previous row in table',
   'table-next': 'Select next row in table',

jdaviz/components/viewer_data_select.vue

@@ -17,7 +17,7 @@
         </v-btn>
       </template>
 
-      <v-list style="max-height: 500px; width: 460px; padding-top: 0px" class="overflow-y-auto">
+      <v-list style="max-height: 500px; width: 465px; padding-top: 0px" class="overflow-y-auto">
         <v-row key="title" style="padding-left: 25px; margin-right: 0px; background-color: #E3F2FD">
             <span style="overflow-wrap: anywhere; font-size: 12pt; padding-top: 6px; padding-left: 6px; font-weight: bold; color: black">
               {{viewerTitleCase}}
@@ -50,6 +50,7 @@
             :icon="layer_icons[item.name]"
             :viewer="viewer"
             :multi_select="multi_select"
+            :n_data_entries="nDataEntries"
             @data-item-visibility="$emit('data-item-visibility', $event)"
             @data-item-unload="$emit('data-item-unload', $event)"
             @data-item-remove="$emit('data-item-remove', $event)"
@@ -79,6 +80,7 @@
               :icon="layer_icons[item.name]"
               :viewer="viewer"
               :multi_select="multi_select"
+              :n_data_entries="nDataEntries"
               @data-item-visibility="$emit('data-item-visibility', $event)"
               @data-item-remove="$emit('data-item-remove', $event)"
             ></j-viewer-data-select-item>
@@ -226,6 +228,10 @@ module.exports = {
     extraDataItems() {
       return this.$props.data_items.filter((item) => this.itemIsVisible(item, true))
     },
+    nDataEntries() {
+      // return number of data entries in the entire plugin that were NOT created by a plugin
+      return this.$props.data_items.filter((item) => item.meta.Plugin === undefined).length
+    },
   }
 };
 </script>

jdaviz/components/viewer_data_select_item.vue

@@ -22,8 +22,8 @@
       </j-tooltip>
     </div>
 
-    <j-tooltip :tooltipcontent="'data label: '+item.name" span_style="font-size: 12pt; padding-top: 6px; padding-left: 6px; width: calc(100% - 80px); white-space: nowrap; cursor: default;">
-      <j-layer-viewer-icon span_style="margin-left: 4px; margin-right: 2px" :icon="icon" color="#000000DE"></j-layer-viewer-icon>      
+    <j-tooltip :tooltipcontent="'data label: '+item.name" span_style="font-size: 12pt; padding-top: 6px; padding-left: 4px; padding-right: 16px; width: calc(100% - 80px); white-space: nowrap; cursor: default;">
+      <j-layer-viewer-icon span_style="margin-left: 4px; margin-right: 4px" :icon="icon" color="#000000DE"></j-layer-viewer-icon>
       <div class="text-ellipsis-middle" style="font-weight: 500">
         <span>
           {{itemNamePrefix}}
@@ -34,7 +34,7 @@
       </div>
     </j-tooltip>
 
-    <div v-if="isSelected && isUnloadable" style="right: 2px">
+    <div v-if="isSelected && isUnloadable" style="padding-left: 2px; right: 2px">
       <j-tooltip tipid='viewer-data-disable'>
         <v-btn
           icon
@@ -46,11 +46,11 @@
       </j-tooltip>
     </div>
 
-    <div v-if="isDeletable" style="right: 2px">
+    <div v-if="isDeletable" style="padding-left: 2px; right: 2px">
       <j-tooltip tipid='viewer-data-delete'>
         <v-btn
           icon
-          @click="$emit('data-item-remove', {item_name: item.name})"
+          @click="$emit('data-item-remove', {item_name: item.name, viewer_id: viewer.id})"
         ><v-icon>mdi-delete</v-icon></v-btn>
       </j-tooltip>
     </div>
@@ -60,7 +60,7 @@
 <script>
 
 module.exports = {
-  props: ['item', 'icon', 'multi_select', 'viewer'],
+  props: ['item', 'icon', 'multi_select', 'viewer', 'n_data_entries'],
   methods: {
     selectClicked() {
       prevVisibleState = this.visibleState
@@ -114,14 +114,15 @@ module.exports = {
         // forbid unloading the original reference cube
         // this logic might need to be generalized if supporting custom data labels
         // per-cube or renaming data labels
+        const extension = this.itemNameExtension
         if (this.$props.viewer.reference === 'flux-viewer') {
-          return this.$props.item.name.indexOf('[FLUX]') === -1
+          return ['SCI', 'FLUX'].indexOf(extension) !== -1
         } else if (this.$props.viewer.reference === 'uncert-viewer') {
-          return this.$props.item.name.indexOf('[IVAR]') === -1
+          return ['IVAR', 'ERR'].indexOf(extension) !== -1
         } else if (this.$props.viewer.reference === 'mask-viewer') {
-          return this.$props.item.name.indexOf('[MASK]') === -1
+          return ['MASK', 'DQ'].indexOf(extension) !== -1
         } else if (this.$props.viewer.reference === 'spectrum-viewer') {
-          return this.$props.item.name.indexOf('[FLUX]') === -1          
+          return ['SCI', 'FLUX'].indexOf(extension) !== -1
         }
       } else if (this.$props.viewer.config === 'specviz2d') {
         if (this.$props.viewer.reference === 'spectrum-2d-viewer') {
@@ -133,14 +134,12 @@ module.exports = {
       return true
     },
     isDeletable() {
-      // only allow deleting products from plugins.  We might want to allow some non-plugin
-      // data to also be deleted in the future, but would probably need more advanced logic
-      // to ensure essential data isn't removed that would break the app.
-      if (this.$props.item.meta.Plugin === undefined) {
-        return false
-      }
-      // for any exceptions not above, enable deleting
-      return !this.isSelected
+      isLastDataset = (this.$props.n_data_entries <= 1)
+      notSelected = !this.isSelected
+      isMosviz = this.$props.viewer.config === 'mosviz'
+      isCubeviz = this.$props.viewer.config === 'cubeviz'
+      isPluginData = !(this.$props.item.meta.Plugin === undefined)
+      return notSelected && (isPluginData || (!isLastDataset && !isMosviz && !isCubeviz))
     },
     selectTipId() {
       if (this.multi_select) {

jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py

@@ -31,6 +31,7 @@ def test_data_selection(cubeviz_helper, spectrum1d_cube, tmpdir):
     assert len(fv.layers) == 2
     assert len([layer for layer in fv.layers if layer.visible]) == 2
 
-    app.vue_data_item_remove({'item_name': app.state.data_items[1]['name']})
+    app.vue_data_item_remove({'item_name': app.state.data_items[1]['name'],
+                              'viewer_id': 'cubeviz-0'})
 
     assert len(fv.layers) == 1

jdaviz/configs/imviz/helper.py

@@ -303,6 +303,9 @@ def get_data(self, data_label=None, spatial_subset=None, cls=None):
         """
         return self._get_data(data_label=data_label, spatial_subset=spatial_subset, cls=cls)
 
+    def get_ref_data(self):
+        return get_reference_image_data(self.app)
+
 
 def split_filename_with_fits_ext(filename):
     """Split a ``filename[ext]`` input into filename and FITS extension.