-
Notifications
You must be signed in to change notification settings - Fork 0
Query Methods for new Catalogs #3
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
Conversation
002f12a to
06e3320
Compare
There was a problem hiding this 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 implements comprehensive query methods for the new MAST Catalogs framework, enabling users to query catalog collections with improved validation, spatial queries, and backwards compatibility features.
Key Changes:
- Implemented
query_criteria,query_region, andquery_objectmethods with support for spatial queries, filtering, sorting, and column selection - Added
DEFAULT_CATALOGSmapping to provide default catalog selections for known collections - Enhanced
_verify_catalogand_verify_collectionmethods with case-insensitive matching, no-prefix lookup, and helpful error messages
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| astroquery/mast/collections.py | Implements the main query methods (query_criteria, query_region, query_object) with ADQL generation, region parsing, criteria formatting, and backwards compatibility handling |
| astroquery/mast/catalog_collection.py | Adds DEFAULT_CATALOGS mapping and updates _verify_catalog to return validated catalog names with improved matching logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @snbianco --- I know the PR isn't completed yet, but I think it would be best to be explicit that spherical sky regions are required for region searches. Also, it might be the case that users provide a spherical Region in a non-ICRS frame. Thus lines 680-687 of collections.py might need checks to ensure / transform the region into the right frame. |
|
Thanks @sprice! Agreed on being explicit — I’ve updated docstrings to clarify the exact types expected when inputting a region. On the frame question: the current implementation should already handle non-ICRS spherical regions correctly. For both |
|
I would still argue we should be extra explicit and say that This would be a chance to consider which type of geometry (spherical vs planar) for a region is truly intended --- but for cone searches I really think everyone would intend it to be "actually on the sky", hence |
|
I see! So this is something that's being added to |
|
Yeah, it's an open PR (promising initial feedback and waiting for review): astropy/regions#618 |
This PR significantly expands the
CatalogCollectionandCatalogsquerying framework to support new MAST catalogs, improve catalog validation, and provide more robust and user-friendly query behaviors.Key Enhancements
query_criteria:region,coordinates, andobjectnamewith cleaner checks.query_regionandquery_coordinatesnow callquery_criteriain their implementation. We are keeping these functions for backwards compatibility.DEFAULT_CATALOGSmapping lists default catalogs for known collections._verify_catalognow:version,page, andpagesizeparameters.panstarrsorgaia), warn them, and then query with the new collection name.catalogparameter. If they are, warn them and use the catalog given as the collection. This is particularly relevant as we are changing the name of the parameter fromcatalogtocollection.