Added a new function to convert SkyCoord to pixel coordinates by astrofrog · Pull Request #2885 · astropy/astropy

astrofrog · 2014-08-22T16:16:55Z

@mdboom @nden - would something like this be acceptable in astropy.wcs? I need it in various project and it will also make the implementation of #2210 easy.

If you think it would be ok, then I can go ahead and add some tests and a changelog entry.

astrofrog · 2014-08-22T16:20:21Z

There is the question of whether we can in some cases interpret the third coordinate of SkyCoord (if present but I can't think of a WCS where this would be appropriate).

cc @eteq and @taldcroft, since they've also thought a lot about coordinates :)

astrofrog · 2014-08-22T16:32:36Z

Of course, I could easily add pixel_to_skycoord too :)

eteq · 2014-08-22T17:12:23Z

perhaps the origin keyword should be available in the convinience function? That is, change the signature of this function to (coords, wcs, wcsorigin=0), and then to the world2pix call as wcs.wcs_world2pix(lon, lat, origin)

Ah yes, of course

eteq · 2014-08-22T17:13:21Z

👍 to this, but I would say it should also include pixel_to_skycoord (although of course no point in doing so if other people are against this for some reason)

astrofrog · 2014-08-22T17:31:28Z

@eteq - yes, I'll wait to make sure there are no objections, the inverse function is trivial to add.

mdboom · 2014-08-25T16:19:56Z

~~I have no objections. Should this be a free function or a method though?~~

EDIT: I see with #2210 where this is ultimately going, so this is fine as-is.

Cadair · 2014-08-26T07:57:41Z

Is there no better way of doing this? The reason I ask is SunPy uses a custom spherical representation to deal with 180 wrapping, which this will undo.

Yes, I can use coords.data.lon instead but I should fall back on coords.spherical.lon if the representation doesn't have lon/lat.

That will probably work, still feels a little hacky (in general) though.

Using data.lon shouldn't be hacky in the sense that this is the true underlying representation. In your view, should creating a new spherical representation preserve the Longitude/Latitude classes/settings? If so, it might make sense to open a new issue for this to discuss it.

Having it do that would be easier, but it might not always be the desired behaviour. I am just concerned that in general there is no real provision for extracting data in predictable fashion which preserved things like custom representations.

(as an aside, I think there should be a way to convert a representation to a Quantity object for piping into different code)

astrofrog · 2014-09-02T16:16:44Z

@mdboom - I've added a pixel to skycoord conversion function to. So at the moment my open questions are:

the functions use the wcs.wcs_* functions but of course maybe we want to use all_* or maybe allow both (or more?)
at the moment, only 2-d WCS are supported because otherwise if one has a 3-d cube where the celestial WCS is correlated with the spectral dimension, things won't work properly.

I'm not sure what the preferred solution is in each case - @mdboom, do you have any suggestions?

astrofrog · 2014-09-08T19:29:44Z

@mdboom - do you have any thoughts on the questions I had above? The issue of 2d vs higher dimensional is probably less urgent, but I'm not sure what the best approach is for allowing the user to select between the different functions. Maybe it would be better after all to simply integrate this at the WCS level rather than having separate functions?

astrofrog · 2014-09-18T19:33:30Z

@mdboom - just a reminder, if you get a chance, could you let me know what you think about the possible solutions described above?

mdboom · 2014-09-19T12:37:44Z

@astrofrog wrote:

the functions use the wcs.wcs_* functions but of course maybe we want to use all_* or maybe allow both (or more?)

I think using all_* is the least confusing, except that it may result in an iterative solution is a distortion image is included in the WCS. I guess make it an kwarg, with all being the default.

@astrofrog wrote:

at the moment, only 2-d WCS are supported because otherwise if one has a 3-d cube where the celestial WCS is correlated with the spectral dimension, things won't work properly.

This won't work if there are SIP or distortion information, but if you call sub(WCSSUB_CELESTIAL) it will extract the celestial axes and fail if they are corelated with any other axes.

So maybe do this: If there is sip or distortion, and naxis != 2, throw an exception. If there is neither sip or distorion, call sub(WCSSUB_CELESTIAL), and let the exception bubble out if it fails.

astrofrog · 2014-09-19T13:02:48Z

@mdboom - thanks, that all sounds reasonable. Since I've never really had to deal with distortions before, is it correct that I can check wcs.sip is None to check if there are SIP distorions? What other kind of distortions can be present, and how do I test for that?

mdboom · 2014-09-19T14:09:58Z

Yes -- you can juts check is None for any of the distorations. There are also lookup table distortions. The complete list is here: http://docs.astropy.org/en/stable/api/astropy.wcs.WCSBase.html#wcsbase

astrofrog · 2014-09-19T14:43:50Z

@mdboom - ok, thanks! I've now implemented your comments, so I think this is ready for final review (after a rebase it seems)

mdboom · 2014-09-19T15:15:02Z

Maybe make this a method on astropy.wcs.WCS?

I've done this and have made it a propery

astrofrog · 2014-09-19T16:43:17Z

@mdboom - I've addressed your comments, so if Travis passes this is good to go!

…g distortions) and pure WCS. Extend tests.

astrofrog · 2014-09-29T11:03:19Z

@mdboom - I have now rebased this, can this be merged if Travis passes?

mdboom · 2014-09-29T15:10:07Z

I think this needs a @property decorator to get the tests to pass.

Oops, I think this was lost during a rebase.

astrofrog · 2014-09-29T21:10:15Z

@mdboom - I addressed your comment and travis passes, so I'll go ahead and merge tomorrow if there are no remaining comments (though feel free to merge first if you come across this)

Added a new function to convert SkyCoord to pixel coordinates

astrofrog added the wcs label Aug 22, 2014

astrofrog mentioned this pull request Aug 22, 2014

WCS should accept / return coordinates objects #812 #2210

Closed

eteq reviewed Aug 22, 2014
View reviewed changes

astrofrog mentioned this pull request Aug 23, 2014

Allow user to use aperture classes for the photometry astropy/photutils#138

Merged

Cadair reviewed Aug 26, 2014
View reviewed changes

embray force-pushed the master branch from cd9dfdf to 5d7acd6 Compare August 29, 2014 21:15

astrofrog added the Ready-for-final-review label Sep 19, 2014

astrofrog added this to the v1.0.0 milestone Sep 19, 2014

astrofrog force-pushed the skycoord-to-pixel branch from 805791e to 6f8d00c Compare September 19, 2014 14:44

mdboom reviewed Sep 19, 2014
View reviewed changes

astrofrog added 5 commits September 29, 2014 12:57

Added a new function to convert SkyCoord to pixel coordinates

2553c95

Added missing imports

df1d359

Add origin parameter

99a2160

Added pixel_to_skycoord and unit test

34b3202

Fix instantiation of SkyCoord

bea732e

astrofrog added 5 commits September 29, 2014 12:59

Implement mode option to select between full transformation (includin…

7f75bf4

…g distortions) and pure WCS. Extend tests.

Added changelog entry

efb3c13

Made has_distortion a property

6cc6f40

Minor rewording

15310a1

Updated changelog

4a8e748

astrofrog force-pushed the skycoord-to-pixel branch from 1546b10 to 4a8e748 Compare September 29, 2014 11:02

mdboom reviewed Sep 29, 2014
View reviewed changes

astrofrog added 2 commits September 29, 2014 17:13

Added missing @Property

740582d

PEP8

0d2f7a8

astrofrog added a commit that referenced this pull request Sep 30, 2014

Merge pull request #2885 from astrofrog/skycoord-to-pixel

820bf52

Added a new function to convert SkyCoord to pixel coordinates

astrofrog merged commit 820bf52 into astropy:master Sep 30, 2014

eteq mentioned this pull request Sep 30, 2014

Add Skycoord.to_pixels_wcs #3002

Merged

mcara mentioned this pull request Nov 17, 2014

has_distortion needs to be clearly documented #3112

Merged

embray removed the Ready-for-final-review label Jan 13, 2015

astrofrog deleted the skycoord-to-pixel branch July 5, 2016 18:46

bsipocz mentioned this pull request Jun 27, 2017

WCS should accept / return coordinates objects #812

Closed

Conversation

astrofrog commented Aug 22, 2014

Uh oh!

astrofrog commented Aug 22, 2014

Uh oh!

astrofrog commented Aug 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eteq commented Aug 22, 2014

Uh oh!

astrofrog commented Aug 22, 2014

Uh oh!

mdboom commented Aug 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 2, 2014

Uh oh!

astrofrog commented Sep 8, 2014

Uh oh!

astrofrog commented Sep 18, 2014

Uh oh!

mdboom commented Sep 19, 2014

Uh oh!

astrofrog commented Sep 19, 2014

Uh oh!

mdboom commented Sep 19, 2014

Uh oh!

astrofrog commented Sep 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 19, 2014

Uh oh!

astrofrog commented Sep 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 29, 2014

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants