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!

CHANGES.rst

docs/imviz/plugins.rst

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?

Thank you for catching that! Honestly, I don’t know what I was thinking when I moved those checks inside the try-except block. You’re absolutely right that they belong outside since they’re not handling exceptions from QTable.read. I’ve fixed it now, and I appreciate your sharp eye for detail!

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

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

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?

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

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).

export_table is ultimately called which will treat the the TableMixin columns as a QTable

jdaviz/jdaviz/core/template_mixin.py

Line 4790 in 48be154

self._qtable.write(filename, overwrite=overwrite)

and write the columns as is, so unless you have a column in our table that is labeled sky_centroid, you wouldn't be able to read it back in after exporting. This is also true if you try exporting Gaia or SDSS.

In the case of 'from file', we already have the sky_centroid column, so we could add the sky_centroid column with the same name to our table, export the table, and then the exported file is able to be reloaded into Imviz.

The other catalogs options could be added in a follow up effort for either writing the sky_centroid column to the loaded catalog table or handle it on export.

I was trying to keep it aligned with the other Search options, but I didn't think of the export functionality then. Thanks for the additional context, all; I will keep the existing sky_centroid column in the table.

codecov · 2025-01-02T18:09:12Z

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.12%. Comparing base (e2faabf) to head (7c01d7a).

Files with missing lines	Patch %	Lines
jdaviz/core/template_mixin.py	75.00%	3 Missing ⚠️
jdaviz/configs/imviz/plugins/catalogs/catalogs.py	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3359      +/-   ##
==========================================
- Coverage   88.12%   88.12%   -0.01%     
==========================================
  Files         127      127              
  Lines       19702    19727      +25     
==========================================
+ Hits        17362    17384      +22     
- Misses       2340     2343       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

jdaviz/configs/imviz/tests/test_catalogs.py

pllim

LGTM. Thanks!

gibsongreen

Everything looks good to me, great work!

… Clear Table functionality (spacetelescope#3359) * Add functionality to load catalogs from a file * Fix table and viewer clearing functionality * Remove redundant 'Clear' button and combine its functionality with 'Clear Table' for intuitive behavior * Codestyle * Add change log * Update tests * Fix failing test * Fix the test, ensuring to first select a row before zooming in. * Update change log * Preserve the unit when serializing * Generate default Object IDs when label column is missing * Update docs related to label column in catalogs * Update tests after merge * Update test * Move changelog to correct milestone * Adjust test assertions * Add test for loading catalogs with additional columns * Update change log * Update based on feedback * Apply suggestions from code review Co-authored-by: P. L. Lim <[email protected]> * Update CHANGES.rst Co-authored-by: P. L. Lim <[email protected]> * Adjust test tolerance --------- Co-authored-by: P. L. Lim <[email protected]>

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 Outdated 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

haticekaratay added 2 commits January 2, 2025 12:33

Update change log

b8ae09e

Update based on feedback

dcb5971

haticekaratay and others added 3 commits January 2, 2025 14:36

Merge branch 'main' into load_catalog_from_file

289f216

Apply suggestions from code review

d692f86

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

Update CHANGES.rst

7c01d7a

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

pllim reviewed Jan 2, 2025

View reviewed changes

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

pllim reviewed Jan 2, 2025

View reviewed changes

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

Adjust test tolerance

974bee4

pllim approved these changes Jan 3, 2025

View reviewed changes

gibsongreen approved these changes Jan 3, 2025

View reviewed changes

pllim merged commit ce20700 into spacetelescope:main Jan 3, 2025
19 checks passed

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

haticekaratay Jan 2, 2025

pllim Dec 27, 2024

pllim Dec 27, 2024

pllim commented Dec 27, 2024

cshanahan1 Dec 27, 2024

pllim Dec 27, 2024

gibsongreen Dec 29, 2024

haticekaratay Jan 2, 2025

codecov bot commented Jan 2, 2025 •

edited

Loading

pllim left a comment

gibsongreen left a comment

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

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

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

pllim commented Dec 27, 2024

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

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

pllim left a comment

Choose a reason for hiding this comment

gibsongreen left a comment

Choose a reason for hiding this comment

haticekaratay commented Dec 16, 2024 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading