Added get_query_payload kwarg to skyview

[nkphysics]

Hi yall!

This is a small PR, but I added in the get_query_payload kwarg to SkyView.

Long story short, I was trying to do some debugging and noticed the kwarg wasn't there and found #2040.

I looked through and didn't see testing for other services/catalogs/archives that used get_query_payload so I didn't add any but I can if you would like. With the changes I made I used the kwarg in the examples to debug and it helped me out.

>>> from astroquery.skyview import SkyView
>>> SkyView.get_images(position='crab', survey=['Fermi 5', 'HRI', 'DSS'], get_query_payload=True)
{'coordinates': 'J2000', 'projection': 'Tan', 'pixels': '300', 'float': 'on', 'scaling': 'Log Sqrt Linear HistEq LogLog', 'resolver': 'SIMBAD-NED', 'Sampler': '_skip_', 'Deedger': '_skip_', 'lut': 'colortables/b-w-linear.bin', 'grid': '_skip_', 'gridlabels': '1', 'CatalogIDs': 'on', 'survey': ['Fermi 5', 'HRI', 'DSS'], 'Position': '83.6324 22.0174'}
>>> 
>>> SkyView.get_image_list(position='Eta Carinae', survey='Fermi 5', get_query_payload=True)
{'coordinates': 'J2000', 'projection': 'Tan', 'pixels': '300', 'float': 'on', 'scaling': 'Log Sqrt Linear HistEq LogLog', 'resolver': 'SIMBAD-NED', 'Sampler': '_skip_', 'Deedger': '_skip_', 'lut': 'colortables/b-w-linear.bin', 'grid': '_skip_', 'gridlabels': '1', 'CatalogIDs': 'on', 'survey': 'Fermi 5', 'Position': '161.265 -59.6844'}

There's a couple of places where if get_query_payload: seems kinda redundant, but I ended up needing them to get this working. Let me know if you want me to find a workaround to clean this up a bit.

[codecov]

Codecov Report

❌ Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.87%. Comparing base (823e49f) to head (8d3320d).
⚠️ Report is 104 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/skyview/core.py	54.54%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3318      +/-   ##
==========================================
- Coverage   69.88%   69.87%   -0.01%     
==========================================
  Files         232      232              
  Lines       19761    19769       +8     
==========================================
+ Hits        13810    13814       +4     
- Misses       5951     5955       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bsipocz]

There are bunch of superflous kwargs added, otherwise this PR looks good.

It would be nice to add a test though.

[bsipocz]

The code is now a bit confusing as the return from e.g. get_image_list is now not URLs any more, but felt back a couple layers down as is the payload from above. I mean it kind of follows what the other modules do, so I'll go ahead and merge it, but it may be a place for some future cleanup work to make it more obvious at first read.

[nkphysics]

I can take a look at changing this in a future PR.
I have the same feeling about it being confusing. Its my understanding though that the xternal request needs to be made in order to get the list of urls.

[keflavich]

Yeah, I did a quick review of this earlier and was also confused, but if the query needs to be made to get the URLs, I get it.... just, the original intent was for get_query_payload to return only those components of the query generated locally, since _async would naturally include the payload sent to the server. I can see a need for an exception here, though.

[bsipocz]

Oh, I'm sorry I made this even more confusing. As I see the code will indeed and correctly return the payload without sending it to the server. The confusing part is only coming from the naming of the variables as they will be e.g. URLs in normal mode, but the payload when the kwarg is passed along. I suspect the rest of the codebase has some level of this confusion bubbling up, so let's indeed table this for now.

[nkphysics]

I've gone ahead and made the following changes from the other comments and code review:

• removed the superfluous **kwargs.

• added a test_get_image_list_payload to test the addition of the get_query_payload kwarg

The returned payload using SkyView.get_image_list(..., get_query_payload=True) I agree is a bit confusing. I can try to find a workaround to avoid having to send a request to get the url list, and I'll submit a new PR if I do. As of right now, I couldn't find a way of getting the image list without having to make the external request.

[bsipocz]

Thanks @nkphysics, and extra thanks for the clean commit history!