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 gammapy.stats.excess_matching_significance #1675

Merged
merged 1 commit into from Aug 8, 2018

Conversation

Projects
None yet
2 participants
@cdeil
Member

cdeil commented Aug 8, 2018

This PR implements excess_matching_significance and adds some tests, following the existing implementation and tests for excess_matching_significance_on_off.

I realise there's some code duplication and _excess_matching_significance_lima could maybe be simplified. I tried to make one root finding function and dispatch to it from both _excess_matching_significance_lima and _excess_matching_significance_lima_on_off. It worked, but I found it to be quite complex, so I preferred the ~ 10 line copy & paste & adapt over that complexity. Maybe there is a better way?

(the reason I did this was that it was one of the very few remaining cases of an xfailed test in Gammapy - we can get rid of all of them by tomorrow)

@bkhelifi @registerrier @adonath or anyone familiar with this - please review.

@cdeil cdeil added the feature label Aug 8, 2018

@cdeil cdeil added this to the 0.8 milestone Aug 8, 2018

@cdeil cdeil assigned bkhelifi and cdeil and unassigned bkhelifi Aug 8, 2018

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 8, 2018

Member

Merging this now. Comments still very welcome. If something can be improved, we can do a follow-up PR tomorrow.

Member

cdeil commented Aug 8, 2018

Merging this now. Comments still very welcome. If something can be improved, we can do a follow-up PR tomorrow.

@cdeil cdeil merged commit 5a8e757 into gammapy:master Aug 8, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@cdeil cdeil changed the title from Implement excess_matching_significance and add test to Add gammapy.stats.excess_matching_significance Aug 15, 2018

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