Restore rectangle search

[weaverba137]

This PR closes #2659. Rectangle searches are passed as a SQL query to query_sql().

• Fix any existing broken tests.
• Enable tests with rectangular search.
• Test calling query_region() with either radius or width; not neither, not both.
• Decide what defaults should be: e.g. what should happen if neither radius nor width are specified?
• Decide on a definition of "rectangle": cos(dec) or not.
• Verify that documentation is clear.
• Change log, etc.

Other notes:

• I rearranged some of the documentation sections, even in functions unrelated to this change, because they did not strictly follow numpydoc standards.
• But it turns out that breaks async_to_sync, so I'll undo that for the purposes of this PR.

[codecov]

Codecov Report

Merging #2663 (dd63c3b) into main (766a4fc) will decrease coverage by 3.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2663      +/-   ##
==========================================
- Coverage   69.17%   65.83%   -3.34%     
==========================================
  Files         304      233      -71     
  Lines       22529    17892    -4637     
==========================================
- Hits        15585    11780    -3805     
+ Misses       6944     6112     -832     

Impacted Files	Coverage Δ
astroquery/sdss/core.py	92.97% <100.00%> (+1.47%)	⬆️

... and 89 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[weaverba137]

The async_to_sync functionality appears to assume that Returns is the last section of the docstring, but this violates the numpydoc standard. I'll eventually get around to restoring the Returns, but it would be more robust for async_to_sync to find and replace the Returns section, rather than just assuming it is at the end.

[keflavich]

A fix to that incorrect assumption would be welcome!

[weaverba137]

@keflavich, good, but I think I will keep that as a separate issue for now.

[weaverba137]

@keflavich, I need some help on this one. I can no longer determine why async_to_sync is not producing compilable doc strings, even after I've restored the doc string sections as closely as possible to the state prior to this PR (as well as the last tag).

[keflavich]

I see https://readthedocs.org/projects/astroquery/builds/19731041/ is the problem.

I'm not sure, but my first guess is that you've changed something like

"""
this
"""

to

"""this
"""

which has broken the dedent code?

I can dig a bit more later if that's not the soln

[keflavich]

That helped, but now it seems to be having problems with lines like: Note that there is a server-side limit of 3 arcmin on `radius`. which is the standard way to refer to arguments to a function. Instead it is being interpreted with the py:obj role.

try double instead of single backticks?

--

Adam

[weaverba137]

@keflavich, et al., I still need to do some updates to the remote data tests, but I think it is time to move this out of draft status and start thinking about the unchecked boxes above:

• What should be the default for query_region() if neither radius nor width are specified? Currently it raises an exception.
• What is the definition of "rectangle". Currently, I implemented an improved version of the original rectangle from the previous tag (now handles RA wrap-around!). It does not currently implement cos(dec) corrections.
• Is the documentation sufficiently clear?

[keflavich]

What should be the default for query_region() if neither radius nor width are specified? Currently it raises an exception.

This is the right behavior

What is the definition of "rectangle". Currently, I implemented an improved version of the original rectangle from the previous tag (now handles RA wrap-around!). It does not currently implement cos(dec) corrections.

The current implementation is fine, but ...

Is the documentation sufficiently clear?

Yes, but include a note that the rectangle coordinates represent (ra +/- width, dec +/- height) as opposed to (ra +/- width/cos(dec), dec +/- height)

Otherwise, looks good. I put in some minor code readability suggestions - if they do not achieve the desired behavior, though, feel free to reject.

[weaverba137]

@keflavich, I've implemented your suggestions and also run remote tests to verify those are working. The final step is updating the documentation.

[weaverba137]

@keflavich, et al., I've added further details to the documentation and the change log. For some reason Codecov is saying the total coverage decreased, even though I increased the coverage of sdss/core.py to 100%. The largest decrease appears to be in cds/core.py, but I don't know why that is.

[keflavich]

I like the clarification text about the shape of the selection. I have two very minor suggestions but I think this is good to merge. I'm not really worried about the coverage decrease because it can't be related, but if anyone else has ideas, voice 'em

[weaverba137]

OK, I've committed the last two suggestions.

[weaverba137]

Reminder: this is waiting for final review.

[keflavich]

@bsipocz I'm good with this, but I'm wary of hitting 'merge' without your eyes on it.

[bsipocz]

All looks good, thanks!