SkyView Overlay Options not working

[duytnguyendtn]

Hello Astroquery Maintainers!

I can't seem get some Skyview.get_image args to work (specifically grid, gridlabels, and lut). As far as I understand, after consulting with my HEASARC colleagues, these overlay options only apply to SkyView's JPEG output, not the FITS output. I mention this because I think Astroquery.Skyview ONLY returns the FITS output:

astroquery/astroquery/skyview/core.py

Lines 288 to 292 in 722385a

	for a in bs.find_all('a'):
	if a.text == 'FITS':
	href = a.get('href')
	urls.append(urlparse.urljoin(response.url, href))
	return urls

Is there a working example somehow showing these arguments working in astroquery, or are they broken?

The context is my (currently closed, but actively working on) PR #2849 to update Skyview's internals from webscraping to proper VO SIAP calls. There are some arguments that are available via the current webscraping implementation, but aren't available currently via our SIAP endpoint. I'm working to add those supported arguments to SkyView before updating astroquery.SkyView, but I can't get some of these arguments to work in the current version to begin with

[keflavich]

Right, in this case, I think these arguments don't make sense: they should be "if filetype is jpeg, allow these arguments", but we never implemented that logic. For FITS returns, these arguments are meaningless. They can be discarded, probably without even warning the user since no user could ever have used them.

[duytnguyendtn]

Thanks @keflavich! Am I correct in interpreting

They can be discarded, probably without even warning

means without going through the deprecation process and not tagging the args with astropy's deprecation decorators? I think that makes sense because, as you say, these options could not have done anything, and I think it would be useful to the users to be notified with an error IF they THOUGHT it was doing something. But I wanted to be crystal clear here.

For what it's worth, adding a jpeg return option isn't particularly high on our priority list at the HEASARC.

[keflavich]

Yes, that's what I'm saying. Given that the keywords never worked, it seems unnecessary to go through the deprecation process.

[bsipocz]

I'm a little bit late to this party, but would still suggest to go through the deprecation, as that's what we do with the other modules too that has non-functional arguments.

E.g. a user may have legitimately working code where they specify these (even though they do not affect their queries), and with the straight-on removal we will break their code without a warning.

(the one exception where I would say to remove it without deprecation is a module is being significantly refactored due to e.g. upstream changes where we cannot keep API compatibility)

[duytnguyendtn]

Thanks for the input, @bsipocz! Is there a process to reach consensus between your and @keflavich thoughts? Would definitely appreciate the green light before I start work to deprecate the arguments