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

Improve field-of-view coordinate transformations #2009

Merged
merged 4 commits into from Jan 29, 2019

Conversation

Projects
3 participants
@lmohrmann
Copy link
Contributor

lmohrmann commented Jan 29, 2019

Following discussion in #1987, this PR improves the field-of-view coordinate transformations by using astropy utilities, namely the SkyOffsetFrame.

I've kept the old versions as comments, since the new ones are apparently much slower (in a very quick test I did). If performance ever becomes an issue (which is not so unlikely), we may want to consider to go back to the old transformations.

@cdeil
Copy link
Member

cdeil left a comment

Two small suggestions inline. Otherwise LGTM , thanks.

Show resolved Hide resolved gammapy/utils/coordinates/fov.py Outdated
Show resolved Hide resolved gammapy/utils/coordinates/fov.py Outdated
@lmohrmann

This comment has been minimized.

Copy link
Contributor Author

lmohrmann commented Jan 29, 2019

I deleted the old code and made the change you suggested, @cdeil.
Thanks!

@cdeil cdeil self-assigned this Jan 29, 2019

@cdeil cdeil added the cleanup label Jan 29, 2019

@cdeil cdeil added this to To do in IRFs via automation Jan 29, 2019

@cdeil cdeil added this to the 0.11 milestone Jan 29, 2019

@cdeil

cdeil approved these changes Jan 29, 2019

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jan 29, 2019

@lmohrmann - Thanks!

I think SkyOffsetFrame was added in astropy/astropy#5023 in 2016 in Astropy 1.2, so should be OK to use in Gammapy.

There's of course the question if we should implement FOV frame using astropy.coordinates directly. But this PR is a big step forward towards simpler code, so I'm merging this in now without further discussion.

Once we have a first caller / use of this functionality or two, we can improve further - suggestions or PRs welcome any time.

@cdeil cdeil merged commit 0f7a518 into gammapy:master Jan 29, 2019

1 of 3 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details

IRFs automation moved this from To do to Done Jan 29, 2019

@lmohrmann lmohrmann deleted the lmohrmann:improve-fov-trafos branch Jan 29, 2019

# Create a frame that is centered on the pointing position
center = SkyCoord(lon_pnt, lat_pnt)
fov_frame = SkyOffsetFrame(origin=center)

This comment has been minimized.

Copy link
@registerrier

registerrier Jan 29, 2019

Contributor

Wouldn't it be simpler to pass a SkyCoord for the pointing direction directly?
If I understand correctly this will work only if the pointing coordinate is in the right AltAz system, right?

This comment has been minimized.

Copy link
@lmohrmann

lmohrmann Jan 30, 2019

Author Contributor

Yes, this might be simpler, but I did not do it because I wasn't sure what would happen when the user passes a SkyCoord that is defined in another system. You're right that currently, the transformation works for the conversion between AltAz and an AltAz-aligned field-of-view system, and not e.g. between RaDec and a RaDec-aligned field-of-view system. The pointing therefore needs to be specified in AltAz coordinates, yes.

My understanding was that Gammapy will work with AltAz-aligned field-of-view systems only, and so I avoided the complication to also support a RaDec-aligned system.

It might be a good idea to make it more clear in the comments that the field-of-view system is always assumed to be AltAz-aligned.

fov_frame = SkyOffsetFrame(origin=center)

# Define coordinate to be transformed.
target_sky = SkyCoord(lon, lat)

This comment has been minimized.

Copy link
@registerrier

registerrier Jan 29, 2019

Contributor

Same here. Having a SkyCoord seems more efficient and less error prone regarding systems used.

This comment has been minimized.

Copy link
@lmohrmann

lmohrmann Jan 30, 2019

Author Contributor

For the same reason as above, I decided against passing this as a SkyCoord.


# Switch sign of longitude angle since this axis is
# reversed in our definition of the FoV-system
return -target_fov.lon, target_fov.lat

This comment has been minimized.

Copy link
@registerrier

registerrier Jan 29, 2019

Contributor

I don't know what is the definition used by CTA here, but couldn't it be simpler to keep lon and lat and simply revert the HESS background IRF to be in the same system?

This comment has been minimized.

Copy link
@lmohrmann

lmohrmann Jan 30, 2019

Author Contributor

I'm using the same transformation/definition of field-of-view system as is being used in the HESS software. That is, if you convert into field-of-view coordinates with these functions, you should get the same values as are listed in the exported EVENTS FITS file as FOV_ALTAZ_LON and FOV_ALTAZ_LAT.


# Create a frame that is centered on the pointing position
center = SkyCoord(lon_pnt, lat_pnt)
fov_frame = SkyOffsetFrame(origin=center)

This comment has been minimized.

Copy link
@registerrier

registerrier Jan 29, 2019

Contributor

I would directly pass the pointing SkyCoord.

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jan 29, 2019

To follow up on the suggestions by @registerrier to use SkyCoord in the API:

@registerrier - I was also thinking about suggesting to use SkyCoord objects as input / output. But I think it requires definition of a new frame class, to match the signs, no? I'm not sure if we want to do that, could discuss in the next Gammapy call.

@kosack - do you already use FOV coordinates in ctapipe? I think in CTA there is no definition for this yet, right? What do you think of the definitions here?

@lmohrmann - is the code here in line with the definitions here? Maybe you could send a follow-up PR to add a link to the definition in the docstring, and if you want to introduce SkyCoord as input / output, do that? Would suggest to wait for @kosack to comment though to avoid changing back and forth.

@lmohrmann

This comment has been minimized.

Copy link
Contributor Author

lmohrmann commented Jan 30, 2019

is the code here in line with the definitions here?

Yes, I think it is. At least it agrees with the transformations as used in the HESS exporter, and my understanding was that that code is in line with the specs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.