Enhance From File catalog loading to support more columns and improve Clear Table functionality #3359

haticekaratay · 2024-12-16T03:23:04Z

Description

This pull request introduces:
Catalog Loading from File:
- Users can now load catalog data (e.g., .ecsv files) directly into the table.
Improved Clear Table Functionality:
- The "Clear Table" button now clears the table, removes markers, and resets selected rows in the viewer.

Screen.Recording.2024-12-15.at.10.42.00.PM.mov

Fixes #

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

…lear Table' for intuitive behavior

jdaviz/core/template_mixin.py

jdaviz/configs/imviz/tests/test_catalogs.py

CHANGES.rst

kecnry · 2024-12-19T15:17:57Z

We should also probably add a section in user docs to document exactly what format of such file the plugin can actually ingest?

Already exists and probably still relevant: https://jdaviz.readthedocs.io/en/latest/imviz/plugins.html#catalog-search. Any specific changes you think are needed?

pllim · 2024-12-19T15:22:07Z

Re: doc -- OK I was confused by the title of PR and change log. Nvm

jdaviz/configs/imviz/plugins/catalogs/catalogs.py

…load_catalog_from_file

pllim

Some technical comments/questions.

I like the new test you added that uses local data. Thanks!

pllim · 2024-12-27T16:30:10Z

CHANGES.rst

@@ -9,6 +9,7 @@ Cubeviz

 Imviz
 ^^^^^
+- Enhance the Catalog Search plugin to support additional columns when loading catalog data from files. [#3359]


Suggested change

- Enhance the Catalog Search plugin to support additional columns when loading catalog data from files. [#3359]

- Enhance the Catalog Search plugin to support additional columns when loading catalog data from files. [#3359]

pllim · 2024-12-27T16:31:58Z

docs/imviz/plugins.rst

@@ -382,8 +382,8 @@ To load a catalog from a supported `JWST ECSV catalog file <https://jwst-pipelin
 The file must be able to be parsed by `astropy.table.Table.read` and contains the following columns:

 * ``'sky_centroid'``: Column with `~astropy.coordinates.SkyCoord` sky coordinates of the sources.
-* ``'label'``: Column with string identifiers of the sources. If you have numerical identifiers,
-  they will be recast as string.
+* ``'label(optional)'``: Column with string identifiers of the sources. If not provided, unique string identifiers will be generated automatically.


Suggested change

* ``'label(optional)'``: Column with string identifiers of the sources. If not provided, unique string identifiers will be generated automatically.

* ``'label'``: (Optional) Column with string identifiers of the sources.

If not provided, unique string identifiers will be generated automatically.

pllim · 2024-12-27T16:34:41Z

jdaviz/configs/imviz/plugins/catalogs/catalogs.py


-        if 'sky_centroid' not in table.colnames:
-            return 'Table does not contain required sky_centroid column', {}
+            if 'sky_centroid' not in table.colnames:


Why does this have to be within try? This check should never fail if table is successfully read and we already have a blanket except for that. It would only result in a bool.

In fact, I think all the if after .read can be after the try-except block. Unless I missed something here?

pllim · 2024-12-27T16:38:58Z

jdaviz/configs/imviz/plugins/catalogs/catalogs.py

@@ -204,6 +207,11 @@ def search(self, error_on_fail=False):
            # all exceptions when going through the UI should have prevented setting this path
            # but this exceptions might be raised here if setting from_file from the UI
            table = self.catalog.selected_obj
+            column_names = list(table.colnames)


Suggested change

column_names = list(table.colnames)

column_names = table.colnames

I think it is always a list. No need to recast.

https://github.com/astropy/astropy/blob/53b673bb4fd98da7b8972837a9650469bc130b02/astropy/table/table.py#L2218

pllim · 2024-12-27T16:39:31Z

jdaviz/configs/imviz/plugins/catalogs/catalogs.py

@@ -204,6 +207,11 @@ def search(self, error_on_fail=False):
            # all exceptions when going through the UI should have prevented setting this path
            # but this exceptions might be raised here if setting from_file from the UI
            table = self.catalog.selected_obj
+            column_names = list(table.colnames)
+            self.table.headers_avail = self.headers + [
+                col for col in column_names if col not in self.headers and col not in ["sky_centroid", "label"]     # noqa:E501


Suggested change

col for col in column_names if col not in self.headers and col not in ["sky_centroid", "label"] # noqa:E501

col for col in column_names if col not in self.headers and col not in ["sky_centroid", "label"] # noqa: E501

p.s. Maybe also make a note here on why "sky_centroid" and "label" get special treatments here?

p.p.s. If the point of this line is to not have duplicates, I wonder if list(set(self.headers + column_names) - {"sky_centroid", "label"}) is easier. 🤔 (You might have to tweak this because I dunno what is the deal with "sky_centroid" and "label".)

pllim · 2024-12-27T16:54:03Z

jdaviz/configs/imviz/plugins/catalogs/catalogs.py

+                    'Right Ascension (degrees)': row['sky_centroid'].ra.deg,
+                    'Declination (degrees)': row['sky_centroid'].dec.deg,
+                    'Object ID': str(row.get('label', f"{idx + 1}")),
+                    'id': idx+1,


I don't grok why there is "Object ID" and then there is "id" but cleaning that up is out of scope here.

That said, I noticed that "id" here is now idx + 1 but still len(self.table) on L271 above. Should this inconsistency be addressed in this PR or is this also out of scope?

pllim · 2024-12-27T16:54:52Z

jdaviz/configs/imviz/plugins/catalogs/catalogs.py

+                    'y_coord': y_coord,
+                }
+                for col in table.colnames:
+                    if col not in ['label', 'sky_centroid']:  # Skip already processed columns


I see these 2 column names hardcoded multiple times. Should we store them as a hidden instance attribute instead so it can be reused?

pllim · 2024-12-27T16:56:21Z

jdaviz/configs/imviz/plugins/catalogs/catalogs.py

@@ -402,9 +412,13 @@ def vue_do_search(self, *args, **kwargs):
        # calls self.search() which handles all of the searching logic
        self.search()

-    def clear(self, hide_only=True):
+    def clear_table(self, hide_only=True):


I see that clear_table is already exposed via user API before this PR. Is this fixing an API bug? If so, it could also be added to change log.

If not, is this accidentally overwriting the inherited method from one of the mixin?

jdaviz/jdaviz/configs/imviz/plugins/catalogs/catalogs.py

Line 53 in 48be154

return PluginUserApi(self, expose=('clear_table', 'export_table',

jdaviz/jdaviz/core/template_mixin.py

Line 4816 in 48be154

def clear_table(self):

If it is an overwrite but not accidental, is this considered API change that also should go into the change log?

pllim · 2024-12-27T17:10:11Z

Somehow the changes here cause ResourceWarning in a catalog test. Unclosed file somewhere? Should investigate.

FAILED jdaviz/configs/imviz/tests/test_catalogs.py::TestCatalogs::test_plugin_image_with_result
...
ResourceWarning: unclosed <ssl.SSLSocket ...>

cshanahan1 · 2024-12-27T17:26:08Z

jdaviz/configs/imviz/plugins/catalogs/catalogs.py

@@ -271,16 +279,19 @@ def search(self, error_on_fail=False):
            if len(self.app._catalog_source_table) == 1 or self.max_sources == 1:
                x_coordinates = [x_coordinates]
                y_coordinates = [y_coordinates]
+            for idx, (row, x_coord, y_coord) in enumerate(zip(self.app._catalog_source_table, x_coordinates, y_coordinates)):  # noqa:E501
+                row_info = {


the loaded table has to have a 'sky centroid' column, but it is then broken up into ra and dec, which means when its written back out it cant be read back in because it doesn't have 'sky centroid'. would it be possible to add a check when loading if the table already contains a ra and dec column rather than unpacking sky centroid? as it is, a table written out by the app can not be read back in

Does that problem pre-date this PR? Just wondering if round-tripping is in scope here (e.g., writing it out back as sky centroid as a single column in ECSV format).

haticekaratay added 3 commits December 14, 2024 23:09

Add functionality to load catalogs from a file

2f28144

Fix table and viewer clearing functionality

746d8db

Remove redundant 'Clear' button and combine its functionality with 'C…

485846e

…lear Table' for intuitive behavior

haticekaratay added this to the 4.1 milestone Dec 16, 2024

github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Dec 16, 2024

haticekaratay added 5 commits December 16, 2024 22:34

Codestyle

edf93eb

Add change log

73aad20

Update tests

d19de55

Fix failing test

5ece13a

Fix the test, ensuring to first select a row before zooming in.

3fb6df9

haticekaratay marked this pull request as ready for review December 19, 2024 04:12

haticekaratay requested review from rosteen, javerbukh, pllim, kecnry, bmorris3, cshanahan1 and gibsongreen as code owners December 19, 2024 04:12

haticekaratay changed the title ~~WIP: Add functionality to load catalogs from a file~~ Add functionality to load catalogs from a file Dec 19, 2024

pllim reviewed Dec 19, 2024

View reviewed changes

jdaviz/core/template_mixin.py Show resolved Hide resolved

pllim reviewed Dec 19, 2024

View reviewed changes

jdaviz/configs/imviz/tests/test_catalogs.py Outdated Show resolved Hide resolved

kecnry reviewed Dec 19, 2024

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

haticekaratay added 2 commits December 19, 2024 11:11

Update change log

640b615

Preserve the unit when serializing

d7d7439

gibsongreen reviewed Dec 19, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/catalogs/catalogs.py Show resolved Hide resolved

Generate default Object IDs when label column is missing

7799594

haticekaratay added 3 commits December 20, 2024 11:15

Update docs related to label column in catalogs

a96babd

Merge branch 'main' into load_catalog_from_file

9dbbeca

Update tests after merge

b5b4570

rosteen modified the milestones: 4.1, 4.2 Dec 23, 2024

Update test

670cf31

haticekaratay changed the title ~~Add functionality to load catalogs from a file~~ Enhance From File catalog loading to support more columns and improve Clear Table functionality Dec 23, 2024

haticekaratay added 4 commits December 23, 2024 13:42

Merge branch 'main' of https://github.com/spacetelescope/jdaviz into …

c2ba76b

…load_catalog_from_file

Move changelog to correct milestone

0b22f24

Adjust test assertions

04bf999

Add test for loading catalogs with additional columns

b1f2277

pllim reviewed Dec 27, 2024

View reviewed changes

cshanahan1 reviewed Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance From File catalog loading to support more columns and improve Clear Table functionality #3359

Enhance From File catalog loading to support more columns and improve Clear Table functionality #3359

haticekaratay commented Dec 16, 2024 •

edited

Loading

This comment was marked as resolved.

kecnry commented Dec 19, 2024

pllim commented Dec 19, 2024

pllim left a comment

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim commented Dec 27, 2024

cshanahan1 Dec 27, 2024

pllim Dec 27, 2024

	- Enhance the Catalog Search plugin to support additional columns when loading catalog data from files. [#3359]

	- Enhance the Catalog Search plugin to support additional columns when loading catalog data from files. [#3359]

	* ``'label(optional)'``: Column with string identifiers of the sources. If not provided, unique string identifiers will be generated automatically.
	* ``'label'``: (Optional) Column with string identifiers of the sources.
	If not provided, unique string identifiers will be generated automatically.

	column_names = list(table.colnames)
	column_names = table.colnames

	col for col in column_names if col not in self.headers and col not in ["sky_centroid", "label"] # noqa:E501
	col for col in column_names if col not in self.headers and col not in ["sky_centroid", "label"] # noqa: E501

Enhance From File catalog loading to support more columns and improve Clear Table functionality #3359

Are you sure you want to change the base?

Enhance From File catalog loading to support more columns and improve Clear Table functionality #3359

Conversation

haticekaratay commented Dec 16, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

This comment was marked as resolved.

kecnry commented Dec 19, 2024

pllim commented Dec 19, 2024

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haticekaratay commented Dec 16, 2024 •

edited

Loading