Make SDSS query region a circle

[eteq]

This issue was discussed in #545, but we opted to do it as a separate fix so the other work in #545 could get in sooner.

Basically, the SDSS region queries (which have a radius) keyword in fact search a box instead of a circular region on the sky. That needs to be fixed. I think @weaverba137 might be working on this?

[weaverba137]

Yes, however, this is not a simple fix. There are two possible solutions:

1. Do a radial query, using the radial query API, http://skyserver.sdss.org/dr12/en/help/docs/api.aspx#search.
   • Advantage: it's radial!
   • Disadvantage: only one coordinate pair at a time.
2. Do a cross-id query.
   • Advantage: this is also radial, under the hood, and accepts multiple coordinate pairs.
   • Disadvantage(?): there would no longer be any distinction between query_region_async and query_crossid_async.

I don't understand the historical intent of this module, so I'm not sure which to prefer. If we do not need to pay attention to historical intent, then I would be happy to hack up this module beyond all recognition.

[keflavich]

If they're going to end up functionally equivalent, we should probably at least include a 'deprecation warning' and redirect from one (cross-id?) to the other. Otherwise, hacking beyond all recognition (and making it like the other astroquery modules) is a good idea. The history of this module is somewhat murky; it has contributions from a few sources and I did not do a very good job of curating it, preferring partial functionality over conformance to standards. But, @bsipocz may be able to fill in some additional details there.

[bsipocz]

I wouldn't be happy if we depracate multi coordinate queries as of crossid for the sake of a radius query around a single coordinate. I think we would loose much more functionality.

[pllim]

7 years on, @svolfman and I were just being very confused why the search area isn't a circle until she dug out this issue!

xref spacetelescope/jdaviz#1413

[bsipocz]

@pllim - #1088 is a better one with a suggested solution, a PR that does an update with that is more than welcome!

[weaverba137]

I'm interested in working on this, proceeding on the basis of the fGetNearbyObjEq() SQL as suggested in #1088.

However, I have a question about the logic in astroquery.sdss.core.SDSSClass._args_to_payload(). Line 1015 defines q_where = 'WHERE ', and then on line 1050 we find if not q_where:. There is no code in between these two lines that would cause q_where to become undefined, therefore the error messages below line 1050 will never be printed, even if they should be. What is going on there?

[bsipocz]

What is going on there?

most likely is a historical mess. This module is kept afloat with bug patches, but for a very long time (if not from the beginning) could use a revamp, or at least a proper test coverage to smoke out issues like you mentioned.

[bsipocz]

So, please go ahead and open PRs as you see fit, I'm happy to do reviews for this module, but as the code history shows, I don't have time anymore to open PRs myself.

[weaverba137]

I'm going down the path of using query_crossid_async under the hood so that we can do circular searches around multiple coordinates. The user-visible API shouldn't change for now, but fundamentally query_region_async and query_crossid_async will be doing the same thing. We can add deprecations if desired, but initially the changes will focus almost entirely on moving from a square to a circular region.

[bsipocz]

for the long term we should keep query_region_async as that's the name the other modules use, but if it's being totally refactored or switched to use query_sql under the hood, I'm fine with it.