Proposed changes for PR 2179 (ENH: Image rotation via WCS-only layers) (

#4)

* Lim edits

* Improve verbiage in docstring

Co-authored-by: Brett M. Morris <[email protected]>

* Address review comments from bmorris3

* Add multi WCS-only test back

* Update concept notebook

* corrections to the pr to the pr to the pr (#10)

* corrections to the pr to the pr to the pr

* using app-level attr

* Update jdaviz/configs/imviz/plugins/parsers.py

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

* Update jdaviz/configs/imviz/plugins/parsers.py

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

---------

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

* Follow up to PR to my PR to your PR

* WCS-only must be loaded
into viewer but not visible.

Fix concept notebook

---------

Co-authored-by: Brett M. Morris <[email protected]>

jdaviz/app.py

@@ -307,6 +307,12 @@ def __init__(self, configuration=None, *args, **kwargs):
         # data loading
         self.auto_link = kwargs.pop('auto_link', True)
 
+        # Imviz linking
+        self._wcs_only_label = "_WCS_ONLY"
+        if self.config == "imviz":
+            self._link_type = None
+            self._wcs_use_affine = None
+
         # Subscribe to messages indicating that a new viewer needs to be
         #  created. When received, information is passed to the application
         #  handler to generate the appropriate viewer instance.
@@ -468,7 +474,7 @@ def _color_to_level(color):
     def _on_layers_changed(self, msg):
         if hasattr(msg, 'data'):
             layer_name = msg.data.label
-            is_wcs_only = msg.data.meta.get('WCS-ONLY', False)
+            is_wcs_only = msg.data.meta.get(self._wcs_only_label, False)
             is_ref_data = getattr(msg._viewer.state.reference_data, 'label', '') == layer_name
         elif hasattr(msg, 'subset'):
             layer_name = msg.subset.label
@@ -494,8 +500,8 @@ def _on_layers_changed(self, msg):
                 }
 
     def _on_refdata_changed(self, msg):
-        old_is_wcs_only = msg.old.meta.get('WCS-ONLY', False)
-        new_is_wcs_only = msg.new.meta.get('WCS-ONLY', False)
+        old_is_wcs_only = msg.old.meta.get(self._wcs_only_label, False)
+        new_is_wcs_only = msg.data.meta.get(self._wcs_only_label, False)
 
         wcs_only_refdata_icon = 'mdi-compass-outline'
         wcs_only_not_refdata_icon = 'mdi-compass-off-outline'
@@ -511,7 +517,7 @@ def switch_icon(old_icon, new_icon):
                 new_layer_icons[layer_name] = switch_icon(
                     layer_icon, wcs_only_not_refdata_icon
                 )
-            elif layer_name == msg.new.label and new_is_wcs_only:
+            elif layer_name == msg.data.label and new_is_wcs_only:
                 new_layer_icons[layer_name] = switch_icon(
                     layer_icon, wcs_only_refdata_icon
                 )
@@ -525,35 +531,32 @@ def _change_reference_data(self, new_refdata_label):
         Change reference data to Data with ``data_label``
         """
         if self.config != 'imviz':
-            # this method is only meant for imviz for now
+            # this method is only meant for Imviz for now
             return
 
-        viewer_reference = self._get_first_viewer_reference_name()
-        viewer = self.get_viewer(viewer_reference)
-        old_refdata = self._viewer_store[viewer_reference].state.reference_data
+        viewer_id = f'{self.config}-0'  # Same as the ID in imviz.destroy_viewer()
+        viewer = self._jdaviz_helper.default_viewer
+        old_refdata = viewer.state.reference_data
 
         if new_refdata_label == old_refdata.label:
             # if there's no refdata change, don't do anything:
             return
 
-        [new_refdata] = [
-            data for data in self.data_collection
-            if data.label == new_refdata_label
-        ]
-
-        change_refdata_message = ChangeRefDataMessage(
+        new_refdata = self.data_collection[new_refdata_label]
+        viewer.state.reference_data = new_refdata
+        self.hub.broadcast(ChangeRefDataMessage(
             new_refdata,
             viewer,
-            viewer_id=viewer_reference,
-            sender=self,
+            viewer_id=viewer_id,
             old=old_refdata,
-            new=new_refdata
-        )
+            sender=self))
 
-        self._viewer_store[viewer_reference].state.reference_data = new_refdata
-        self.hub.broadcast(change_refdata_message)
+        # Re-link
+        self._jdaviz_helper.link_data(link_type=self._link_type,
+                                      wcs_use_affine=self._wcs_use_affine,
+                                      error_on_fail=True)
 
-        self._viewer_store[viewer_reference].state.reset_limits()
+        viewer.state.reset_limits()
 
     def _link_new_data(self, reference_data=None, data_to_be_linked=None):
         """
@@ -1947,10 +1950,10 @@ def set_data_visibility(self, viewer_reference, data_label, visible=True, replac
                 if id != data_id:
                     selected_items[id] = 'hidden'
 
-        # remove wcs-only data from selected items,
+        # remove WCS-only data from selected items,
         # add to wcs_only_layers:
         for layer in viewer.layers:
-            is_wcs_only = getattr(layer.layer, 'meta', {}).get('WCS-ONLY', False)
+            is_wcs_only = getattr(layer.layer, 'meta', {}).get(self._wcs_only_label, False)
             if layer.layer.data.label == data_label and is_wcs_only:
                 layer.visible = False
                 viewer.state.wcs_only_layers.append(data_label)
@@ -2073,7 +2076,7 @@ def _on_data_deleted(self, msg):
     def _create_data_item(self, data):
         ndims = len(data.shape)
         wcsaxes = data.meta.get('WCSAXES', None)
-        wcs_only = data.meta.get('WCS-ONLY', False)
+        wcs_only = data.meta.get(self._wcs_only_label, False)
         if wcsaxes is None:
             # then we'll need to determine type another way, we want to avoid
             # this when we can though since its not as cheap

jdaviz/configs/default/plugins/metadata_viewer/tests/test_metadata_viewer.py

@@ -37,7 +37,7 @@ def test_view_dict(imviz_helper):
     assert mv.has_metadata
     assert mv.metadata == [
         ('BAR', '10.0', ''), ('BOZO', 'None', ''), ('EXTNAME', 'SCI', ''),
-        ('EXTVER', '1', ''), ('FOO', '', ''), ('WCS-ONLY', 'False', '')]
+        ('EXTVER', '1', ''), ('FOO', '', '')]
 
     mv.dataset_selected = 'has_nested_meta[DATA]'
     assert not mv.has_primary
@@ -46,9 +46,7 @@ def test_view_dict(imviz_helper):
     assert mv.has_metadata
     assert mv.metadata == [
         ('EXTNAME', 'ASDF', ''), ('REF.bar', '10.0', ''),
-        ('REF.foo.1', '', ''), ('REF.foo.2.0', '1', ''), ('REF.foo.2.1', '2', ''),
-        ('WCS-ONLY', 'False', '')
-    ]
+        ('REF.foo.1', '', ''), ('REF.foo.2.0', '1', ''), ('REF.foo.2.1', '2', '')]
 
     mv.dataset_selected = 'has_primary[DATA,1]'
     assert mv.has_primary

jdaviz/configs/default/plugins/viewers.py

@@ -182,7 +182,7 @@ def _get_layer_info(layer):
         for layer in self.state.layers[::-1]:
             layer_is_wcs_only = (
                     hasattr(layer.layer, 'meta') and
-                    layer.layer.meta.get('WCS-ONLY', False)
+                    layer.layer.meta.get(self.jdaviz_app._wcs_only_label, False)
             )
             if layer.visible and not layer_is_wcs_only:
                 prefix_icon, suffix = _get_layer_info(layer)

jdaviz/configs/imviz/helper.py

@@ -19,11 +19,6 @@ class Imviz(ImageConfigHelper):
     _default_configuration = 'imviz'
     _default_viewer_reference_name = "image-viewer"
 
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self.app._link_type = None
-        self.app._wcs_use_affine = None
-
     def create_image_viewer(self, viewer_name=None):
         """Create a new image viewer.
 
@@ -174,11 +169,22 @@ def load_data(self, data, data_label=None, show_in_viewer=True, **kwargs):
 
         # find the current label(s) - TODO: replace this by calling default label functionality
         # above instead of having to refind it
-        applied_labels = [label for label in self.app.data_collection.labels if label not in prev_data_labels]  # noqa
+        applied_labels = []
+        applied_visible = []
+        for data in self.app.data_collection:
+            label = data.label
+            if label not in prev_data_labels:
+                applied_labels.append(label)
+                if not data.meta.get(self.app._wcs_only_label, False):
+                    applied_visible.append(True)
+                else:
+                    applied_visible.append(False)
 
         if show_in_viewer is True:
             show_in_viewer = f"{self.app.config}-0"
 
+        # NOTE: We will never try to batch load WCS-only, but if we do, add extra logic
+        #       in batch_load within core/helpers.py module.
         if self._in_batch_load and show_in_viewer:
             for applied_label in applied_labels:
                 self._delayed_show_in_viewer_labels[applied_label] = show_in_viewer
@@ -192,8 +198,8 @@ def load_data(self, data, data_label=None, show_in_viewer=True, **kwargs):
             # NOTE: If the batch_load context manager was used, it will
             # handle that logic instead.
             if show_in_viewer:
-                for applied_label in applied_labels:
-                    self.app.add_data_to_viewer(show_in_viewer, applied_label)
+                for applied_label, visible in zip(applied_labels, applied_visible):
+                    self.app.add_data_to_viewer(show_in_viewer, applied_label, visible=visible)
 
     def link_data(self, **kwargs):
         """(Re)link loaded data in Imviz with the desired link type.
@@ -349,12 +355,14 @@ def layer_is_2d(layer):
     return isinstance(layer, BaseData) and layer.ndim == 2
 
 
+# NOTE: Sync with app._wcs_only_label as needed.
 def layer_is_image_data(layer):
-    return layer_is_2d(layer) and not layer.meta.get('WCS-ONLY', False)
+    return layer_is_2d(layer) and not layer.meta.get("_WCS_ONLY", False)
 
 
+# NOTE: Sync with app._wcs_only_label as needed.
 def layer_is_wcs_only(layer):
-    return layer_is_2d(layer) and layer.meta.get('WCS-ONLY', False)
+    return layer_is_2d(layer) and layer.meta.get("_WCS_ONLY", False)
 
 
 def layer_is_table_data(layer):
@@ -374,9 +382,7 @@ def get_reference_image_data(app):
     """
     Return the reference data in the first image viewer and its index
     """
-    viewer_reference = app._get_first_viewer_reference_name(require_image_viewer=True)
-    viewer = app.get_viewer(viewer_reference)
-    refdata = viewer.state.reference_data
+    refdata = app._jdaviz_helper.default_viewer.state.reference_data
 
     if refdata is not None:
         iref = app.data_collection.index(refdata)
@@ -458,15 +464,17 @@ def link_image_data(app, link_type='pixels', wcs_fallback_scheme='pixels', wcs_u
     else:
         link_plugin = None
 
+    data_already_linked = []
     if link_type == app._link_type and wcs_use_affine == app._wcs_use_affine:
-        data_already_linked = [link.data2 for link in app.data_collection.external_links]
+        for link in app.data_collection.external_links:
+            if link.data1.label != app._wcs_only_label:
+                data_already_linked.append(link.data2)
     else:
         for viewer in app._viewer_store.values():
             if len(viewer._marktags):
                 raise ValueError(f"cannot change link_type (from '{app._link_type}' to "
                                  f"'{link_type}') when markers are present. "
                                  f" Clear markers with viewer.reset_markers() first")
-        data_already_linked = []
 
     refdata, iref = get_reference_image_data(app)
     links_list = []
@@ -487,6 +495,7 @@ def link_image_data(app, link_type='pixels', wcs_fallback_scheme='pixels', wcs_u
             continue
 
         ids1 = data.pixel_component_ids
+        new_links = []
         try:
             if link_type == 'pixels':
                 new_links = [LinkSame(ids0[i], ids1[i]) for i in ndim_range]

jdaviz/configs/imviz/plugins/parsers.py

@@ -146,7 +146,10 @@ def get_image_data_iterator(app, file_obj, data_label, ext=None):
         data_iter = _hdu_to_glue_data(file_obj, data_label)
 
     elif isinstance(file_obj, NDData):
-        data_iter = _nddata_to_glue_data(file_obj, data_label)
+        if file_obj.meta.get(app._wcs_only_label, False):
+            data_iter = _wcsonly_to_glue_data(file_obj, data_label)
+        else:
+            data_iter = _nddata_to_glue_data(file_obj, data_label)
 
     elif isinstance(file_obj, np.ndarray):
         data_iter = _ndarray_to_glue_data(file_obj, data_label)
@@ -181,7 +184,8 @@ def _parse_image(app, file_obj, data_label, ext=None):
             # for outside_*_bounding_box should also be updated.
             data.coords._orig_bounding_box = data.coords.bounding_box
             data.coords.bounding_box = None
-        data_label = app.return_data_label(data_label, alt_name="image_data")
+        if not data.meta.get(app._wcs_only_label, False):
+            data_label = app.return_data_label(data_label, alt_name="image_data")
         app.add_data(data, data_label)
 
     # Do not run link_image_data here. We do it at the end in Imviz.load_data()
@@ -412,37 +416,17 @@ def _nddata_to_glue_data(ndd, data_label):
     if ndd.data.ndim != 2:
         raise ValueError(f'Imviz cannot load this NDData with ndim={ndd.data.ndim}')
 
-    for attrib, sub_attrib in zip(
-            ['data', 'mask', 'uncertainty'],
-            [None, None, 'array']
-    ):
+    for attrib in ('data', 'mask', 'uncertainty'):
         arr = getattr(ndd, attrib)
         if arr is None:
             continue
-        cur_data = Data()
+        comp_label = attrib.upper()
+        cur_label = f'{data_label}[{comp_label}]'
+        cur_data = Data(label=cur_label)
         cur_data.meta.update(standardize_metadata(ndd.meta))
         if ndd.wcs is not None:
             cur_data.coords = ndd.wcs
         raw_arr = arr
-
-        if sub_attrib is not None:
-            # since NDDataArray.uncertainty may be an object like
-            # StdDevUncertainty, we need to take another attr
-            # like StdDevUncertainty.array:
-            base_arr = getattr(raw_arr, sub_attrib)
-        else:
-            base_arr = raw_arr
-        wcs_only = np.all(np.isnan(base_arr))
-
-        if 'WCS-ONLY' not in cur_data.meta or not cur_data.meta.get('WCS-ONLY'):
-            cur_data.meta.update({'WCS-ONLY': wcs_only})
-
-        cur_label = f'{data_label}'
-        comp_label = attrib.upper()
-        if not wcs_only:
-            cur_label += f'[{comp_label}]'
-        cur_data.label = cur_label
-
         if attrib == 'data':
             bunit = ndd.unit or ''
         elif attrib == 'uncertainty':
@@ -463,3 +447,16 @@ def _ndarray_to_glue_data(arr, data_label):
     component = Component.autotyped(arr)
     data.add_component(component=component, label='DATA')
     yield (data, data_label)
+
+
+# ---- Functions that handle WCS-only data -----
+
+def _wcsonly_to_glue_data(ndd, data_label):
+    """Return Data given NDData containing WCS-only data."""
+    arr = ndd.data
+    data = Data(label=data_label)
+    data.meta.update(standardize_metadata(ndd.meta))
+    data.coords = ndd.wcs
+    component = Component.autotyped(arr, units="")
+    data.add_component(component=component, label="DATA")
+    yield (data, data_label)

jdaviz/configs/imviz/plugins/viewers.py

@@ -1,5 +1,6 @@
 import numpy as np
 
+from glue.core.link_helpers import LinkSame
 import astropy.units as u
 from astropy.wcs.utils import pixel_to_pixel
 from astropy.visualization import ImageNormalize, LinearStretch, PercentileInterval
@@ -280,15 +281,25 @@ def get_link_type(self, data_label):
         if len(self.session.application.data_collection) == 0:
             raise ValueError('No reference data for link look-up')
 
-        # TODO: Brett Morris might want to look at this for
-        # https://github.com/spacetelescope/jdaviz/pull/2179
-        #     ref_label = self.state.reference_data ???
-        #
-        # The original links were created against data_collection[0], not necessarily
-        # against the current viewer reference_data
-        ref_label = self.session.application.data_collection[0].label
-
-        return self.jdaviz_helper.get_link_type(ref_label, data_label)
+        ref_label = self.state.reference_data.label
+        if data_label == ref_label:
+            return 'self'
+
+        link_type = None
+        for elink in self.session.application.data_collection.external_links:
+            elink_labels = (elink.data1.label, elink.data2.label)
+            if (data_label in elink_labels and
+                    (ref_label in elink_labels or ref_label == self.jdaviz_app._wcs_only_label)):
+                if isinstance(elink, LinkSame):  # Assumes WCS link never uses LinkSame
+                    link_type = 'pixels'
+                else:  # If not pixels, must be WCS
+                    link_type = 'wcs'
+                break  # Might have duplicate, just grab first match
+
+        if link_type is None:
+            raise ValueError(f'{data_label} not found in data collection external links')
+
+        return link_type
 
     def _get_center_skycoord(self, data=None):
         if data is None: