IRSA review based on spectral retrieval

[keflavich]

I'm doing some experimenting with the IRSA module today.
https://gist.github.com/keflavich/33140330d1e1695d0d24ffdfdf9934fc

A question came up about the API:

---------------------------------------------------------------------------
InvalidQueryError                         Traceback (most recent call last)
Cell In[4], line 1
----> 1 Irsa.query_region(SkyCoord.from_name('Sgr B2'))

File [/orange/adamginsburg/miniconda3/envs/python312/lib/python3.12/site-packages/astropy/utils/decorators.py:603](http://localhost:23456/lab/workspaces/auto-n/tree/jwst/brick/notebooks/miniconda3/envs/python312/lib/python3.12/site-packages/astropy/utils/decorators.py#line=602), in deprecated_renamed_argument.<locals>.decorator.<locals>.wrapper(*args, **kwargs)
    600             msg += f"\n        Use {alternative} instead."
    601         warnings.warn(msg, warning_type, stacklevel=2)
--> 603 return function(*args, **kwargs)

File [/red/adamginsburg/repos/astroquery/astroquery/ipac/irsa/core.py:176](http://localhost:23456/red/adamginsburg/repos/astroquery/astroquery/ipac/irsa/core.py#line=175), in IrsaClass.query_region(self, coordinates, catalog, spatial, radius, width, polygon, get_query_payload, columns, verbose, cache)
    137 """
    138 Queries the IRSA TAP server around a coordinate and returns a `~astropy.table.Table` object.
    139 
   (...)
    173 table : A `~astropy.table.Table` object.
    174 """
    175 if catalog is None:
--> 176     raise InvalidQueryError("Catalog name is required!")
    178 adql = f'SELECT {columns} FROM {catalog}'
    180 if spatial == 'All-Sky':

InvalidQueryError: Catalog name is required!

Why is catalog an optional keyword argument when it is required? Change was made here: 6ab0362

I think the * should be after the catalog keyword. I'll propose that change.

query_sia's documentation has problems inherited from pyvo:

Parameters:
possingle or list of tuples
angle units (default: deg) the positional region(s) to be searched for data. Each region can be expressed as a tuple representing a CIRCLE, RANGE or POLYGON as follows: (ra, dec, radius) - for CIRCLE. (angle units - defaults to) (long1, long2, lat1, lat2) - for RANGE (angle units required) (ra, dec, ra, dec, ra, dec … ) ra/dec points for POLYGON all in angle units

Note that "defaults to" isn't followed by anything. By experimentation it appears to be degrees. It is not clear how to specify units for those with 'angle units required'. This is an upstream issue: astropy/pyvo#593

Otherwise, I was pleasantly surprised to discover that SIA works just fine for retrieving spectra

[keflavich]

OK, the answer to the first comment is sort of weird: catalog is required, but the first argument, coordinate, technically is not because of the spatial='All-Sky' possibility. So catalog=None is necessary. But still, the order should change, imo. But this is such a tiny change.... it's hardly worth it.

[keflavich]

Also, query_sia does not accept SkyCoord or Region input, afaict. It should. Again, that's more a pyvo issue.

[bsipocz]

Ahh, interesting, that should work. Can you dump the examples you tried into a gist, so we can eventually add all of those to the test suite either here and/or at pyvo.

[bsipocz]

(skycoord to SIA2 was solved before for scalar cases, but not for vectors: astropy/pyvo#409 and astropy/pyvo#459; and I use skycoords in here: https://caltech-ipac.github.io/irsa-tutorials/tutorials/cloud_access/cloud-access-intro.html#find-an-image-using-pyvo-and-a-coordinate-search)

[keflavich]

the gist is linked at the top.

But thanks, indeed (crd, rad) works, e.g.:

crd = SkyCoord.from_name('Sgr B2')
sgrb2_q = Irsa.query_sia(pos=(crd, 6*u.arcmin,) )

(...don't run that, 6 arcmin is way too much for this target)

[bsipocz]

the gist is linked at the top.

🤦‍♀️ sorry, I didn't notice..

[bsipocz]

For the SkyCoord search, is https://astroquery.readthedocs.io/en/latest/ipac/irsa/irsa.html#simple-image-access-queries enough, or we need more documentation?

[keflavich]

the narrative docs are good, but the docstring needs to show this too. that's the pyvo issue, though, right?

[bsipocz]

OK, the answer to the first comment is sort of weird: catalog is required, but the first argument, coordinate, technically is not because of the spatial='All-Sky' possibility. So catalog=None is necessary. But still, the order should change, imo. But this is such a tiny change.... it's hardly worth it.

I won't change the order of the parameters while allowing them to be positional as that would break the API in a really bad way. However, I would be on board of making coordinates optional and keyword only, and then making catalog mandatory (I suppose some queries may work where the catalog is not provided, but I would say anyone want to do such a non-standard query should write up their own query and usequery_tap directly.

[bsipocz]

Otherwise, I was pleasantly surprised to discover that SIA works just fine for retrieving spectra

Keep in mind that this will change. Currently it works as there is no distinction between SIA and SSA collections.

[bsipocz]

I think the * should be after the catalog keyword. I'll propose that change.

OK, looking at this again: the * only says that catalog has to be a keyword argument. I think the InvalidQueryError made it clear that is required, and the docstring didn't contradict it either as usually it mentions when something is optional.

So my two proposals: make it mandatory keyword argument without the None default. Note that this would raise the exception

TypeError: IrsaClass.query_region() missing 1 required keyword-only argument: 'catalog'

While currently we raise the InvalidQueryError: Catalog name is required! instead.

So, which of these two options would you like better?

[bsipocz]

Note that "defaults to" isn't followed by anything. By experimentation it appears to be degrees.

OK, so we'll need to add extensive testings. The defaults are in degrees by the VO standard, but that doesn't have to be shown on the user level, and we can enforce using actual Quantity/angle objects here.

[keflavich]

I'm happy enough leaving the order of arguments and the exception raising as-is. I was caught a bit off-guard by having a required argument after the '*', but the message is indeed clearer this way.

Agree to enforcing quantity inputs, though. That solves the unspecified unit issue as cleanly as possible.

[bsipocz]

I'm cleaning up the docstrings over at pyvo, I hope it will be cleaner that any floats are in degrees. However, before opening that PR I need to clean up some questions of what other inputs to support. I may tag you on those PRs as a user test particle as everyone else is very much deep-dived into VO standards, so they may not pick up on the non-user-friendly/jargony phrasing of the docstrings.