search

legacysurvey / imagine

The code that runs http://legacysurvey.org/viewer -- map-like viewer for astronomical images, especially the Dark Energy Camera Legacy Survey
BSD 3-Clause "New" or "Revised" License
27 stars 11 forks source link

Add option to draw lslga ellipses on cutout images #48

Closed ver09934 closed 5 years ago

dstndstn commented 5 years ago

Hi Ian,

This is great! I have some minor comments that I'll make in-line, but this is impressive progress!

--dustin

dstndstn commented 5 years ago

Oh, good question. merge, I guess? Really, whatever is easiest for you. I am not fussy about keeping the history beautiful.

On Fri, Jul 19, 2019 at 1:09 PM Ian Vernooy notifications@github.com wrote:

@ver09934 commented on this pull request.

In map/views.py https://github.com/legacysurvey/decals-web/pull/48#discussion_r305446514 :

  • from PIL import Image, ImageDraw
  • img = Image.open(tilefn)
  • ra, dec = wcs.crval
  • pixscale = np.abs(wcs.cd[0]) * 3600
  • width, height = img.size
  • ralo = ra - ((width / 2) * pixscale / 3600)
  • rahi = ra + ((width / 2) * pixscale / 3600)
  • declo = dec - ((height / 2) * pixscale / 3600)
  • dechi = dec + ((height / 2) * pixscale / 3600)
  • import requests
  • json_url = 'http://legacysurvey.org/viewer/lslga/1/cat.json?ralo={}&rahi={}&declo={}&dechi={}'.format(ralo, rahi, declo, dechi)
  • r = requests.get(json_url).json()

Awesome, thanks! A quick question: to incorporate the changes into my branch, would it be preferred for me to merge or rebase?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/legacysurvey/decals-web/pull/48?email_source=notifications&email_token=AAIEH7LJGOG2C22D6ZZJ453QAHYMZA5CNFSM4IE3NVP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7AVVUA#discussion_r305446514, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIEH7PGJDTALSO25TWXVL3QAHYMZANCNFSM4IE3NVPQ .

dstndstn commented 5 years ago

And by the way, that function could return some LSLGA objects that don't actually touch the requested box, because it only checks the radius and assumes a circle, not the correct ellipse.

On Fri, Jul 19, 2019 at 1:11 PM Dustin Lang dstndstn@gmail.com wrote:

Oh, good question. merge, I guess? Really, whatever is easiest for you. I am not fussy about keeping the history beautiful.

On Fri, Jul 19, 2019 at 1:09 PM Ian Vernooy notifications@github.com wrote:

@ver09934 commented on this pull request.

In map/views.py https://github.com/legacysurvey/decals-web/pull/48#discussion_r305446514 :

  • from PIL import Image, ImageDraw
  • img = Image.open(tilefn)
  • ra, dec = wcs.crval
  • pixscale = np.abs(wcs.cd[0]) * 3600
  • width, height = img.size
  • ralo = ra - ((width / 2) * pixscale / 3600)
  • rahi = ra + ((width / 2) * pixscale / 3600)
  • declo = dec - ((height / 2) * pixscale / 3600)
  • dechi = dec + ((height / 2) * pixscale / 3600)
  • import requests
  • json_url = 'http://legacysurvey.org/viewer/lslga/1/cat.json?ralo={}&rahi={}&declo={}&dechi={}'.format(ralo, rahi, declo, dechi)
  • r = requests.get(json_url).json()

Awesome, thanks! A quick question: to incorporate the changes into my branch, would it be preferred for me to merge or rebase?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/legacysurvey/decals-web/pull/48?email_source=notifications&email_token=AAIEH7LJGOG2C22D6ZZJ453QAHYMZA5CNFSM4IE3NVP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7AVVUA#discussion_r305446514, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIEH7PGJDTALSO25TWXVL3QAHYMZANCNFSM4IE3NVPQ .

ver09934 commented 5 years ago

OK, thanks. Whether the ellipses are in the box or not shouldn't cause any problems; they will just get drawn outside the frame, which PIL doesn't seem to care about.

dstndstn commented 5 years ago

Great, just thought I'd mention the potential pitfall!

On Fri, Jul 19, 2019 at 1:20 PM Ian Vernooy notifications@github.com wrote:

OK, thanks. Whether the ellipses are in the box or not shouldn't cause any problems; they will just get drawn outside the frame, which PIL doesn't seem to care about.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/legacysurvey/decals-web/pull/48?email_source=notifications&email_token=AAIEH7IZGHL7SQPOUO2SW23QAHZVTA5CNFSM4IE3NVP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2MHNRA#issuecomment-513308356, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIEH7KDL57ON5NEWVXM3A3QAHZVTANCNFSM4IE3NVPQ .

dstndstn commented 5 years ago

This is now live in the "dev" version:

http://legacysurvey.org//viewer-dev/cutout.jpg?ra=25.9303&dec=4.2215&pixscale=0.5&layer=dr8&lslga

Thanks, Ian!

(Do we want to change the color? Or allow the color to be specified as a URL argument? Apparently the color on the interactive version in #3388ff.)

ver09934 commented 5 years ago

That's awesome!

Perhaps we could do something like this for the color:

ellipse_color = '#' + req.GET.get('lslgacolor', '#3388ff').lstrip('#')
draw.ellipse(box_corners, fill=None, outline=ellipse_color, width=3)
dstndstn commented 5 years ago

That seems like a great option!

ver09934 commented 5 years ago

Should I submit another pull request with these changes?

moustakas commented 5 years ago

Should I submit another pull request with these changes?

Yup, a 1-line PR is the way to go.