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

Fix excess for given significance computation #1289

Merged
merged 3 commits into from Feb 14, 2018

Conversation

@oscarblanchbigas
Copy link
Contributor

@oscarblanchbigas oscarblanchbigas commented Feb 7, 2018

Adapt test_sensitivity; do the root finding of Lima formula with its square and make it working for requested significance close to 0; and add a check that we look for positive significance.

@cdeil - Please check the negative excess handling and if this really works for arrays.

Adapt test_sensitivity; do the root finding of Lima formula with its square and make it working for requested significance close to 0; and add a check that we look for positive significance.
@cdeil cdeil self-assigned this Feb 7, 2018
@cdeil cdeil added the bug label Feb 7, 2018
@cdeil cdeil added this to the 0.7 milestone Feb 7, 2018
@kosack
Copy link

@kosack kosack commented Feb 7, 2018

here's some old sensitivity code I made that could help:

https://bitbucket.org/kosack/commonsens/src

see the sensitivity.py and stats.py

@cdeil cdeil mentioned this pull request Feb 9, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Feb 9, 2018

I've attached a commit here ( 4d0f8a4 ) that (hopefully) improves the tests and code in gammapy.stats.poisson. We discussed two days ago and thought that "sensitivity" is unclear, so I changed to "excess_matching_significance" which is pretty clear, no? This is a low-level stats function that wouldn't be called often, just when computing flux sensitivities for spectra or maps.

The implementation is also changed a bit compared to what @oscarblanchbigas you put - after thinking a bit about it, the case that really needs to be special-handled is n_on <= 0, for the cases where the caller asks for an excess matching a very low significance, that isn't possible with n_on >=0, or when the optimiser runs into that range n_on <= 0.

I think this is working, and all that's missing here is to adjust the test to have reference value NaN for those "impossible cases".

But I wanted to ask for some feedback / review here concerning this thinking and the new implementation. Is it correct? Is it numerically stable for all cases? Can it be written in a better way (using Python/numpy/scipy only, without going to Cython or Numba)? @oscarblanchbigas @mackaiver @kosack @adonath - if you have time, please have a look.

@cdeil cdeil removed their assignment Feb 14, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Feb 14, 2018

@oscarblanchbigas doesn't have time this week.

@mackaiver - could you please review this and give some feedback (or edit directly and add a commit)? I'd like to finish this up.

@kbruegge
Copy link
Contributor

@kbruegge kbruegge commented Feb 14, 2018

Look's okay to me. Don't know how edge cases should be handled so I can't say much about that. Howerver what about the code which is currently here? Seems like there are again two solutions to the same problem.
gammapy/scripts/cta_sensitivity.py ?

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 14, 2018

The idea would be to update gammapy/scripts/cta_sensitivity.py and to call the gammapy.stats function from there. Before doing it, a test with an assert on that excess number should be added.
I'll just finish up here and merge this later today and leave that to you or @bkhelifi for a follow-up PR.

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 14, 2018

@mackaiver - Thanks for the review!

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 14, 2018

Now with f7a9564 I think this is really working correctly.

@mackaiver @oscarblanchbigas - If you have time, please have another look.

If not, I'll still merge this tomorrow morning and then everyone can try it via master and comments are still welcome of course.

Copy link
Contributor

@kbruegge kbruegge left a comment

Yes. Much better now.

@cdeil cdeil changed the title Fix bugs and functions called to compute on/off sensitivity Improve stats function to compute excess for given Li&Ma significance Feb 14, 2018
@cdeil cdeil self-assigned this Feb 14, 2018
@cdeil cdeil merged commit 1deef22 into gammapy:master Feb 14, 2018
1 of 2 checks passed
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
Copy link
Member

@cdeil cdeil commented Feb 14, 2018

@oscarblanchbigas - Thanks for this contribution!

@cdeil cdeil changed the title Improve stats function to compute excess for given Li&Ma significance Fix excess for given significance computation Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants