MAINT: standardize spatial query parameter name to 'coordinates'#3609
MAINT: standardize spatial query parameter name to 'coordinates'#3609keflavich wants to merge 4 commits into
Conversation
Part of the API consolidation tracked in astropy#2803: the first argument of the query_region/query_object/cone_search family is now uniformly named 'coordinates'. The old names ('coordinate' in alma, esa.euclid, esa.hsa, esa.jwst, gaia, ipac.irsa.ibe, noirlab; 'position' in esasky and heasarc; 'coord' in ogle) keep working via deprecated_renamed_argument shims (since 0.4.12) and now emit AstropyDeprecationWarning. In heasarc, the renamed parameter shadowed the 'from astropy import coordinates' module import, so that import was narrowed to the names actually used. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3609 +/- ##
==========================================
+ Coverage 73.14% 73.62% +0.47%
==========================================
Files 226 227 +1
Lines 21060 21212 +152
==========================================
+ Hits 15405 15618 +213
+ Misses 5655 5594 -61 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hmm, that is quite a few prominent modules, I would say if we do this it should have a very long deprecation period. OTOH, we should also make sure if we change the parameter to plural then all these modules indeed work with a vector SkyCoord object. Anyways; I'm milestoning it as "API clenaup" -- I would think that could be a release where we also break the versioning scheme and also do other API cleanups, so we break things only once. |
|
Good idea. I can mark it all deprecated and check whether they take vectors. Should we formalize the API such that any services that don't take a vector use |
Audit of all renamed methods per review feedback: the parameter should be plural 'coordinates' only where the method genuinely accepts multiple positions, singular 'coordinate' otherwise. Of the renamed modules, only ogle supports multiple positions (a list of coordinates produces a valid multi-line query), so its rename to 'coordinates' stays. heasarc and esasky are single-position cone/box searches, so their 'position' keyword is now deprecated in favor of singular 'coordinate' instead. The seven modules that already used 'coordinate' for single-position queries (alma, esa.euclid, esa.hsa, esa.jwst, gaia, ipac.irsa.ibe, noirlab) were named correctly all along and are reverted to their original state. All deprecations use deprecated_renamed_argument (since 0.4.12), so the old keywords keep working with a warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
I think the astroquery API should always take the vectors and if there is no server side support we should just loop it in the module. |
|
OK, I'm happy enough to do that. I think we've avoided it in some cases to avoid spamming the services. Should we impose a default (local) rate limit? |
In those cases we could/should reach out and ask about their preferences. The spamming was prblematic when the service were supporting vector queries yet we did it in a loop of signletons |
|
Yeah.... we can reach out, but for first pass, if we add a looped-singleton implementation where before there was none, I'd like to impose a default maximum number of parallel requests or a minimum time between requests. Are you ok with me doing that arbitrarily? It would be nice to clean up the API in a uniform sweep and handle archive detailed preferences later. |
This reverts commit 930d67c.
Resolution of the singular/plural question for the 'coordinates' rename: instead of renaming scalar-only services back to singular, give them real multi-coordinate support. The new astroquery.utils.multicoord.support_multiple_coordinates decorator lets a single-position query method accept a list of coordinates or a non-scalar SkyCoord by running one query per position. Table results are stacked with a query_index column mapping rows to inputs; TableList results merge per key; anything else returns as a list in input order. Requests go through a bounded worker pool with a minimum spacing between submissions. A survey of the affected archives (ESA/ESAC, ALMA, IRSA, HEASARC, NOIRLab) found no published request-rate limits, so the defaults are set conservatively (3 parallel requests, >=0.3 s apart, i.e. ~3 requests/s aggregate) -- below every published limit elsewhere (e.g. MAST's 5 req/s, SIMBAD's 6-10 q/s ban threshold) and below the parallelism the archives' own tooling uses. Both knobs are configurable via astroquery.utils.multicoord.conf. Applied to gaia, esa.euclid, alma, esa.hsa, esa.jwst, esasky, heasarc, ipac.irsa.ibe, and noirlab. ogle already supported lists natively. For alma, an explicit sync query_region now exists (async_to_sync skips it), which also fixes in-place mutation of a user-supplied payload dict in repeated calls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
(this is an experiment @bsipocz - take with a heavy grain of salt, I tried out new claude fable this morning. My first-pass review says this is reasonable: it's actually pretty limited in scope, just changing coordinate->coordinates for the most part.)
Part of the API consolidation tracked in #2803: the first argument of the query_region/query_object/cone_search family is now uniformly named 'coordinates'. The old names ('coordinate' in alma, esa.euclid, esa.hsa, esa.jwst, gaia, ipac.irsa.ibe, noirlab; 'position' in esasky and heasarc; 'coord' in ogle) keep working via
deprecated_renamed_argument shims (since 0.4.12) and now emit AstropyDeprecationWarning.
In heasarc, the renamed parameter shadowed the 'from astropy import coordinates' module import, so that import was narrowed to the names actually used.