Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SDSS image cutout query tool #1383

Closed
wants to merge 8 commits into from
Closed

Add SDSS image cutout query tool #1383

wants to merge 8 commits into from

Conversation

kakirastern
Copy link
Contributor

Fixes #444.

To add two convenience SDSS image cutout tools in response to @eteq's suggestions. The first case is as described in Issue #444. So when one does

sc = SkyCoord(1*u.deg, 2*u.deg)
view_in_sdss_navigate(sc)

the tool then uses webbrowser to open a new browser tab in the SDSS "navigate" interface, at http://skyserver.sdss.org/dr12/en/tools/chart/navi.aspx?ra=1&dec=2&scale=0.3&opt=GO .

The second case proved to be problematic if implemented as envisioned. This is because not only the 'ra' and 'dec' need to be supplied, but also the 'target_name' needs to be provided. So I simplified the function to

view_in_sdss_imagelist()

This is so that at the call of the property-like function, the SDSS "image list" interface would be brought up in a new browser tab at https://skyserver.sdss.org/dr15/en/tools/chart/listinfo.aspx.

I have tested the added file locally, and everything seems to be in working order. Any suggestion for further improvements would be appreciated.

@pep8speaks
Copy link

pep8speaks commented Mar 17, 2019

Hello @kakirastern! 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! 🍻

Comment last updated at 2020-09-24 14:20:18 UTC

@bsipocz bsipocz added this to the v0.3.10 milestone Mar 17, 2019
@kakirastern
Copy link
Contributor Author

Reckon I might have used the data for the requests POST request in the incorrect format... may be that's why I could not get the second case working as originally conceived. Will investigate soon and update with another commit to follow up.

@bsipocz
Copy link
Member

bsipocz commented Mar 18, 2019

Before you put more effort in it, I think it should be a method for the current SDSS() class rather than a standalone one. Don't have time to do a proper review atm.

@kakirastern
Copy link
Contributor Author

Okay, cool. That should be relatively easy to fix... I will do them both in a single commit (or two).

@kakirastern
Copy link
Contributor Author

Figured it out now... will make the changes in the PR soon.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #1383 into master will decrease coverage by 0.05%.
The diff coverage is 19.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1383      +/-   ##
==========================================
- Coverage   64.51%   64.45%   -0.06%     
==========================================
  Files         200      200              
  Lines       16022    16043      +21     
==========================================
+ Hits        10337    10341       +4     
- Misses       5685     5702      +17     
Impacted Files Coverage Δ
astroquery/sdss/core.py 81.60% <19.04%> (-4.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2676dd4...1409e7c. Read the comment docs.

@kakirastern
Copy link
Contributor Author

kakirastern commented Mar 19, 2019

So to recap on what I have done... I have added in two image cutout query tools to the SDSS() class as suggeted by @eteq. Such that now I can access the image cutout service of SDSS using two of their tools using Python. So if I do:

from astropy.coordinates import SkyCoord
from astropy import units as u
from astroquery.sdss import SDSS
sc = SkyCoord(1*u.deg, 2*u.deg)
SDSS.view_in_sdss_navigate(sc)

It then uses webbrowser to open a browser tab in the SDSS "navigate" interface at http://skyserver.sdss.org/dr15/en/tools/chart/navi.aspx?ra=1&dec=2&opt=GO. In the second case, if I do the following steps:

scs = SkyCoord([1, 2]*u.deg, [3,4]*u.deg)
SDSS.view_in_sdss_imagelist(scs)

Then it shows me the SDSS "image list" tool at http://skyserver.sdss.org/dr15/en/tools/chart/listinfo.aspx.

@kakirastern
Copy link
Contributor Author

The PR is now ready for a preliminary review.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've requested a few changes, but the PR looks like it's mostly in good shape.

This is kind of a weird tool that doesn't fit that well within the rest of the astroquery landscape. It needs its own documentation. I would suggest including web browser screenshots too, since this one necessarily opens a web browser window.

astroquery/sdss/core.py Outdated Show resolved Hide resolved
astroquery/sdss/core.py Outdated Show resolved Hide resolved
astroquery/sdss/core.py Show resolved Hide resolved
@kakirastern
Copy link
Contributor Author

Yeah, agreed that special documentation would be needed for this pair of tools. I will include screenshots as well, as the user is redirected to a webpage by webbrowser.

@kakirastern
Copy link
Contributor Author

Added documentation for the new tools. Ready for a review.

@keflavich
Copy link
Contributor

@eteq, when you have a chance - is this what you had in mind in #444 ?

@bsipocz bsipocz removed this from the v0.3.10 milestone Jun 3, 2019
@bsipocz
Copy link
Member

bsipocz commented Sep 1, 2020

@keflavich - shall we move ahead with this, or still waiting for a review from @eteq?

@keflavich
Copy link
Contributor

let's move ahead.

@bsipocz bsipocz added this to the v0.4.2 milestone Sep 1, 2020
@bsipocz
Copy link
Member

bsipocz commented Sep 1, 2020

OK, I'll wrap it up this week then (after all it's AstroHackWeek :) )

@bsipocz
Copy link
Member

bsipocz commented Sep 16, 2020

@kakirastern - please rebase to retrigger CI before we do a final review.

@kakirastern
Copy link
Contributor Author

Sorry for the late response, but I will follow up within the next few days to rebase my PR.

@kakirastern
Copy link
Contributor Author

Sorry guys, has been experiencing some really awkward editor issues with VS Code when rebasing. So switched over to use PyCharm instead. But might have triggered some extra messages to the watchers of this repo. Apologies for any inconvenience made.

@kakirastern
Copy link
Contributor Author

Added a changelog for this PR.

@kakirastern
Copy link
Contributor Author

Rebased

@kakirastern
Copy link
Contributor Author

All crucial CI tests have been passed! Ready for a code review.

@bsipocz
Copy link
Member

bsipocz commented Nov 7, 2020

I think my new comment on the issue needs a discussion first, before moving ahead with a rereview of this PR:

#444 (comment)

@eteq
Copy link
Member

eteq commented Dec 15, 2020

closed at request of author, last SHA 1409e7c

@eteq eteq closed this Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDSS img cutout query tool?
5 participants