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 evaluation of asymmetric background models #2033

Merged
merged 10 commits into from Feb 19, 2019

Conversation

4 participants
@watsonjj
Copy link
Contributor

watsonjj commented Feb 14, 2019

Previously, the background map returned from make_map_background_irf assumed a symmetric background IRF in the FoV. This PR correctly calculates the FoV coordinates to evaluate the background at. This is achieved with the FixedPointingInfo class, which eases the conversion from sky coordinates to FoV coordinates.

The FixedPointingInfo is generated from the pointing metadata contained in the EVENTS hdu.

Adresses #1987

@adonath adonath added the feature label Feb 15, 2019

@adonath adonath added this to To do in Map analysis via automation Feb 15, 2019

@adonath adonath added this to the 0.11 milestone Feb 15, 2019

@adonath adonath requested review from lmohrmann and registerrier Feb 15, 2019

@registerrier
Copy link
Contributor

registerrier left a comment

Thanks @watsonjj ! This looks really good. OK to merge.
Please some minor comments mostly regarding code duplication in tests.

@lazyproperty
def altaz_frame(self):
"""ALT / AZ frame (`~astropy.coordinates.AltAz`)."""
return AltAz(obstime=self.time_start, location=self.location)

This comment has been minimized.

Copy link
@registerrier

registerrier Feb 15, 2019

Contributor

Using the middle of the time interval might give better results on average, no?

This comment has been minimized.

Copy link
@watsonjj

watsonjj Feb 18, 2019

Author Contributor

Okay, done

def radec(self):
"""RA / DEC pointing postion from table
(`~astropy.coordinates.SkyCoord`)"""
print(self.meta.keys())

This comment has been minimized.

Copy link
@registerrier

registerrier Feb 15, 2019

Contributor

Why the print here?

This comment has been minimized.

Copy link
@watsonjj

watsonjj Feb 18, 2019

Author Contributor

Removed

)

assert m.data.shape == pars["shape"]
assert m.unit == ""
assert_allclose(m.data.sum(), pars["sum"], rtol=1e-5)


def test_make_map_background_irf_constant(bkg_3d_constant, fixed_pointing_info):

This comment has been minimized.

Copy link
@registerrier

registerrier Feb 15, 2019

Contributor

Maybe the tests here could be parametrized to limit the code duplication?

@@ -18,10 +27,70 @@ def bkg_3d():
return Background3D.read(filename, hdu="BACKGROUND")


def geom(map_type, ebounds):
@pytest.fixture(scope="session")
def bkg_3d_constant():

This comment has been minimized.

Copy link
@registerrier

registerrier Feb 15, 2019

Contributor

Maybe reduce code duplication by passing argument to bkg_3d?

Map analysis automation moved this from To do to In progress Feb 18, 2019

@watsonjj

This comment has been minimized.

Copy link
Contributor Author

watsonjj commented Feb 18, 2019

I've tried to remove code duplication, and I have also improved the testing for different symmetries. Following #2035 the tests should pass.

@registerrier
Copy link
Contributor

registerrier left a comment

Thanks @watsonjj . Looks good!

@lmohrmann
Copy link
Contributor

lmohrmann left a comment

Looks good to me! I'll do another sanity check on my side once this is merged...

@adonath adonath assigned adonath and unassigned lmohrmann Feb 18, 2019

@watsonjj watsonjj dismissed stale reviews from lmohrmann and registerrier via 39ee1c7 Feb 19, 2019

@watsonjj watsonjj force-pushed the watsonjj:background_asymmetric branch from b9ad17f to 39ee1c7 Feb 19, 2019

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Feb 19, 2019

The Travis fails seem unrelated. I'll merge and fix remaining test fails in master. Thanks @watsonjj!

@adonath adonath merged commit d6614c8 into gammapy:master Feb 19, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 28 updated code elements – Tests: passed
Details

Map analysis automation moved this from In progress to Done Feb 19, 2019

@adonath adonath changed the title Correct asymmetric background map generation Add evaluation of asymmetric background models in alt-az aligned fov coordinates Mar 26, 2019

@adonath adonath changed the title Add evaluation of asymmetric background models in alt-az aligned fov coordinates Add evaluation of asymmetric background models Mar 29, 2019

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.