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

Fix PSF evaluate error at low energy and high offset #1150

Merged
merged 6 commits into from Sep 22, 2017

Conversation

Projects
None yet
2 participants
@bkhelifi
Contributor

bkhelifi commented Sep 21, 2017

From tests made by Yves and Fabio, the PSF machinery crashed when one scan the space parameters where the PSF is not defined, e.g. at very low energy at large offset.
I propose fixes at the level of constructors at low level..

@bkhelifi bkhelifi requested a review from cdeil Sep 21, 2017

@cdeil cdeil self-assigned this Sep 22, 2017

@cdeil cdeil added the bug label Sep 22, 2017

@cdeil cdeil added this to the 0.7 milestone Sep 22, 2017

@cdeil cdeil changed the title from PSF functions crashed when outside the range of valid values to Fix PSF evaluate error at low energy and high offset Sep 22, 2017

@cdeil

@bkhelifi - I would suggest to extend the test in two ways:

  • add an assert for the current "failure case" that establishes the the current return value (presumably the 1D TablePSF computed has value 0 for all entries?
  • add a second test case at a different offset / energy where a PSF exists (e.g. offset 1 deg, energy 1 TeV), and assert that the resulting PSF is reasonable (e.g. R68 ~ 0.04 deg) via an assert_allclose. We currently don't have any tests for CTA 1DC data, so it would be good to establish via this test that it's working.

@bkhelifi - I'd be happy to add those lines and merge, but after the HGPS call. It's at 10 am and I'll come over at ~ 11 am after. There is fresh coffee in your room.

@cdeil

cdeil approved these changes Sep 22, 2017

I added assert statements to the test in aca886a .

Merging this now.
@bkhelifi @facero - Can you please try again and see if there's still any issue for your use cases.

@cdeil cdeil merged commit 7df332f into gammapy:master Sep 22, 2017

0 of 2 checks passed

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