From 2f8ef4cec6a7648fc2e628dbd419cf53bce49a3f Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Thu, 13 Oct 2022 08:13:48 -0400 Subject: [PATCH] Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --- CHANGES.rst | 2 +- docs/conf.py | 1 - docs/imviz/plugins.rst | 2 +- .../imviz/plugins/catalogs/catalogs.py | 2 +- jdaviz/configs/imviz/tests/test_catalogs.py | 55 ++++++++++--------- 5 files changed, 31 insertions(+), 31 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 9035aeae4a..a6a430583a 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,7 +12,7 @@ Cubeviz Imviz ^^^^^ -- Catalogs plugin now supports loading a JWST catalog from a local ecsv file. [#1707] +- Catalogs plugin now supports loading a JWST catalog from a local ECSV file. [#1707] Mosviz ^^^^^^ diff --git a/docs/conf.py b/docs/conf.py index b479ed243e..983f1d3f69 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -260,7 +260,6 @@ nitpick_ignore.append((dtype, target)) # Extra intersphinx in addition to what is already in sphinx-astropy -intersphinx_mapping['astropy'] = ('https://docs.astropy.org/en/stable/', None) intersphinx_mapping['glue'] = ('http://docs.glueviz.org/en/stable/', None) intersphinx_mapping['glue_jupyter'] = ('https://glue-jupyter.readthedocs.io/en/stable/', None) intersphinx_mapping['regions'] = ('https://astropy-regions.readthedocs.io/en/stable/', None) diff --git a/docs/imviz/plugins.rst b/docs/imviz/plugins.rst index a55468a8c5..4f09fd8842 100644 --- a/docs/imviz/plugins.rst +++ b/docs/imviz/plugins.rst @@ -223,7 +223,7 @@ catalog dropdown menu. This plugin is still under active development. As a result, the search only uses the SDSS DR17 catalog and works best when you only have a single image loaded in a viewer. -To load a catalog from a supported `JWST ecsv catalog file `_, choose "From File...". +To load a catalog from a supported `JWST ECSV catalog file `_, choose "From File...". The file must be able to be parsed by `astropy.table.Table.read` and contain a column labeled 'sky_centroid'. Clicking :guilabel:`SEARCH` will show markers for any entry within the filtered zoom window. diff --git a/jdaviz/configs/imviz/plugins/catalogs/catalogs.py b/jdaviz/configs/imviz/plugins/catalogs/catalogs.py index bb553c34da..f37c821d2d 100644 --- a/jdaviz/configs/imviz/plugins/catalogs/catalogs.py +++ b/jdaviz/configs/imviz/plugins/catalogs/catalogs.py @@ -65,7 +65,7 @@ def _on_file_path_changed(self, event): self.from_file_message = 'Could not parse file with astropy.table.QTable.read' return - if 'sky_centroid' not in table.keys(): + if 'sky_centroid' not in table.colnames: self.from_file_message = 'Table does not contain required sky_centroid column' return diff --git a/jdaviz/configs/imviz/tests/test_catalogs.py b/jdaviz/configs/imviz/tests/test_catalogs.py index 815319afd7..3ca1a157e3 100644 --- a/jdaviz/configs/imviz/tests/test_catalogs.py +++ b/jdaviz/configs/imviz/tests/test_catalogs.py @@ -118,30 +118,31 @@ def test_plugin_image_with_result(self, imviz_helper, tmp_path): assert catalogs_plugin.results_available assert catalogs_plugin.number_of_results == 2473 - def test_from_file_parsing(self, imviz_helper, tmp_path): - catalogs_plugin = imviz_helper.app.get_tray_item_from_name('imviz-catalogs') - - # _on_file_path_changed is fired when changing the selection in the file dialog - catalogs_plugin._on_file_path_changed({'new': './invalid_path'}) - assert catalogs_plugin.from_file_message == 'File path does not exist' - - # observe('from_file') is fired when setting from_file from the API (or after clicking - # select in the file dialog) - with pytest.raises(ValueError, match='./invalid_path does not exist'): - catalogs_plugin.from_file = './invalid_path' - - # setting to a blank string from the API resets the catalog selection to the - # default/first entry - catalogs_plugin.from_file = '' - assert catalogs_plugin.catalog.selected == catalogs_plugin.catalog.choices[0] - - not_table_file = tmp_path / 'not_table.tst' - not_table_file.touch() - catalogs_plugin._on_file_path_changed({'new': not_table_file}) - assert catalogs_plugin.from_file_message == 'Could not parse file with astropy.table.QTable.read' # noqa - - qtable = QTable({'not_sky_centroid': [1, 2, 3]}) - not_valid_table = tmp_path / 'not_valid_table.ecsv' - qtable.write(not_valid_table, overwrite=True) - catalogs_plugin._on_file_path_changed({'new': not_valid_table}) - assert catalogs_plugin.from_file_message == 'Table does not contain required sky_centroid column' # noqa + +def test_from_file_parsing(imviz_helper, tmp_path): + catalogs_plugin = imviz_helper.app.get_tray_item_from_name('imviz-catalogs') + + # _on_file_path_changed is fired when changing the selection in the file dialog + catalogs_plugin._on_file_path_changed({'new': './invalid_path'}) + assert catalogs_plugin.from_file_message == 'File path does not exist' + + # observe('from_file') is fired when setting from_file from the API (or after clicking + # select in the file dialog) + with pytest.raises(ValueError, match='./invalid_path does not exist'): + catalogs_plugin.from_file = './invalid_path' + + # setting to a blank string from the API resets the catalog selection to the + # default/first entry + catalogs_plugin.from_file = '' + assert catalogs_plugin.catalog.selected == catalogs_plugin.catalog.choices[0] + + not_table_file = tmp_path / 'not_table.tst' + not_table_file.touch() + catalogs_plugin._on_file_path_changed({'new': not_table_file}) + assert catalogs_plugin.from_file_message == 'Could not parse file with astropy.table.QTable.read' # noqa + + qtable = QTable({'not_sky_centroid': [1, 2, 3]}) + not_valid_table = tmp_path / 'not_valid_table.ecsv' + qtable.write(not_valid_table, overwrite=True) + catalogs_plugin._on_file_path_changed({'new': not_valid_table}) + assert catalogs_plugin.from_file_message == 'Table does not contain required sky_centroid column' # noqa