From 33473e0b1507355ff91a87884c977b43a2ccce9f Mon Sep 17 00:00:00 2001 From: "Pey Lian Lim (Github)" <2090236+pllim@users.noreply.github.com> Date: Fri, 9 Sep 2022 16:52:11 -0400 Subject: [PATCH 1/2] BUG: Allow data_label as pos arg for Imviz, Cubeviz, and Specviz load_data methods for their helpers. --- CHANGES.rst | 8 +++++ jdaviz/app.py | 18 +++++++++-- jdaviz/configs/cubeviz/helper.py | 5 +-- .../cubeviz/plugins/tests/test_parsers.py | 5 +-- jdaviz/configs/imviz/helper.py | 31 +++++++++++-------- jdaviz/configs/imviz/tests/test_parser.py | 3 +- jdaviz/configs/mosviz/helper.py | 2 +- jdaviz/configs/specviz/helper.py | 2 +- jdaviz/configs/specviz/tests/test_helper.py | 6 ++++ jdaviz/core/helpers.py | 4 ++- 10 files changed, 60 insertions(+), 24 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 83f3b9f7d9..9ba9fdc425 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -57,12 +57,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 ^^^^^^ diff --git a/jdaviz/app.py b/jdaviz/app.py index f2dd0e75c0..50701f4feb 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -475,16 +475,27 @@ 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: try: # Properly form path and check if a valid file @@ -508,6 +519,7 @@ def load_data(self, file_obj, parser_reference=None, **kwargs): data = self.state.settings.get('data', None) if parser_reference: parser = data_parser_registry.members.get(parser_reference) + elif data and isinstance(data, dict): data_parser = data.get('parser', None) if data_parser: diff --git a/jdaviz/configs/cubeviz/helper.py b/jdaviz/configs/cubeviz/helper.py index d857603ed3..a1c1e32ea5 100644 --- a/jdaviz/configs/cubeviz/helper.py +++ b/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): diff --git a/jdaviz/configs/cubeviz/plugins/tests/test_parsers.py b/jdaviz/configs/cubeviz/plugins/tests/test_parsers.py index 0bbb80435b..e65f3583af 100644 --- a/jdaviz/configs/cubeviz/plugins/tests/test_parsers.py +++ b/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 diff --git a/jdaviz/configs/imviz/helper.py b/jdaviz/configs/imviz/helper.py index eb6662d938..f6db46ffca 100644 --- a/jdaviz/configs/imviz/helper.py +++ b/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) diff --git a/jdaviz/configs/imviz/tests/test_parser.py b/jdaviz/configs/imviz/tests/test_parser.py index 539fdc6407..754616d4aa 100644 --- a/jdaviz/configs/imviz/tests/test_parser.py +++ b/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' diff --git a/jdaviz/configs/mosviz/helper.py b/jdaviz/configs/mosviz/helper.py index 5bf1927c88..49c58c10d9 100644 --- a/jdaviz/configs/mosviz/helper.py +++ b/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)): diff --git a/jdaviz/configs/specviz/helper.py b/jdaviz/configs/specviz/helper.py index 46c7bb471d..558eb0eabe 100644 --- a/jdaviz/configs/specviz/helper.py +++ b/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) diff --git a/jdaviz/configs/specviz/tests/test_helper.py b/jdaviz/configs/specviz/tests/test_helper.py index 35c97a0dea..2d762251a9 100644 --- a/jdaviz/configs/specviz/tests/test_helper.py +++ b/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' diff --git a/jdaviz/core/helpers.py b/jdaviz/core/helpers.py index 7045ebba1e..5c7794723d 100644 --- a/jdaviz/core/helpers.py +++ b/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 From 9dd46eadaa899cb5262760d6dbc32aa39a14634c Mon Sep 17 00:00:00 2001 From: "Pey Lian Lim (Github)" <2090236+pllim@users.noreply.github.com> Date: Thu, 15 Sep 2022 13:46:35 -0400 Subject: [PATCH 2/2] Remove accidental blank line additions --- jdaviz/app.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 50701f4feb..8fda698e67 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -495,7 +495,6 @@ def load_data(self, file_obj, parser_reference=None, **kwargs): """ self.loading = True - try: try: # Properly form path and check if a valid file @@ -519,7 +518,6 @@ def load_data(self, file_obj, parser_reference=None, **kwargs): data = self.state.settings.get('data', None) if parser_reference: parser = data_parser_registry.members.get(parser_reference) - elif data and isinstance(data, dict): data_parser = data.get('parser', None) if data_parser: