Skip to content

Implement catalog part of AIDA #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jun 9, 2025

Conversation

mwcraig
Copy link
Member

@mwcraig mwcraig commented May 28, 2025

This implements several things related to catalogs:

  1. Change add_markers/get_markers to load_catalog/get_catalog.
  2. Change marker_name to catalog_label
  3. Add set_catalog_style/get_catalog_style
  4. Made it an error to set_catalog_style before the catalog has been loaded.
  5. Made it an error to get_catalog or get_catalog_style without a catalog_label if there is more than one catalog.
  6. Test fairly thoroughly for the ability to set/get without using catalog labels if there is only one catalog.
  7. Made it an error to get_catalog if the provided catalog_label is a list.
  8. Add and test get_catalog_names.

Edit:

@mwcraig mwcraig requested review from Copilot, eteq and pllim May 28, 2025 18:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces deprecated marker methods with a catalog-based API and adds styling and error handling for multiple catalogs.

  • Renames add_markers/get_markers to load_catalog/get_catalog and introduces get_catalog_names
  • Implements set_catalog_style/get_catalog_style with errors when catalogs are not loaded or ambiguous
  • Updates tests to cover new catalog behaviors, style defaults, and error cases

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
widget_api_test.py Renamed marker tests to catalog tests and added new catalog-style tests and error cases
interface_definition.py Updated interface to declare catalog methods and styling, removed marker constants
dummy_viewer.py Implemented in-memory catalog storage, loading, styling, retrieval, and removal
Comments suppressed due to low confidence (2)

src/astro_image_display_api/interface_definition.py:125

  • The catalog_style parameter is typed as str but described as a dictionary in the docstring. Update the annotation to dict or adjust the description to match the intended type.
catalog_style : str, optional

src/astro_image_display_api/interface_definition.py:103

  • The docstring for load_catalog still refers to 'markers'. Update it to describe 'catalogs' to reflect the new API.
Add markers to the image.

@mwcraig mwcraig marked this pull request as draft May 28, 2025 19:22
@pllim
Copy link
Member

pllim commented May 29, 2025

Ginga also has catalog support. Should we also ping Eric J here in case he is interested to review?

@mwcraig
Copy link
Member Author

mwcraig commented May 29, 2025

Sure -- I'm still working out some issues this morning, hope to have this ready for review in a bit.

@mwcraig mwcraig force-pushed the marker-to-catalog-migration branch from 2158bb9 to ee57bd9 Compare May 29, 2025 15:17
@mwcraig mwcraig marked this pull request as ready for review May 29, 2025 15:18
@mwcraig mwcraig requested a review from Copilot May 29, 2025 15:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the existing "markers" API with a more general "catalog" API, adds style management for catalogs, and enforces error conditions around loading and retrieving multiple catalogs.

  • Rename add_markers/get_markers/remove_markers to load_catalog/get_catalog/remove_catalog and introduce get_catalog_names.
  • Introduce set_catalog_style/get_catalog_style and enforce errors on misuse.
  • Update tests to cover single/multiple catalogs, style defaults, error cases, and input immutability.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
src/astro_image_display_api/widget_api_test.py Renamed marker tests to catalog tests, added fixtures and new catalog-style tests
src/astro_image_display_api/interface_definition.py Updated interface: replaced marker methods with catalog methods and their docs
src/astro_image_display_api/dummy_viewer.py Implemented in-memory catalog storage, style handling, load/get/remove logic
Comments suppressed due to low confidence (1)

src/astro_image_display_api/widget_api_test.py:193

  • The test method name has a typo ('caralog') and is missing the test_ prefix, so pytest will not collect it. Rename to test_set_get_catalog_style_preserves_extra_keywords.
def set_get_caralog_style_preserves_extra_keywords(self, catalog):

@mwcraig mwcraig requested a review from Copilot May 29, 2025 21:58
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the existing “marker” API with a more general “catalog” API by renaming methods, adding style support, enforcing error conditions, and expanding test coverage.

  • Renamed add_markers/get_markers/remove_markers to load_catalog/get_catalog/remove_catalog, replaced marker_name with catalog_label, and introduced set_catalog_style/get_catalog_style.
  • Updated ImageViewerInterface signatures, docstrings, and added get_catalog_names.
  • Enhanced ImageViewer dummy implementation to store catalog data and styles, handle WCS coordinate conversions, and added comprehensive tests.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/astro_image_display_api/widget_api_test.py Updated tests to exercise the new catalog API and removed all marker-based tests.
src/astro_image_display_api/interface_definition.py Changed the interface to define load_catalog, get_catalog, remove_catalog, style methods, and get_catalog_names.
src/astro_image_display_api/dummy_viewer.py Implemented catalog storage, style management, WCS logic, and updated remove/get/catalog methods.
Comments suppressed due to low confidence (2)

src/astro_image_display_api/interface_definition.py:185

  • [nitpick] The remove_catalog docstring still mentions markers; update it to reference removing catalogs (e.g., "Remove catalog entries from the viewer").
        Remove markers from the image.

src/astro_image_display_api/interface_definition.py:197

  • [nitpick] The get_catalog docstring still refers to markers; revise it to describe retrieving catalog entries.
        Get the marker positions.

@mwcraig mwcraig force-pushed the marker-to-catalog-migration branch from d584fe1 to 233a247 Compare May 29, 2025 22:11
@mwcraig mwcraig requested a review from Copilot May 30, 2025 13:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the image viewer’s marker API into a catalog-based API and adds functionality for catalog styling. Key changes include renaming and replacing marker-related methods with catalog alternatives, adding error‐checking around catalog style settings and label usage, and updating the dummy viewer implementation to support these behaviors.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/astro_image_display_api/widget_api_test.py Updates tests to reflect the new catalog API and associated error handling.
src/astro_image_display_api/interface_definition.py Alters the interface definition to replace marker methods with catalog methods.
src/astro_image_display_api/dummy_viewer.py Implements catalog loading, styling, and removal; introduces a new internal catalog data structure.
Comments suppressed due to low confidence (2)

src/astro_image_display_api/dummy_viewer.py:373

  • In the get_catalog method, the deletion of the catalog entry removes the data instead of returning it. Remove the deletion so the catalog data is preserved for retrieval.
            del self._catalogs[catalog_label]

src/astro_image_display_api/dummy_viewer.py:334

  • The vstack function is used for concatenating tables but it is not imported in this file. Ensure that vstack is imported from astropy.table to avoid runtime errors.
self._catalogs[catalog_label].data = vstack([old_table, to_add])

@mwcraig
Copy link
Member Author

mwcraig commented May 30, 2025

@pllim @eteq @kecnry @bmorris3 -- this is ready for a look.

@pllim
Copy link
Member

pllim commented May 30, 2025

@ejeschke too?

@mwcraig
Copy link
Member Author

mwcraig commented May 30, 2025

@ejeschke too?

Yes, please!

@mwcraig
Copy link
Member Author

mwcraig commented May 30, 2025

FYI, rebasing to pull in changes from #44...

@ejeschke
Copy link

ejeschke commented Jun 2, 2025

Does this remove the general idea of markers (as unrelated to any specific purpose)? I can think of a few use cases where you want to use markers that are unrelated to catalogs. It seems like you could add catalogs and connect them to markers as orthogonal concepts. Sorry if this is uninformed commentary, I've not been privy to the discussion behind this.

@mwcraig
Copy link
Member Author

mwcraig commented Jun 2, 2025

The word "catalog" here really means "table with positions that are represented by markers on the screen." Backends could (and ideally should) allow interactive marking, assigning it a catalog name of the backend's choosing, which could then be retrieved using the new get_catalog method.

It is completely fine if the "catalog" is not a catalog in the sense that catalog is usually used in astronomy.

This will present a documentation challenge, but am hoping it can be leared up with a few decent examples.

@mwcraig
Copy link
Member Author

mwcraig commented Jun 9, 2025

Gonna yolo-merge this...

@mwcraig mwcraig merged commit 2a2416a into astropy:main Jun 9, 2025
1 check passed
@mwcraig mwcraig deleted the marker-to-catalog-migration branch June 9, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants