Merge pull request #1644 from pllim/gotta-catch-them-all

BUG: Allow data_label as pos arg

CHANGES.rst

@@ -59,12 +59,20 @@ Bug Fixes
 Cubeviz
 ^^^^^^^
 
+- Calling ``cubeviz.load_data(data, data_label)``, where ``data_label`` is passed in
+  as second positional argument instead of keyword, is now allowed. [#1644]
+
 Imviz
 ^^^^^
 
 - Fixed inaccurate aperture photometry results when aperture photometry is done on
   a non-reference image if images are linked by WCS. [#1524]
 
+- Calling ``imviz.load_data(data, data_label)``, where ``data_label`` is passed in
+  as second positional argument instead of keyword, is now allowed. Previously,
+  this will crash because second positional argument is actually a
+  ``parser_reference`` that is meant for internal use. [#1644]
+
 Mosviz
 ^^^^^^
 

jdaviz/app.py

@@ -475,14 +475,24 @@ def _link_new_data(self, reference_data=None, data_to_be_linked=None):
     def load_data(self, file_obj, parser_reference=None, **kwargs):
         """
         Provided a path to a data file, open and parse the data into the
-        `~glue.core.data_collection.DataCollection` for this session. This also attempts to
-        find WCS links that exist between data components. Extra key word
-        arguments are passed to the parsing functions.
+        `~glue.core.data_collection.DataCollection` for this session.
+
+        For some parsers, this also attempts to find WCS links that exist
+        between data components.
 
         Parameters
         ----------
         file_obj : str or file-like
             File object for the data to be loaded.
+
+        parser_reference : str or `None`
+            The actual data parser to use. It must already be registered
+            to glue's data parser registry. This is mainly for internal use.
+
+        **kwargs : dict
+            Additional keywords to be passed into the parser defined by
+            ``parser_reference``.
+
         """
         self.loading = True
         try:

jdaviz/configs/cubeviz/helper.py

@@ -36,7 +36,7 @@ def _set_spectrum_x_axis(self, msg):
             else:
                 viewer.state.x_att_pixel = ref_data.id["Pixel Axis 2 [x]"]
 
-    def load_data(self, data, **kwargs):
+    def load_data(self, data, data_label=None, **kwargs):
         """
         Load and parse a data cube with Cubeviz.
         (Note that only one cube may be loaded per Cubeviz instance.)
@@ -49,7 +49,8 @@ def load_data(self, data, **kwargs):
         """
         if len(self.app.state.data_items) != 0:
             raise RuntimeError('only one cube may be loaded per Cubeviz instance')
-
+        if data_label:
+            kwargs['data_label'] = data_label
         super().load_data(data, parser_reference="cubeviz-data-parser", **kwargs)
 
     def select_slice(self, slice):

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

@@ -23,10 +23,11 @@ def test_fits_image_hdu_parse(image_cube_hdu_obj, cubeviz_helper):
 
 @pytest.mark.filterwarnings('ignore')
 def test_fits_image_hdu_with_microns(image_cube_hdu_obj_microns, cubeviz_helper):
-    cubeviz_helper.load_data(image_cube_hdu_obj_microns, data_label='has_microns')
+    # Passing in data_label keyword as posarg.
+    cubeviz_helper.load_data(image_cube_hdu_obj_microns, 'has_microns')
 
     assert len(cubeviz_helper.app.data_collection) == 3
-    assert cubeviz_helper.app.data_collection[0].label.endswith('[FLUX]')
+    assert cubeviz_helper.app.data_collection[0].label == 'has_microns[FLUX]'
 
     flux_cube = cubeviz_helper.app.data_collection[0].get_object(Spectrum1D, statistic=None)
     assert flux_cube.spectral_axis.unit == u.um

jdaviz/configs/imviz/helper.py

@@ -66,7 +66,7 @@ def destroy_viewer(self, viewer_id):
             raise ValueError(f"Default viewer '{viewer_id}' cannot be destroyed")
         self.app.vue_destroy_viewer_item(viewer_id)
 
-    def load_data(self, data, parser_reference=None, do_link=True, show_in_viewer=True, **kwargs):
+    def load_data(self, data, data_label=None, do_link=True, show_in_viewer=True, **kwargs):
         """Load data into Imviz.
 
         Parameters
@@ -93,8 +93,11 @@ def load_data(self, data, parser_reference=None, do_link=True, show_in_viewer=Tr
               loading too many slices will cause performance issue,
               so consider using Cubeviz instead.
 
-        parser_reference
-            This is used internally by the app.
+        data_label : str or `None`
+            Data label to go with the given data. If not given, this is
+            automatically determined from filename or randomly generated.
+            The final label shown in Imviz may have additional information
+            appended for clarity.
 
         do_link : bool
             Link the data after parsing. Set this to `False` if you want to
@@ -124,24 +127,25 @@ def load_data(self, data, parser_reference=None, do_link=True, show_in_viewer=Tr
         if isinstance(data, str):
             filelist = data.split(',')
 
-            if len(filelist) > 1 and 'data_label' in kwargs:
+            if len(filelist) > 1 and data_label:
                 raise ValueError('Do not manually overwrite data_label for '
                                  'a list of images')
 
             for data in filelist:
                 kw = deepcopy(kwargs)
-                filepath, ext, data_label = split_filename_with_fits_ext(data)
+                filepath, ext, cur_data_label = split_filename_with_fits_ext(data)
 
                 # This, if valid, will overwrite input.
                 if ext is not None:
                     kw['ext'] = ext
 
                 # This will only overwrite if not provided.
-                if 'data_label' not in kw:
+                if not data_label:
+                    kw['data_label'] = cur_data_label
+                else:
                     kw['data_label'] = data_label
 
-                self.app.load_data(
-                    filepath, parser_reference=parser_reference, **kw)
+                self.app.load_data(filepath, parser_reference='imviz-data-parser', **kw)
 
         elif isinstance(data, np.ndarray) and data.ndim >= 3:
             if data.ndim > 3:
@@ -159,14 +163,15 @@ def load_data(self, data, parser_reference=None, do_link=True, show_in_viewer=Tr
                 kw = deepcopy(kwargs)
 
                 # This will only append index to data label if provided.
-                if 'data_label' in kw:
-                    kw['data_label'] = f'{kwargs["data_label"]}_{i}'
+                if data_label:
+                    kw['data_label'] = f'{data_label}_{i}'
 
-                self.app.load_data(data[i, :, :], parser_reference=parser_reference, **kw)
+                self.app.load_data(data[i, :, :], parser_reference='imviz-data-parser', **kw)
 
         else:
-            self.app.load_data(
-                data, parser_reference=parser_reference, **kwargs)
+            if data_label:
+                kwargs['data_label'] = data_label
+            self.app.load_data(data, parser_reference='imviz-data-parser', **kwargs)
 
         if do_link:
             self.link_data(link_type='pixels', error_on_fail=False)

jdaviz/configs/imviz/tests/test_parser.py

@@ -80,7 +80,8 @@ def test_parse_numpy_array_1d_2d(self, imviz_helper):
         with pytest.raises(ValueError, match='Imviz cannot load this array with ndim=1'):
             parse_data(imviz_helper.app, np.zeros(2))
 
-        imviz_helper.load_data(np.zeros((2, 2)), data_label='some_array', show_in_viewer=False)
+        # Passing in data_label keyword as posarg.
+        imviz_helper.load_data(np.zeros((2, 2)), 'some_array', show_in_viewer=False)
         data = imviz_helper.app.data_collection[0]
         comp = data.get_component('DATA')
         assert data.label == 'some_array'

jdaviz/configs/mosviz/helper.py

@@ -425,7 +425,7 @@ def load_data(self, spectra_1d=None, spectra_2d=None, images=None,
                 warnings.warn(msg)
                 instrument = "nirspec"
             if instrument.lower() == "nirspec":
-                super().load_data(directory, "mosviz-nirspec-directory-parser")
+                super().load_data(directory, parser_reference="mosviz-nirspec-directory-parser")
             elif instrument.lower() == "niriss":
                 self.load_niriss_data(directory)
         elif directory is not None and is_zipfile(str(directory)):

jdaviz/configs/specviz/helper.py

@@ -39,7 +39,7 @@ def __init__(self, *args, **kwargs):
 
     def load_spectrum(self, data, data_label=None, format=None, show_in_viewer=True):
         super().load_data(data,
-                          'specviz-spectrum1d-parser',
+                          parser_reference='specviz-spectrum1d-parser',
                           data_label=data_label,
                           format=format,
                           show_in_viewer=show_in_viewer)

jdaviz/configs/specviz/tests/test_helper.py

@@ -288,3 +288,9 @@ def test_plugin_user_apis(specviz_helper):
         plugin = plugin_api._obj
         for attr in plugin_api._expose:
             assert hasattr(plugin, attr)
+
+
+def test_data_label_as_posarg(specviz_helper, spectrum1d):
+    # Passing in data_label keyword as posarg.
+    specviz_helper.load_data(spectrum1d, 'my_spec')
+    assert specviz_helper.app.data_collection[0].label == 'my_spec'

jdaviz/core/helpers.py

@@ -71,7 +71,9 @@ def _propagate_callback_to_viewers(self, method, msg):
             if hasattr(viewer, method):
                 getattr(viewer, method)(msg)
 
-    def load_data(self, data, parser_reference=None, **kwargs):
+    def load_data(self, data, data_label=None, parser_reference=None, **kwargs):
+        if data_label:
+            kwargs['data_label'] = data_label
         self.app.load_data(data, parser_reference=parser_reference, **kwargs)
 
     @property