Open keflavich opened 2 months ago
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.
Also, query_sia
does not accept SkyCoord
or Region
input, afaict. It should. Again, that's more a pyvo issue.
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.
(skycoord to SIA2 was solved before for scalar cases, but not for vectors: https://github.com/astropy/pyvo/issues/409 and https://github.com/astropy/pyvo/pull/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)
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)
the gist is linked at the top.
🤦♀️ sorry, I didn't notice..
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?
the narrative docs are good, but the docstring needs to show this too. that's the pyvo issue, though, right?
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.
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.
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?
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.
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.
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.
I'm doing some experimenting with the IRSA module today. https://gist.github.com/keflavich/33140330d1e1695d0d24ffdfdf9934fc
A question came up about the API:
Why is
catalog
an optional keyword argument when it is required? Change was made here: https://github.com/astropy/astroquery/commit/6ab036275c23b0fef1bc7ca9153164202a7c3438I think the
*
should be after thecatalog
keyword. I'll propose that change.query_sia
's documentation has problems inherited from pyvo: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: https://github.com/astropy/pyvo/issues/593
Otherwise, I was pleasantly surprised to discover that SIA works just fine for retrieving spectra