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

Improve SkyMap.coordinates #544

Merged
merged 3 commits into from Jun 9, 2016

Conversation

Projects
None yet
3 participants
@OlgaVorokh
Member

OlgaVorokh commented Jun 2, 2016

On this PR I have moved out pixel coordinates into a separate method. In the SkyMap.coordinates coord_type has been removed. Also I update the docstrings, tests and other functions

Returns
-------
coordinates : tuple or `~astropy.coordinates.SkyCoord`

This comment has been minimized.

@adonath

adonath Jun 3, 2016

Member

Return type tuple is not correct anymore, should be removed from the docs.

@@ -220,18 +220,19 @@ def write(self, filename, *args, **kwargs):
hdu = self.to_image_hdu()
hdu.writeto(filename, *args, **kwargs)
def coordinates(self, coord_type='skycoord', origin=0, mode='center'):
def pix_coordinates(self, mode='center'):

This comment has been minimized.

@adonath

adonath Jun 3, 2016

Member

Totally minor, but can you rename it to coordinates_pix()? This has the slight advantage, that when typing coord... and hitting tab completion, it will be listed in IPython.

def solid_angle(self):
"""
Solid angle image
"""
xsky, ysky = self.coordinates(coord_type='galactic', mode='edges')
coordinates = self.coordinates(mode='edges')
xsky = coordinates.galactic.l.wrap_at('180d')

This comment has been minimized.

@cdeil

cdeil Jun 4, 2016

Member

You shouldn't force Galactic coordinates here and in other places in this PR.

What you want is just the lon / lat in whatever coordinate frame the image is (e.g. galactic or icrs or ...).

So instead of calling:

xsky = coordinates.galactic.l.wrap_at('180d')
ysky = coordinates.galactic.b

you should call

xsky = coordinates.data.lon
ysky = coordinates.data.lat

and then you'll get the lon / lat in the coordinate frame of the image

There will be very few cases in Gammapy where we want to force the frame to galactic or icrs.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 4, 2016

Please rebase this branch against master.

@cdeil cdeil added the cleanup label Jun 4, 2016

@cdeil cdeil added this to the 0.5 milestone Jun 4, 2016

@cdeil cdeil self-assigned this Jun 4, 2016

@OlgaVorokh

This comment has been minimized.

Member

OlgaVorokh commented Jun 4, 2016

@cdeil thanks! Is there a documentation aside from http://docs.astropy.org/en/stable/api/astropy.coordinates.SkyCoord.html on the fields of SkyCoord? I didn't knew that it had .lon .lat fields because they are different for each frame.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 4, 2016

SkyCoord doesn't have lon and lat attributes, it has a data attribute which has lon and lat if the representation is "spherical" or "unitspherical". :-)

SkyCoord is a very complex convenience object on top of a lot of stuff.
There's quite some documentation here:
http://docs.astropy.org/en/stable/coordinates/index.html

The attributes are described a bit here:
http://docs.astropy.org/en/stable/coordinates/skycoord.html#attributes

I'm not a SkyCoord expert, usually I just play around in IPython with an example to find out how it works, and sometimes browse the docs. If you have any questions, just ask me or at https://gitter.im/astropy/astropy .

@OlgaVorokh

This comment has been minimized.

Member

OlgaVorokh commented Jun 4, 2016

@cdeil I have an issue: when I replace

xsky = coordinates.galactic.l.wrap_at('180d')
ysky = coordinates.galactic.b

with

xsky = coordinates.data.lon
ysky = coordinates.data.lat

I get the following error:

@requires_data('gammapy-extra')
    def test_lon_lat_rectangle_mask():
        counts = SkyMap.from_image_hdu(FermiGalacticCenter.counts())
        coordinates = counts.coordinates()
        lons = coordinates.data.lon
        lats = coordinates.data.lat
        #lons = coordinates.galactic.l.wrap_at('180d')
        #lats = coordinates.galactic.b
        mask = lon_lat_rectangle_mask(lons.degree, lats.degree, lon_min=-1,
                                      lon_max=1, lat_min=-1, lat_max=1)
>       assert_allclose(mask.sum(), 400)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-07, atol=0
E       
E       (mismatch 100.0%)
E        x: array(200)
E        y: array(400)

gammapy/image/tests/test_utils.py:186: AssertionError

What should I do: change tests or return to the last version of code?

@adonath

This comment has been minimized.

Member

adonath commented Jun 6, 2016

@OlgaVorokh
You still need the call to lon.wrap_at('180d') to get a symmetric longitude range. Right now, there's a
jump from 0 to 360 deg at longitude 0. Maybe this should be rather handled in lon_lat_rectangle_mask, depending on the parameters lon_min and lon_max.

Split coordinates into coordinates_pix, update all functions that dep…
…end on it, remove galactic mode from coordinates

@OlgaVorokh OlgaVorokh force-pushed the OlgaVorokh:coordinates branch from 7b7d1b9 to 0e77772 Jun 9, 2016

OlgaVorokh added some commits Jun 9, 2016

@cdeil cdeil merged commit c690866 into gammapy:master Jun 9, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment