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

Change IRF extrapolation behaviour #1195

Merged
merged 3 commits into from Nov 30, 2017

Conversation

Projects
None yet
2 participants
@cdeil
Member

cdeil commented Nov 29, 2017

Last week @bkhelifi reported that CTA DC-1 AGN spectrum analysis failed miserably.

The core of the issue was identified by @registerrier to lie in negative effective area and energy dispersion from the evaluate method in our IRF classes:

As demonstrated there, what we currently do is to place the interpolation nodes at the bin centers, i.e. at 0.5, 1.5, 2.5, ... deg in FOV offset, and thus since the AGN were placed at offset = 0.0 deg, we extrapolate the IRF values from the nodes at 0.5 and 1.5 deg, and this can lead to negative EDISP and predicted counts and analysis.

This can be fixed in several ways, e.g. by padding with an extra row of interpolation nodes at offset = 0 deg with the same values as at 0.5 deg on IRF read, or by implementing a custom IRF interpolator that has axis-specific behaviour and knows about energy and offset axes and how to extend.

However, the simplest solution is to clip the IRF values to >= 0, i.e. to replace negative values with zero. Doing that is also exactly what Gammalib / ctools does (linear extrapolation followed by clipping to >=0), so at least for CTA DC-1 where simulation was done by ctools, it means that we should be able to analyse the data and get pretty good results. I think there will still be issues from IRF and sampling noise when doing analysis, but that remains to be seen / studies in the coming weeks, it's not clear at the moment.

So I plan to make a pull request shortly where I introduce the clip to >= 0 shortly, applying it to all IRFs (AEFF, EDISP, BKG, table PSF)

cc @jjlk

@cdeil cdeil added bug feature labels Nov 5, 2017

@cdeil cdeil added this to the 0.7 milestone Nov 5, 2017

@cdeil cdeil self-assigned this Nov 5, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 29, 2017

Member

I added the clipping to >= 0 in NDData.evaluate, attached as a commit here: a19e769

Locally I see three test fails, because results in high-level analyses that evaluate IRFs changed very slightly: https://gist.github.com/cdeil/50b29850f074af1c898e1c663f7dac88

@registerrier or @joleroi anyone interested - OK or do you see a better way to implement the fix?

I plan to add some unit tests here for NDData and IRF evaluation that establish the behaviour concerning extrapolation and clipping to >= 0, but the fix seems OK to me. If we have cases later where we need to have negative outputs from NDData.evaluate, we could add a configure option to allow it in those cases later, no?

Member

cdeil commented Nov 29, 2017

I added the clipping to >= 0 in NDData.evaluate, attached as a commit here: a19e769

Locally I see three test fails, because results in high-level analyses that evaluate IRFs changed very slightly: https://gist.github.com/cdeil/50b29850f074af1c898e1c663f7dac88

@registerrier or @joleroi anyone interested - OK or do you see a better way to implement the fix?

I plan to add some unit tests here for NDData and IRF evaluation that establish the behaviour concerning extrapolation and clipping to >= 0, but the fix seems OK to me. If we have cases later where we need to have negative outputs from NDData.evaluate, we could add a configure option to allow it in those cases later, no?

@cdeil cdeil assigned joleroi and unassigned cdeil Nov 30, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 30, 2017

Member

@joleroi - I've restructured test_nddata.py. Thoughts?

Member

cdeil commented Nov 30, 2017

@joleroi - I've restructured test_nddata.py. Thoughts?

@joleroi

discussed offline

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 30, 2017

Member

I didn't add new tests for IRFs (only for NDDataArray), but just to mention that after this change the AEFF and EDISP in the notebooks linked to above change in the expected way, i.e. they are clipped to non-negative values:

image

image

Member

cdeil commented Nov 30, 2017

I didn't add new tests for IRFs (only for NDDataArray), but just to mention that after this change the AEFF and EDISP in the notebooks linked to above change in the expected way, i.e. they are clipped to non-negative values:

image

image

@cdeil cdeil merged commit b9850e9 into gammapy:master Nov 30, 2017

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
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 30, 2017

Member

@bkhelifi or @jjlk - now that this fix is merged, could you re-run the CTA DC-1 AGN analysis that uncovered this IRF extrapolation issue a few weeks ago?
And please commit it to https://github.com/gammasky/cta-analyses even if work in progress, so that we can discuss / figure out remaining issues?

Member

cdeil commented Nov 30, 2017

@bkhelifi or @jjlk - now that this fix is merged, could you re-run the CTA DC-1 AGN analysis that uncovered this IRF extrapolation issue a few weeks ago?
And please commit it to https://github.com/gammasky/cta-analyses even if work in progress, so that we can discuss / figure out remaining issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment