Refactor Skyview module to use pyVO's SIAP

[duytnguyendtn]

Hello Astroquery Maintainers!

This PR offers to refactor the Skyview module, currently webscraping parameters using our Perl script, to instead use the Virtual Observatory's Simple Image Access Protocol via the pyVO package. Feedback and suggestions would be highly welcome!

There were some notable changes that I made that is different from the original Skyview class, including:

1. The reported parameters the Skyview surveys support are much smaller in scope than the full capabilities of the original Astroquery Skyview package. Removed parameters include: the resolver, deedger, lut, grid, and gridlabels arguments. (Commit bd6864c4b4faeb13357607fa82d4f88539043051)
2. The online Perl form only supports specifying the size of the FOV in degrees, rather than through width and height. Because the Astroquery spec states only one is required, I brought the skyview API in line with the Perl script and only provided radius as a parameter, dropping the width/height args. (Commit 9e83dd0a69f8b4e01ac22c3c1f64872943ca753c)
3. ~To make it more explicit, I renamed the survey arg to surveys (with an s) to indicate the user may specify multiple surveys.~ Reverted (see https://github.com/astropy/astroquery/pull/2849#discussion_r1350815065)

Tests have been updated as well, though I did run into an issue with one of the remote tests, where the download is timing out, but there is no way for us to override the timeout from the level of the astroquery module. (If I run the remote tests consecutively (4-5 times), the test passes presumably after some of the files are cached somewhere between my computer and the server). I reported it in #astroquery on slack (copied here for reference):

Hi all, undergoing a similar effort as Brigitta and refactoring the SkyView class to use pyVO's SIA. I'm hitting a timeout error trying to download the FITS files from one of our larger archives to open them with astropy.io.fits. I'm using astroquery.utils.commons.FileContainer.get_fits , but there doesn't seem to be a way to control the timeout. Is the recommended way to handle this the same way astroquery.besancon does and call astroquery.utils.commons.get_readable_fileobj directly to handle the timeout manually, or is there a better way?

This PR is still in draft until I complete the documentation changes. Thank you!

[pep8speaks]

Hello @duytnguyendtn! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2023-10-11 21:27:51 UTC

[codecov[bot]]

Codecov Report

Merging #2849 (2a6135a) into main (8fe1a8b) will decrease coverage by 0.06%. The diff coverage is 90.62%.

@@            Coverage Diff             @@
##             main    #2849      +/-   ##
==========================================
- Coverage   66.51%   66.46%   -0.06%     
==========================================
  Files         235      235              
  Lines       18096    18058      -38     
==========================================
- Hits        12037    12002      -35     
+ Misses       6059     6056       -3     

Files	Coverage Δ
astroquery/skyview/core.py	89.23% <90.62%> (-1.07%)	:arrow_down:

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

[duytnguyendtn]

@bsipocz Thanks for the comments! I think I addressed them and my documentation changes, so this PR should be ready for a full review. Thank you in advance!

Also pinging @zoghbi-a, another HEASARC member who may be interested in taking a look

[duytnguyendtn]

@bsipocz Any sense of when we might be able to get a review on this PR? Thanks!

[duytnguyendtn]

Thanks for the comments @keflavich! Sadly no, this does not fully replace the old API due to the SIAP parameters not being a 1-1 match to the ones available via the online perl form. As per https://github.com/astropy/astroquery/pull/2849#discussion_r1350816301, I tried to properly deprecate them on the code side and explicitly list the supported parameters in the documentation in https://github.com/astropy/astroquery/pull/2849/commits/2a6135a3d410ff6c023b793f4b1e4444a69387fa.

Would it be appropriate to explicitly list the parameters being removed in a warning? Or are you instead thinking of a smaller guiding the user's attention to the difference in available parameters?

[keflavich]

The deprecation strategy will already do that, but I'm concerned: if we're losing functionality, then we shouldn't just straight up replace the old code; instead, these should live side-by-side.

However, it's not entirely clear to me that we do lose functionality. Yes, we lose keywords, but do those keywords have equivalents in SIAP? i.e., can the same images be retrieved with a different set of keywords? If so, we should provide users with at least some examples of how that would look.

[duytnguyendtn]

Hi all, thanks for your patience! Coming back to this after a bit of hiatus, I spoke with members of the HEASARC regarding how to move forward and we think we'd like to refactor our SIA service on our end first to support the missing arguments described here. Because a lot of the deprecation work that was added to this branch won't be needed (and we don't have a timeline to when this SIA work will be complete), I'm going to close this PR to get this off of your radar and reintroduce it once we (A) finish supporting the missing arguments and (B) I address the above comments the Astroquery team has made.

Thanks again! I'll be back with a second attempt at this!